-
Notifications
You must be signed in to change notification settings - Fork 42
[정새론] sprint4 #130
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
base: Basic-정새론
Are you sure you want to change the base?
The head ref may contain hidden characters: "basic-\uC815\uC0C8\uB860-sprint4"
[정새론] sprint4 #130
Conversation
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.
수고하셨습니다!
js파일에 드린 코멘트 한번 참고해보시고, 리팩토링 진행해보시면 좋을것같아요 :)
주요 리뷰 포인트
- 유지보수를 고려한 개발
- 코드 간소화 및 중복 제거
</div> | ||
</body> | ||
<body> | ||
<script src="./script/login.js" type="module"></script> |
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.
script 태그 위치도 브라우저 동작 원리를 이해하고계시다면 달라질수있답니다 :)
아래 아티클 참고로 읽어보세요!
} | ||
} | ||
|
||
function passwordShowHide(button, input) { |
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.
NIT: 함수이름은 명사형보다는 동사로 시작하는게 좀 더 일반적입니다.
passwordShowHide => showHidePassword
네이밍 컨벤션 참고해볼까요?
참고
if (checkValid) { | ||
button.removeAttribute("disabled"); | ||
button.classList.add("login-button-enable"); | ||
} else { | ||
button.setAttribute("disabled", true); | ||
button.classList.remove("login-button-enable"); | ||
} |
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.
checkValid를 이런식으로 변수화하면 더 의미가 명확해질것같아요.
const isValid = validatorResults.every(Boolean);
그리고, classList.toggle을 사용하면 조건문을 사용하지않게되어 코드를 간소화할수있겠네요!
button.disabled = !checkValid;
button.classList.toggle('login-button-enable', isValid);
if (input.getAttribute("type") === "password") | ||
input.setAttribute("type", "text"); | ||
else input.setAttribute("type", "password"); |
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 isPasswordVisible = input.type === INPUT_TYPES.PASSWORD;
input.type = isPasswordVisible ? 'text' : 'password';
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.
현재 불필요한 DOM 접근이 많고, 유효성 검사 로직이 if 체이닝을 통해 복잡하게 중첩되어있어 가독성이 떨어지고, 이벤트 핸들러 내부에서 너무 많은 로직을 처리하고있어 유지보수에도 좋지 않을것같아요.
이 문제들을 해결하기위해 몇가지 리팩토링 제안을 드려볼게요!
우선 DOM 접근은 상단에서 한번만 하도록할까요?
const elements = {
loginButton: document.getElementById("login-button"),
emailInput: document.getElementById("email-input"),
passwordInput: document.getElementById("password-input"),
warningMessages: document.getElementsByClassName("warning-message"),
};
또, 상태 관리의 경우에도 객체로 한번에 관리하면 구조가 더 명확해질것같아요.
const state = {
email: {
isValid: false,
isEmpty: true,
isInvalid: false,
},
password: {
isValid: false,
isEmpty: true,
isInvalid: 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 업데이트
- 이벤트 핸들러 연결
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와 결합될 필요가 있을까요?
유효성 검사 규칙과 수행은 프로그램내에서 어디서든 사용될수있고, UI 상태에 따라 변화하지도않아요.
따라서, 재사용을 고려해 아래와 같이 모듈화해보시는게 좋을것같아요.
// validation.js - 검증 로직
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: '' };
},
nickname: (value) => {
if (!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: '' };
},
passwordCheck: (value, formValues) => {
if (!value) return { isValid: true, message: '' }; // 빈 값은 허용
if (value !== formValues.password) {
return { isValid: false, message: '비밀번호가 일치하지 않습니다' };
}
return { isValid: true, message: '' };
}
};
그러면 이벤트 핸들러 내부에서 유효성 검사를 수행하지않고 이런식으로 우리가 만든 validators를 재사용해줄 수 있어요.
const { isValid, message } = validators.password(e.target.value);
state.password.isValid = isValid;
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 updateUI = (inputElement, messageElement, isValid, message) => {
inputElement.classList.toggle("warning-input-border", !isValid);
messageElement.textContent = message;
messageElement.classList.toggle("warning-message-active", !isValid);
};
질문에 대한 답변
문제를 파악하는것부터가 문제 해결의 시작입니다. |
요구사항
기본
로그인
회원가입
심화
스크린샷
멘토에게