8기 4조 코드리뷰 #86
Open
8기 4조 코드리뷰 #86
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for taaing ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
shoveller
commented
Feb 1, 2024
Comment on lines
16
to
24
| async function findId(e) { | ||
| e.preventDefault(); | ||
|
|
||
| const emailValue = inputEmail.value; | ||
| const formData = new FormData(e.currentTarget); | ||
| const { email } = Object.fromEntries(formData.entries()); | ||
|
|
||
| try { | ||
| const res = await getData('users', { | ||
| filter: `email='${emailValue}'`, | ||
| filter: `email='${email}'`, | ||
| }); |
Author
There was a problem hiding this comment.
거의 다 왔습니다.
form 의 submit 이벤트 사용법을 이해했다면, form 에서 값을 꺼내오는 방법을 배울 차례입니다.
shoveller
commented
Feb 1, 2024
Comment on lines
+37
to
55
| const isValidEmail = (emailValue) => { | ||
| const regexEmail = /^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/; | ||
| const emailValue = inputEmail.value; | ||
|
|
||
| if (regexEmail.test(emailValue) || emailValue === '') { | ||
| return regexEmail.test(emailValue) || emailValue === ''; | ||
| }; | ||
|
|
||
| /** | ||
| * TODO: 하나의 함수가 세가지 일을 하고 있군요. | ||
| * 1. 이메일 벨리데이션 | ||
| * 2. 벨리데이션이 실패했을 때의 화면 처리 | ||
| * 3. 벨리데이션이 성공했을 때의 화면 처리 | ||
| * 적절하게 추상화해서 함수를 분리하는 것이 좋겠습니다. | ||
| * 2와 3항목도 분리해 보시면 어떨까요? | ||
| */ | ||
| function validateEmail() { | ||
| if (isValidEmail(inputEmail.value)) { | ||
| emailErrorNotice.classList.remove('block'); | ||
| emailErrorNotice.classList.add('hidden'); | ||
| submitButton.classList.remove('bg-button-submit-default'); |
Author
There was a problem hiding this comment.
함수 하나가 세가지 일을 하고 있군요.
함수의 이름을 동사로 바꾸고 벨리데이션 함수만 분리해 보았습니다.
이제 if 블록에 있는 내용물과 else 블록에 있는 내용을 다른 함수로 분리해 보세요.
스스로의 행동을 설명하는 코드가 될 것이라 생각합니다.
shoveller
commented
Feb 1, 2024
Comment on lines
27
to
+30
| async function findId(e) { | ||
| e.preventDefault(); | ||
|
|
||
| const userIdValue = inputUserId.value; | ||
| const formData = new FormData(e.currentTarget); | ||
| const { userId } = Object.fromEntries(formData.entries()); |
Author
There was a problem hiding this comment.
폼의 이벤트에서 값을 읽어보면 이벤트 핸들러가 순수함수가 된다는 장점이 있지요.
shoveller
commented
Feb 1, 2024
|
|
||
| // 데이터를 가져오는 함수 | ||
| async function getData(collectionName, options = {}) { | ||
| export async function getData(collectionName, options = {}) { |
Author
There was a problem hiding this comment.
개인적으로는 눈알이 움직이는 에너지도 아끼기 위해 함수 선언에 export 를 붙입니다.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
훌륭한 코드입니다!
각각의 코드가 해야하는 일을 정확히 알고 있고 군더더기가 없습니다.
폼을 사용할 때, 폼이 내장하고 있는 자체 기능을 좀 더 사용하면 좋을 것 같습니다.
그리고 else 와 삼항연산자 사용만 조금 줄이면 좋겠습니다.