-
Notifications
You must be signed in to change notification settings - Fork 31
[이나경] sprint4 #133
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 #133
The head ref may contain hidden characters: "Basic-\uC774\uB098\uACBD-sprint4"
Conversation
dongqui
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.
나경님~!
코드 퀄리티를 위해 고민하시는 모습이 너무 좋습니다! 👍 👍
질문 주신 부분은 리뷰 참고해 주세요 :)
| export const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | ||
|
|
||
| /* 에러 메시지 */ | ||
| export function showError(input, error, 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.
기능별로 함수를 잘 나눠 주셨네요! 👍
| } | ||
|
|
||
| /* 이메일 유효성 검사 */ | ||
| export function validationEmail(input, error) { |
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.
로그인/회원가입에서 공통되는 유효성 검사를 formvalidation.js로 분리하고, 비밀번호 토글 기능도 toggle.js로 따로 나눴습니다! 그런데 validation 함수뿐 아니라 버튼 상태 업데이트 함수나 페이지 이동 함수까지 같이 넣은 게 괜찮은 설계인지 궁금합니다. 혹시 지금 구조보다 더 나은 방식이 있다면 어떻게 나눌 수 있을지도 조언해주시면 감사하겠습니다!
-> 물론 지금 보다 더 나뉠 여지는 충분이 있습니다만, 지금처럼 하나의 모듈로 묶는 것도 규모가 크지 않다면 충분히 합리적인 선택입니다. 정답이 있지는 않아요 🤣 지금 처럼 쓰다가 규모가 커지면 분리해도 괜찮습니다. 다만 여러 기능이 모인 파일인 만큼 formvalidation.js보다는 조금 더 범용적인 느낌의 이름이 좋겠죠 :)
| } | ||
|
|
||
| /* 버튼 누르면 페이지 이동 */ | ||
| export function goToPage(input, button, url) { |
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.
이 부분은 공통으로 쓰이는 로직은 아닐 거 같아요~ 페이지 이동이기보다는 로그인, 회원 가입 등의 요청을 하고 응답을 처리하므로 각각 구현하게 될 거 같습니다 :)
|
|
||
| /* 모든 입력값이 유효한지 확인 */ | ||
| export function inputsValid(input) { | ||
| for (let i = 0; i < input.length; i++) { |
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.
for of, forEach 등을 활용하시는 것이 실수를 줄이고, 가독성에 조금 더 도움이 됩니다 :)
| /* 페이지 이동 */ | ||
| goToPage(inputs, loginButton, '../html/items.html'); | ||
|
|
||
| emailInput.addEventListener('blur', 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.
현재 유효성 검사를 각각의 input마다 blur/input 이벤트에 addEventListener로 바인딩하고 있는데, 이 구조가 너무 반복적이고 지저분하다는 느낌이 들어서 더 깔끔하게 작성할 수 있는 방법이 있을지 궁금합니다!
-> 이벤트 버블링을 활용해 보실 수 있을 거 같네요! 🤔
const inputValidations = {
email: validateEmail,
password: validatePassword,
};
const $form = document.querySelector("form");
$form.addEventListener("input", (e) => {
const validation = inputValidations[e.target.id];
validation();
updateButtonState(inputs, loginButton);
});| } | ||
|
|
||
| /* 비밀번호 유효성 검사 */ | ||
| export function validationPassword(input, error) { |
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.
또 구현하면서 이메일, 닉네임, 비밀번호 등 유효성 검사 함수 내부 로직이 거의 비슷해서, 조건과 메시지만 다르게 공통 함수로 추출하는 게 더 나은 구조일까 고민이 들었습니다. 각각의 역할이 명확한 게 유지보수나 SOLID 원칙에 더 부합할지, 아니면 중복을 줄이기 위해 하나로 합치는 게 더 실무적으로 좋은 선택일지 궁금합니다!
->
음 제가 정확하게 잘 이해를 못했는데, 유효성 검사를 하나의 함수로 합친다는 말씀이 맞을까요? 만약 그렇다면 공통화는 가능하긴 하지만, 함수가 너무 커질 거 같아요 (해당 부분은 생각하시는 코드를 예시로 보여주시면 더 이야기 할 수 있을 거 같습니다)🤔
오히려 좀 더 나누는 것은 가능할 거 같아요. 지금의 경우 함수가 유효성 검사도 하고 에러 핸들링도 다루고 있습니다. 그런데 만약 이메일 찾기 같은 페이지가 추가되고, 이메일 찾기 페이지에서 이메일 입력 에러 발생시 로그인 페이지와 다른 UI를 보여줘야 한다면 이 함수는 수정이 필요하겠죠..! 🤣
하지만 늘 정답은 없습니다..! 지금 상황에서는 충분히 잘 나누셨다고 생각해요. 만약 이 프로젝트가 그냥 여기서 끝난다면? 혹은 시간이 너무 부족하다면? 어느정도 단순하고 빠르게 개발하고 나중에 필요할 때 리팩토링 하는 것도 방법이 되겠죠 :)
| @@ -0,0 +1,67 @@ | |||
| export const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | |||
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.
정규식을 쓰셨군요! 👍
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게