코드리뷰 3

common/util/StoreUtil.js

• 이 유틸리티가 필요한 이유가 무엇인가요?
  
• 유틸 구조가 모호함. 굳이 계층 구조로 만드는 이유가 있나요?
  • Utils
    
• JavaScript 에서는 obj.get("key") 이렇게 사용하는 방식 대신에  obj.key 로 쓰는게 좋을 것 같습니다.
  
  • getUtils
  • getStore
  • 디콤포지션(decomposition) 이 지원되서 편합니다.

 

 

common/util/UserUtil.js

 

 

const UserUtils = {    getLoginUserId: () => {      const UserStore = StoreUtils.getStore("UserStore");      return UserStore?.getLoginUserId();    }, };

• Util 은 보통 정적(Static) 메소드로 이루어져 있습니다
  
  • 로그인 ID 가져오는 것은 유틸의 성격에 맞지 않는 것 같습니다.
    
  • 의존 관계도 꼬입니다.
    • Store -> Repository -> Util -> Store
      
• 다른 스토어에 대한 접근은 스토어 레벨 이상에서만 했으면 합니다.

 

App.js

 

 

<Provider ViewModel={rootStore} RootStore={rootStore}> ...

• 라우팅의 뒤쪽 파트는 네스티드 라우티을 써야하는 부분 맞죠?
• viewModel 이라는 용어보다 이제는 Store 라는 용어로 통일했으면 합니다.
  

 

repositories/ChannelRepository.js

 

 

