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

Part1 Review #153

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Part1 Review #153

wants to merge 11 commits into from

Conversation

kaori-killer
Copy link

  1. import 문을 이용해 스트립트 모듈화하신 부분 깔끔해서 가독성이 좋았습니다.

  2. randomNumber.js에서 indexOf 메소드를 이용해 중복방지 처리를 하신 부분 신기했습니다. 저보다 짧고 괜찮은 코드 같아요!

  3. Compare.js에서 ball부분 구현 잘하신 것 같아요. 저는 include메소드를 몰라서 for문을 이용했네요.

  4. 전체적으로 코드 인덴트를 잘 지켜주셨네요. 저는 이 부분이 너무 어려웠어요.. 참고 많이 됐습니다.

  5. https://github.com/airbnb/javascript에 따르면, randomNumber.js에서 객체 생성하실 때 리터럴 구문하는 게 더 좋은 코드라고 나와있습니다. 참고하세요!
    new Array() <- Bad!
    [] <- Good!

  6. makeResetButton함수도 그렇고 몇몇 부분에서 js로 html을 새로 만드셨더라고요! 하지만 Html파일에 (재시작 버튼을) 미리 생성해놓고, display:none으로 해놓았다가 필요할 때 display:block으로 수정하면 더 코드가 짧고 편합니다!

  7. PrintResult부분에서 if조건에 상관없이 document.getElementById("result").innerHTML = string;가 중복되어 작성되어있습니다. 조건밖에 쓰셔도 될 것 같아요!

  8. commit 메시지 작성 부분은 조금 아쉽네요 ㅎㅎ 구현 전에 readme에 기능을 작성하고, 그에 맞춰서 commit하셨으면 더 좋았을 것 같아요! 수고하셨습니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants