-
Notifications
You must be signed in to change notification settings - Fork 39
[정수민] sprint4 #148
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 #148
The head ref may contain hidden characters: "Basic-\uC815\uC218\uBBFC-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 emailValue = emailInput.value.trim(); | ||
| if (emailValue === '') { |
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 업데이트 로직이 복잡하고 많아서 그런 특징이 생기는것같아요.
UI 업데이트를 위한 값을 모아서 관리할수있는 객체를 만들어볼까요?
// 상태 관리 객체
const loginState = {
email: {
value: '',
isValid: false,
errors: {
empty: false,
invalid: false
}
},
password: {
value: '',
isValid: false,
errors: {
empty: false,
invalid: false
}
},
isPasswordVisible: false,
isFormValid: false
};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를 업데이트하는 함수를 개별적으로 만들어볼까요?
// 유효성 검사를 처리하는 함수
function validateForm() {
// 이메일 검증
const emailValue = emailInput.value.trim();
loginState.email.errors.empty = emailValue === '';
loginState.email.errors.invalid = !loginState.email.errors.empty && !isValidEmail(emailValue);
loginState.email.isValid = !loginState.email.errors.empty && !loginState.email.errors.invalid;
// 비밀번호 검증
const passwordValue = passwordInput.value;
loginState.password.errors.empty = passwordValue === '';
loginState.password.errors.invalid = !loginState.password.errors.empty && passwordValue.length < 8;
loginState.password.isValid = !loginState.password.errors.empty && !loginState.password.errors.invalid;
// 전체 폼 유효성
loginState.isFormValid = loginState.email.isValid && loginState.password.isValid;
// UI 업데이트
updateUI();
return loginState.isFormValid;
}
// 상태 업데이트 함수
function updateState(field, value) {
loginState[field].value = value;
validateForm();
}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.
이렇게 각 함수가 하나의 일만 책임지는 형태로 (단일 책임 원칙) 수정되니까,
이런 장점들이 생길거예요.
- html 마크업 변경에 따른 함수 내부 로직 변경을 최소화 할 수 있습니다.
- 코드의 실행 흐름이 더 예측 가능한 형태로 바뀝니다 (유지보수 및 협업에 용이).
- 구조에 의해 더 명확하게 의도를 드러낼 수 있습니다 (유지보수 및 협업에 용이).
| // 에러 표시 함수 | ||
| function showError(input, errorElement) { | ||
| input.style.border = '1px solid #f74747'; | ||
| errorElement.style.display = 'block'; | ||
| } | ||
|
|
||
| // 에러 숨기기 함수 | ||
| function hideError(errorElement) { | ||
| errorElement.style.display = 'none'; | ||
| } | ||
|
|
||
| // 테두리 초기화 | ||
| function clearBorder(input) { | ||
| input.style.border = 'none'; | ||
| } |
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.
이 함수들도 지금처럼 너무 세부적인 단위로 작업을 쪼개기보다는,
toggleFormField와 같이 네이밍 바꾸고, 하나의 함수에서 UI 상태에 따른 UI 업데이트가 일어날수있게끔 인자와 함수 내부 구현 내용도 조금 맞춰서 바꿔보면 좋겠죠?
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.
해당 파일에서도 비슷한 코드가 반복되고있는데, 재사용이 가능한 로직들은 모듈화하고 import받아서 사용해주면 코드가 훨씬 정리될수있겠죠? 이번 미션 구현사항은 아니지만 미리 참고해보시면 좋을것같아요 :)
질문에 대한 답변
html css js만 사용하는 상황을 가정하다면 다만 외부 웹사이트가 아니라 내부적으로 페이지를 바꿔주고싶은거면 기존에 쓰시던 방법을 사용하는게 가장 직관적이고, 커스텀 라우팅 모듈을 구현해서 window의 popState와 history 객체의 pushState를 사용해 내부 라우팅을 구축하셔도됩니다. 자바스크립트 사용 관련해서는 PR 본문 내에 자세히 피드백 드렸습니다~ |
요구사항
기본
심화
주요 변경사항
스크린샷
-[x] 로그인




-[x] 회원가입


멘토에게