-
Notifications
You must be signed in to change notification settings - Fork 39
[김이서] sprint4 #144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[김이서] sprint4 #144
The head ref may contain hidden characters: "Basic-\uAE40\uC774\uC11C-sprint4"
Conversation
addiescode-sj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!
함수 네이밍도 잘쓰셨고 가독성도 괜찮았는데, 몇가지 유지보수에 유리한 개선 방안을 코멘트로 남겨드렸습니다 :)
주요 리뷰 포인트
- 유지보수를 고려한 개발
| const loginButton = document.getElementById("login-btn"); | ||
|
|
||
| // 이메일 유효성 검사 | ||
| function validateEmail() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
자세히보면, validateEmail() 과 validatePassword() 함수 구현 body가 굉장히 비슷합니다. 즉 재사용이 가능한 로직이 있고 코드 중복을 제거할만한 개선 여지가 있다는 뜻입니다 :)
이런식으로 재사용 가능한 비즈니스 로직을 분리해 validators 객체를 만들어서 관리해보면 어떨까요?
export const validators = {
email: (value) => {
if (!value) return { isValid: false, message: "이메일을 입력해주세요" };
if (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(value)) {
return { isValid: false, message: "올바른 이메일 형식이 아닙니다" };
}
return { isValid: true, message: "" };
},
password: (value) => {
if (!value) return { isValid: false, message: "비밀번호를 입력해주세요" };
if (value.length < 8) {
return { isValid: false, message: "비밀번호는 8자 이상이어야 합니다" };
}
return { isValid: true, message: "" };
},
};There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그리고 UI에 관련된 코드들은 변경의 여지도 많고 재사용하기 어려우니 지금과 같이 페이지별 js파일에서 코드를 사용해주되, 하나의 함수가 너무 많은 역할을 담당하지않게끔 아래 예시처럼 작업 단위를 분리해주면 좋겠죠?
- 유효성 검사 규칙에 따른 UI 업데이트
if (result.isValid) {
clearError(inputEl);
if (field === 'nickname') {
inputEl.classList.add('valid');
}
} else {
showError(inputEl, result.message);
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 구조를 바꾸면 몇가지 확실한 장점이 생깁니다 :)
- 각 함수가 { isValid, message } 형태로 일관된 형태로 결과를 반환해 코드를 좀 더 예측이 쉽고 안정적인 흐름을 가질수있도록 만들수있습니다.
- 객체를 통한 명확한 매핑을 사용해 조건문을 사용하는것보다 훨씬 코드의 의도를 명확히 드러낼 수 있습니다.
- 각 함수가 독립적이어서 개별적인 동작을 테스트하기쉽고, 수정 및 확장에 용이합니다.
- 재사용 가능한 로직을 쉽게 분리할 수 있습니다.
| const signupButton = document.getElementById("signupButton"); | ||
|
|
||
| // 이메일 유효성 검사 | ||
| function validateEmail() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
signup.js에서도 코드를 복사+붙여넣기 하듯이 똑같이 작성할 필요없이 공통 비즈니스 로직(ex. validators)을 모듈화해서 재사용해주면 유지보수에 더 좋겠죠?
| const emailIsValid = emailInput.value && !emailError.textContent; | ||
| const passwordIsValid = | ||
| passwordInput.value && passwordError.textContent === ""; | ||
| const confirmPasswordIsValid = | ||
| confirmPasswordInput.value && confirmPasswordError.textContent === ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 조건식도 toggleSignupButton() 함수 내부에서만 쓰인다기보다는 여러군데에서 사용될수있으니 재사용을 고려해보는게 좋겠죠? 혹은 제가 코멘트 드린 validators를 사용하게된다면 각 요소별 유효성 검사 결과에 따른 데이터를 올바로 저장하게되니까, 지금과 같이 따로 상수로 만들어줄 필요가 없어지겠네요:)
질문에 대한 답변
모듈화 방법보다는 기준이 궁금하신거겠죠? :) |
요구사항
기본
심화
주요 변경사항
스크린샷
배포
guileless-zuccutto-c0d1ab.netlify.app
멘토에게
모듈화를 어떻게 하는 건지 모르겠습니다!
로그인페이지와 회원가입페이지에서 중복되지 않는
"비밀번호 확인" 기능과, "회원가입 버튼 활성화/비활성화"
이런 것들만 signUp.js 에 따로 작성하고
중복건만 import 해서 사용하면 되는 것인가요?
그렇다면 import시 const 변수선언도 다시 다 붙여넣어줘야 하는지,
signUp.js 에서 사용되는 변수만 추가로 기재하면 되는지도
알려주세요.
다른 분들 것을 봐도 잘 모르겠어서 제 코드의 경우 모듈화를
어떻게 하면 효율적인지도 짚어주시면 감사하겠습니다!
셀프 코드 리뷰를 통해 질문 이어가겠습니다.