Skip to content
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

[2022.04.26] level2 - 예상대진표 풀이 추가 #44

Merged
merged 1 commit into from
Apr 27, 2022
Merged

Conversation

le2sky
Copy link
Contributor

@le2sky le2sky commented Apr 26, 2022

대진표가 부전승이 없는 특성을 이용해서 풀이했습니다. 리뷰 부탁드립니다

@prove-ability
Copy link
Contributor

prove-ability commented Apr 26, 2022

제 PR 인줄 알고 close 누르고 reopen 했습니다..!!
그리고 Assignees, Labels 수정했습니다!

Copy link
Owner

@codeisneverodd codeisneverodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은 코드 잘 보았습니다 😁
함수를 적절히 분리해주신 점이 너무 좋았습니다!!
꼭 개선해주셨으면 하는 부분은 없어 확인후 Merge 진행합니다!

Comment on lines +34 to +38
if (isLeft()) {
arr.splice(arr.length / 2)
} else {
arr.splice(0, arr.length / 2)
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌱 반영해도 좋고 넘어가도 좋아요. 🌱

위의 다른 코드들 처럼 ternary operator를 사용해보시는 것도 좋을 것 같습니다!

Suggested change
if (isLeft()) {
arr.splice(arr.length / 2)
} else {
arr.splice(0, arr.length / 2)
}
isLeft() ? arr.splice(arr.length / 2) : arr.splice(0, arr.length / 2)

Comment on lines +24 to +26
return (
(arr.indexOf("A") + 1 > arr.length / 2 && arr.indexOf("B") + 1 <= arr.length / 2) ||
(arr.indexOf("A") + 1 <= arr.length / 2 && arr.indexOf("B") + 1 > arr.length / 2)) ? true : false
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌱 반영해도 좋고 넘어가도 좋아요. 🌱

불필요한 tenary operator가 사용되어 가독성이 조금 떨어진다고 생각합니다! boolean 값을 직접 리턴 하시는 것보단 비교문 그 자체를 쓰시는 것이 좋을 것이라 생각됩니다!

Suggested change
return (
(arr.indexOf("A") + 1 > arr.length / 2 && arr.indexOf("B") + 1 <= arr.length / 2) ||
(arr.indexOf("A") + 1 <= arr.length / 2 && arr.indexOf("B") + 1 > arr.length / 2)) ? true : false
return (
(arr.indexOf("A") + 1 > arr.length / 2 && arr.indexOf("B") + 1 <= arr.length / 2) ||
(arr.indexOf("A") + 1 <= arr.length / 2 && arr.indexOf("B") + 1 > arr.length / 2))

(arr.indexOf("A") + 1 <= arr.length / 2 && arr.indexOf("B") + 1 > arr.length / 2)) ? true : false
};
const isLeft = () => {
return (arr.indexOf("A") + 1 > arr.length / 2) ? false : true
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌱 반영해도 좋고 넘어가도 좋아요. 🌱

이전 코멘트와 동일하게 불필요한 boolean 값들을 제거하시면 더 좋을 것이라 생각합니다!

Suggested change
return (arr.indexOf("A") + 1 > arr.length / 2) ? false : true
return arr.indexOf("A") + 1 <= arr.length / 2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 생각은 전혀 못했었네요 항상 좋은 리뷰 감사드립니다! ㅎㅎ

@codeisneverodd codeisneverodd merged commit 3fdce79 into codeisneverodd:main Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants