Skip to content
This repository has been archived by the owner on Sep 19, 2021. It is now read-only.

[3주차] 유현우 미션 제출합니다. #5

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

puba5
Copy link
Member

@puba5 puba5 commented Apr 17, 2020

https://react-vote-11th.puba5.now.sh/

이번 미션은 사실 스터디 때 모르는 것들을 다 여쭤보고, 구현을 어느정도 했었기 때문에 그렇게 어렵지는 않았던 것 같습니다! 너무 설명을 잘해주셔가지구 ㅎㅎ 근데 중간 고사 기간이 점점 다가오니 다른 디테일에 신경을 썼어야하는데 신경을 많이 못 쓴 것 같아 아쉽습니다 ㅠㅠ

비동기함수나 Request, Response, axios, POST, GET 같은 개념이나 함수들을 사용하면서 이런 개념들을 더 잘 이해하게 된 것 같습니다. 개념 자체는 알고 있고 한두번씩 써보긴 했었지만 개념이 조금 모호했었는데 이번에 사용하면서 완벽하진 않지만 좀 더 잘 이해하게 된 것 같습니다.

컴포넌트나 state 같은 것들을 사용하면 사용할수록 점점 익숙해지는 것 같습니다.

그런데 아직 memo는 정확히 어떨 때 사용해야하는지 애매하네요..로그인 창에서 쓰는 건 알겠지만 이번에 후보자들 보여주는 창에서는 어디다 넣어야할지 고민하다가 그냥 상위 컴포넌트에 넣었는데 잘 넣었는지 모르겠네요...ㅠㅠ

그리고 예전에 혼자서 과제나 코딩을 할 때는 컨벤션을 엄격하게 지키지 않았는데 계속 보면서 익숙해져야겠습니다...그리고 css 틈 날때마다 공부는 하는데 아직은 생각한대로 척척 짜지지는 않네요..ㅠㅠ

혼자 개발하면 기능 구현은 해도 막개발이라는 생각이 지워지질 않았는데 이렇게 코드 리뷰해주시니까 그런 불안이 많이 없어져서 좋은 것 같습니다. 꼼꼼하게 코드 리뷰해주시느라 항상 감사합니다 ㅎㅎ

@puba5
Copy link
Member Author

puba5 commented Apr 17, 2020

주소입니다
https://react-vote-11th.puba5.now.sh/

Copy link

@ybh1760 ybh1760 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이번 과제 리뷰를 담당한 양병훈입니다!
정말 너무 잘하셔서 제가 뭘 리뷰할게 없더라구요!. 그래도 제 생각을 정리해서 리뷰 남겨봤습니다.
제가 꼭 정답은 아니기 때문에 뭔가 이상한거 같은 것들 있으면 물어봐주세요! 수고하셨습니다!😀

pages/index.js Outdated

export default function Home() {
const [loggedIn, setLoggedIn] = useState(false);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loggedIn은 로그인 여부를 체크하는 변수 같은데 loggedIn 대신에 isLoggedIn로 선언하는게 좋은 거 같아요.

Comment on lines 10 to 12
.put(process.env.API_HOST + "/candidates/" + id + "/vote/", {
params: {},
})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 템플릿 리터럴이라는 문법이 존재합니다. 백틱(`)을 이용해서 문자열을 감싸주는 방법입니다. 이 문법의 장점은 백틱 안에 표현식을 넣을 수 있다는 점입니다. 그래서 아래와 같이 수정할 수 있을 거 같아요!. 자세한 내용은 밑에 링크를 참고해주세요.
  1. put요청 시 파라미터가 없다면, 생략해도됩니다.
Suggested change
.put(process.env.API_HOST + "/candidates/" + id + "/vote/", {
params: {},
})
.put(process.env.API_HOST + `/candidates/${id}/vote/`)

Comment on lines 21 to 23
.finally(function () {
// always executed
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finally블록은 사용되고 있지 않은 것 같아서 없애는 게 좋을 거 같아요~

Comment on lines 32 to 37
<VoteBtn
onClick={() => {
alert(name + "님에게 투표 완료!");
voteCandidate();
}}
>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alert(name + "님에게 투표 완료!"); 이 부분은 voteCandidate메소드 안에서 성공적으로 put했을시에 추가되면 좋을 것 같아요. 지금 방식은 put요청이 성공여부와 상관없이 투표완료 알림 메세지가 뜨는 구조이기 때문에 아래와 같이 하는 게 어떨까요?

axios
       .put(process.env.API_HOST + `/candidates/${id}/vote/`)
       .then(res=>alert(name + "님에게 투표 완료!");

Comment on lines 32 to 35
.post(process.env.API_HOST + "/auth/signin/", {
email: email,
password: password,
})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

post요청의 data는 userData이기 때문에 굳이 새로운 객체를 생성해서 data로 넘길 필요가 없을 것 같아요!. 아래와 같이 data로 userData를 넘겨주는 게 어떨까요?

Suggested change
.post(process.env.API_HOST + "/auth/signin/", {
email: email,
password: password,
})
.post(process.env.API_HOST + "/auth/signin/", uesrData)

Comment on lines 42 to 61
if (error.response.status === 404) {
alert("이메일이 존재하지 않습니다!!!");
setUserData({
email: "",
password: "",
});
} else if (error.response.status === 422) {
alert("비밀번호가 틀렸습니다!!!");
setUserData({
email: email,
password: "",
});
} else {
alert("다시 시도해주세요!");
setUserData({
email: "",
password: "",
});
}
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

catch블록은 에러가 발생할 시 실행되는 부분이기 때문에 여기서 폼을 초기화하는 로직을 집어넣는게 좋지 않은 것 같습니다.
아래와 같이 폼을 초기화하는 함수를 선언하여 catch 블록에서 실행하는게 좋을 거 같아요!

const initFormData = (status) => {
      const initializedUserData = {
            email: "",
            password: "",
          }

      switch(status){
                  case:404:
                          alert("이메일이 존재하지 않습니다!!!");
                          break;
                  case:422:
                          alert("비밀번호가 틀렸습니다!!!");
                          initializedUserData.email = email;
                          break;
                  default:
      }

     setUserData(initializedUserData);
}

}

export const MemoizedLoginForm = React.memo(LoginForm);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

login-form에서는 export 하는 컴포넌트가 하나 뿐이기때문에 default export를 하는 게 좋을 거 같아요. LoginForm의 default export를 React.memo적용한 후에 하는 게 어떨까요?

export default React.memo(LoginForm);

Comment on lines 41 to 57
{candidateList !== null && (
<CandidateListWrapper>
{candidateList
.sort((a, b) => {
return b.voteCount - a.voteCount;
})
.map((candidate) => (
<CandidateForm
key={candidate._id}
id={candidate._id}
rank={candidateList.indexOf(candidate) + 1}
name={candidate.name}
voteCount={candidate.voteCount}
/>
))}
</CandidateListWrapper>
)}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. candidateList !== null && 대신에 candidateList && 를 해도 똑같이 작동해요.

  2. candidateList가 없으면 어짜피 안에 아무것도 안 나오기 때문에 CandidateListWrapper를 candidateList체크하기전에 감싸주는 게 가독성에 좋은 거 같아요!

  3. map콜백의 두번째 인자로 배열의 인덱스 값을 받을 수 있습니다. candidateList.indexOf(candidate) 이렇게 index를 구하지 않아도 됩니다~

Suggested change
{candidateList !== null && (
<CandidateListWrapper>
{candidateList
.sort((a, b) => {
return b.voteCount - a.voteCount;
})
.map((candidate) => (
<CandidateForm
key={candidate._id}
id={candidate._id}
rank={candidateList.indexOf(candidate) + 1}
name={candidate.name}
voteCount={candidate.voteCount}
/>
))}
</CandidateListWrapper>
)}
<CandidateListWrapper >
{candidateList &&
candidateList
.sort((a, b) => b.voteCount - a.voteCount)
.map((candidate, index) => {
const { _id : id, name, voteCount } = candidate;
return (
<CandidateForm
key={id}
rank={index + 1}
{...{name,voteCount,id}}
/>
);
})}
</CandidateListWrapper>

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants