-
Notifications
You must be signed in to change notification settings - Fork 31
전수영 sprint4 #177
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 #177
The head ref may contain hidden characters: "Basic-\uC804\uC218\uC601-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.
수영님 자바스크립트 기본 개념은 충분히 알고 계신 거 같습니다 :) 추상화, 함수명, 변수명에 조금만 더 신경써 주시면 좋을 거 같습니다!
최근 열심히 달리셨는데, 고급 프로젝트도 화이팅입니다~! :)
질문 주신 부부은 리뷰 참고해 주세요!
| </div> | ||
| </main> | ||
|
|
||
| <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.
스크립트 파일을 따로 만드시면 좀 더 깔끔하게 정리할 수 있습니다 :)
그리고 로그인, 회원가입 페이지에 겹치는 부분이 많은데, module 로 만드시면 공통된 부분을 import해서 쓸 수 있죠!
https://developer.mozilla.org/ko/docs/Web/JavaScript/Guide/Modules#applying_the_module_to_your_html
| const inputEmail = document.querySelector('#input_email'); | ||
| const inputPw = document.querySelector('#input_pw'); | ||
| const inputPwCheck = document.querySelector('#input_pw_check'); | ||
| const email_regex = /^[A-Za-z0-9_.\-]+@[A-Za-z0-9\-]+\.[A-Za-z0-9\-]+$/; |
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.
정규식을 써주셨군요! 👍
| toggleJoinButton(); | ||
| }); | ||
| }); | ||
| function inputValidate(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.
기능이 여러개 있는데, 전체적으로 어떤 구조로 짜야하는지 어려웠습니다.
인풋 여러개를 너무 여러가지 조건을 나눠서 구분하고 있는 것 같기도 하고,
깔끔하지 않다는 느낌이 드는데, 왜 그런지 알 수 있으면 좋을 것 같습니다
-> 지금은 하나의 함수가 너무 많은 일을 하고 있어요!
우선 전반적으로 로직을 작게 자르고 함수로 정의해 보시면 좋을 거 같습니다.
예를 들면
function validateEmail(value) {
if (!value) {
return "이메일을 입력해주세요.";
} else if (email_regex.test(value) == false) {
return "잘못된 이메일 형식입니다.";
}
return "";
}이렇게 인풋 단위로만 쪼개도 코드 파악 하기가 훨씬 쉬워집니다.
function inputValidate(input) {
let msg = "";
let inputId = input.getAttribute("id");
if (inputId == "input_email") {
msg = validateEmail(input.value);
} else if (inputId == "input_nickname") {
msg = validateNickname(input.value);
}
// ...
setInputBlurMessage(input, msg);
}더 나아가 여기서 함수의 역할을 더 명확하게 하기 위해 유효성 검사를 제외한 부분은 제거할 수도 있겠죠!
function inputValidate(input) {
let msg = "";
let inputId = input.getAttribute("id");
if (inputId == "input_email") {
msg = validateEmail(input.value);
} else if (inputId == "input_nickname") {
msg = validateNickname(input.value);
}
// ...
return msg;
}
function handleBlurInput(e) {
const input = e.target;
const errorMessage = inputValidate(input);
if (errorMessage) {
setInputBlurMessage(input, errorMessage);
}
}
inputs.forEach(function (input) {
input.addEventListener("focusout", handleBlurInput);
});이렇게 함수의 역할을 줄이고 (가능하면 하나의 책임만) 이름만 잘 지어줘도 코드 읽기가 훨씬 쉽습니다!
현재 inputValidate는 if문을 해석하며 파악해야 하지만 위처럼 함수로 묶어주면 이메일을 검증하고 있구나~ 닉네임을 검증하는구나~ 하고 바로 알 수 있죠!
결국 우리가 코드를 잘 짜도록 노력하는 이유는 유지 보수하기 쉽도록 하는 것이죠. 원하는 코드를 쉽게 찾고, 쉽게 수정 가능해야 합니다. 그런데 하나의 함수에 너무 많은 일을 하면 코드를 파악하기 힘들고, 로직간에 서로 영향을 받을 가능성이 커집니다!
|
|
||
| setInputBlurMessage(input, msg); | ||
| } | ||
| function setInputBlurMessage(input, msg) { |
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를 다루는 것이라면 handeErrorUI 같은 이름이 좀 더 직관적일 거 같습니다!
| } | ||
| } | ||
|
|
||
| function toggleJoinButton() { |
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.
여기도 마찬가지입니다! 어떤 것을 토글하는 것인가요?
버튼의 disabled 상태를 관리하는 것이라면
updateSubmitButtonState 정도가 있겠네요!
| } | ||
| const toggleButtons = document.querySelectorAll('.btn_pw'); | ||
|
|
||
| toggleButtons.forEach(function (button) { |
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.
이 부분도 함수로 묶어 주기만 해도 훨씬 파악하기 쉽죠!
toggleButtons.forEach(function (button) {
button.addEventListener('click', togglePasswordVisibility)
}
}
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게
기능이 여러개 있는데, 전체적으로 어떤 구조로 짜야하는지 어려웠습니다.
인풋 여러개를 너무 여러가지 조건을 나눠서 구분하고 있는 것 같기도 하고,
깔끔하지 않다는 느낌이 드는데, 왜 그런지 알 수 있으면 좋을 것 같습니다
셀프 코드 리뷰를 통해 질문 이어가겠습니다.