const ChannelRepository = {     getChannelList(chTypeList) {       const params = {          userId: userUtils.getLoginUserId(),          chTypeList,       };       return rest.get(`PTask/ChannelList`, params); },

• rest 라고 해야할까요?
  • 소문자로 시작하는 것은 객체를 의미하는 것인데, 그럼 보통 new 를 해서 만들어낸 것임.
  • 그런데 rest 는 클래스 레벨으로 보임

 

common/api/api.js

 

 

export const serverUrl = "http://222.122.67.166";

• configuration 으로 빼주세요
  

 

 

export const api = {    calendar: "",    file: "",    note: "", };

• 이건 무엇을 위해서 만든 것인가요? 공통 코드에 calendar? note?

 

 

export const rest = {     get(svcName, params) {        const dto = { dto: params };        const t = Date.now();        return axi.get(`${svcName}?${JSON.stringify(dto)}&c=${t}`);     },     ... };

• 여러 메소드를 포함하는 경우 가능하면 이렇게 객체 형식이 아닌 클래스를 썼으면 좋겠습니다.
  
• JSON.stringify 로 처리해서 넘기는 것은 아닌듯 보입니다. API 단 수정 필요
  • dto 로 감싸서 쿼리 파람으로 넘기는 것도 이상합니다.
    
• 뒤에 c= ${t} 이건 무엇인가요? 
  • 모든 GET API 가 c 를 사용하지는 않을 것 같은데요.
• axios 에서 query string 이나 헤더 등은 간단하게 객체 형태로 넘길 수가 있네요.
• axios 응답을 그대로 넘겨버리면 axios 에 바인딩 됩니다.
  • 자체 API 응답 포맷을 정했으면 합니다.
    

 

 

repositories/ChannelRepositiory.js

• 왜 클래스를 안쓰나요?
  • 안 쓸거면 function 으로 따로 정의하고 마지막에 function 을 묶어서 리턴하는 것이 readability 가 좋을 것 같은데요
    

getChannelList(chTypeList) {    const params = {      userId: userUtils.getLoginUserId(),      chTypeList,    };   return rest.get(`PTask/ChannelList`, params); }, getChannelsBrief(channelType) {    const params = {      userId: userUtils.getLoginUserId(),      channelType,    };   return rest.get(`PTask/ChannelsBrief`, params); },

• params  에서 어떤 것은 channel 로 시작하고 어떤 것은 ch 로 시작하는데, 변수 이름으로 사용할 용어는 통일했으면 합니다.
  
  • ch vs channel
    
• userId 항상 고정인가요? 
  • 예를 들어 Admin 화면에서 모든 채널을 조회하고 싶은때가 있지 않을까요?
  • 로그인 되지 않는 상태에서 호출되면 어떻게 되는 것인가요?
• 채널 목록을 가져올 때 Pagination 은 없나요?
  • 일반적으로 목록 조회는 특별한 이유가 없으면 반드시 Pagination 을 넣습니다.
• 레파지토리마다 baseURL 멤버 변수를 갖게 하고 이를 URL 넘길 때 이를 이용했으면 합니다.
  • rest.get(`${baseURL}/ChannelList`.....)

 

async getOrgUserList(companyCode, departmentCode) {    return rest.post(`PTask/OrgUserList?action=Get`, {      COMPANY_CODE: companyCode,      DEPARTMENT_CODE: departmentCode,    }); },

• 이 메소드는 왜 async 가 붙나요?
• API 설계 시 query string 관련해서 대문자 정책을 쓸 것인이 camel 정책을 쓸 것인지 정해야 합니다
  • COMPANY_CODE vs companyCode
  • 요청 body 나 요청 응답 형식도 마찬가지입니다. 
  • 대부분 camel 을 사용합니다.

 

setChannelUpdateInfo(param) {    const response = rest.put(`PTask/Channel`, param);    return response; },

• response 로 받아서 따로 return 할 필요는 없을 것 같습니다. 
  
  • 디버깅 때문에 그랬을 것 같은데...
    
  • 커밋할 때는 깔끔하게 처리해주세요
• 레파지토리 메소드 네이밍 컨벤션도 있어야 할 것 같습니다.
  • 가능하는 HTTP 메소드를 그대로 따르는 것이 좋지 않을까?
    • setChannelUpdateInfo => putChannelUpdateInfo
  • get, put, post, delete

 

 

stores/RootStore.js

 

 

const hydrate = create({    storage: localStorage,   jsonify: true, });

• mobx-persist 의 개념에 대해서 알려주세요
  
  • hydrate 라는 의미도요
    

 

class RootStore {    ChannelStore = new ChannelStore();    PostStore = new PostStore();    UserStore = new UserStore();    ... }

• 객체는 특별한 이유가 없으면 소문자로 시작하는 걸로 하시죠

 

 

Promise.all([    hydrate("channelStore", this.ChannelStore),    hydrate("userStore", this.UserStore), ]).then(() => {});

• then 에 () => {}  를 사용하는 것보다 console.log 라도 찍는 것이 좋을 것 같습니다
  

 

 

stores/ChannelStore.js

 

 

@persist("object") @observable selectedChannel = {...}

• mobx 어노테이션 붙이는 컨벤션도 정했으면 합니다.
  • 개인적으로 어노테이션은 같은 라인이 아니라 윗 라인에 붙이는 것이 깔끔하다고 생각됩니다.
    

 

console.log("data.dto.channelList", data.dto.channelList);

• 디버그 로그는 지워주세요. 깔끔하게
  

 

data.dto.channelList?.forEach((channelItem) => {    this.channels[channelItem.channelType] = channelItem; });

• ?. 은 간결하고 좋은 문법이긴 한테 사용에는 주의해야할 필요가 있어보입니다.
  
  • 플로우가 눈에 잘 들어오지 않습니다.
  • 예외 처리를 고민하지 않게 만들 것 같습니다.
• this.channels 가 채널인가요? 채널 타입인가요? forEach 안의 내용이 잘 이해가  안 됩니다.

 

 

const { data, status } = yield ChannelRepository.getChannelsBrief(

• status 핸들링 하지 않을거면 빼주세요
  
• 만약 getChannelBrief 에서 에러가 나면 어떻게 되나요?
  • 에러 핸들링 함수를 액션의 아귀먼트로 받고 이를 호출한다?
    
  • 에러를 핸들링할 수 있는 state 를 두어서 그 값을 설정한다?
    • 이 상태를 구독하는 컴포넌트는 최상위 컴포넌트여야 되겠네...
      

 

if (data.dto.RESULT_CD === "RST0001") {   ...   return true; } else {   alert("즐겨찾기 실패");   return false; }

• RESULT_CD? 먼가요? 정상 응답인데 RESULT_CD 에 따라 다른 처리가 필요하다?
  
  • API 에러 응답에 대한 수정 필요
    
• alert 가 아닌 에러를 처리할 수 있는 통합적인 플로우/UI 가 필요해보입니다.

 

addChannelMember(params) {

• Action 함수 아귀먼트 이름으로 params, props 같은 이름은 안썼으면 합니다.
  • params => member 

 

 

if (this.popUserList[userType] == null) {     this.popUserList[userType] = []; }

• js 는 특별한 경우가 아니면 A == null 비교를 하지 않습니다.
  
  • if(this.popUserList[userType]) {
    
• == 대신에 === 비교를 보통합니다.(타입과 값 모두 비교)
  

 

 

stores/PostStore.js

 

 

@observable tasksBrief = new Map();

• 어떨 때 Object 가 아닌 Map을 써야하나요?
  • Map 에 대한 가이드 작성 필요
    

 

 

@observable selectedChannel = {   channelId: null,   channelType: "join",   channelName: "참여중인 채널", };

• selectedChannel 은 UI 와 연관이 있는 것 같은데... 꼭 필요했던 state 인가요?
  
  • 단순 ui 관련이면 localState 로 처리해도 될 것 같다는 생각이 들어서요
    
• UI 상태도 저장을 해야한다면, 도메인 스토어와 UI 스토어를 분리하는 정책이 필요한  것 아닌가 생각됩니다.
  

 

@observable searchCondition = {   postRead: "total",   postSearchStartDate: moment().subtract(1, "month").format("YYYYMMDD"),   postSearchEndDate: moment().format("YYYYMMDD"),   postsNum: 20,   rowNum: 1,   sortType: "recent",   searchWord: null, };

• posts 나 postSearch , search 같은 prefix 는 굳이 붙일 필요는 없어보입니다. 
  • startDate, endDate
    
• postRead 는 어떤 조건인지 잘 와닫지 않네요.
  
• postsNum, rowNum 아 아니며 Pagination 으로 사용된 것 같은데요.
  • pageSize, pageNum 정도로 정하고 모두 다른 곳에서도 같은 형식을 썼으면 좋겠습니다
• 정렬도 이렇게 많이 쓰입니다. 확장성도 고려할 필요가 있습니다. 멀티 키라든지...
  • sortOrder
    
  • sortKey 

 

if (this.searchCondition.rowNum == 1) {    this.PostList = data.dto.postList.map((post) => new PostModel(post));

• 모델 클래스를 따로 두어야 하는 이유는 무엇인가요?
  • Repository 응답을 이용해서 Observable 한 모델을 만들겠다 인가요?
    
  • extendObservable 가이드 필요.
    • extendObservable vs set
  • Java 에서 DB 를 조회하면 이미 Model 처리가 된 상태로 넘어오는데, 이런 방식을 사용하면 어떤가요?
    • Repository 단에서 모델 매핑이 이루어지게

 

 

for (let i = 0; i < extraPostList.length; i++) {    this.PostList.push(extraPostList[i]); }

• 자바스크립트에서는 일반적으로 for 를 잘 쓰지 않음
  
  • concat(extraPostList)
  • [...extraPostList]
• 배열이나 이런 것 처리한 때 언더스코어로 많이 썼던 것 같은데

models/PostModel.js

 

•