[클린코드 6기 송용주] 자동차 경주 미션 STEP 1 #222
Conversation
car 내부에서 직접 비교하는 로직을 이동 조건 함수를 호출하도록 변경
객체간 비교시 private field에 접근하지 못하기 때문에 동등성을 검증하지 못한다 getter를 활용하여 객체 field의 값을 비교하도록 변경
최소 길이로 생성 불가 오류 수정
position 객체가 위치 값을 직접 비교하도록 변경
쉼표(,)를 기준으로 분리하며 Name 객체 생성 시 예외에 의해 애플리케이션이 종료되도록 한다
There was a problem hiding this comment.
안녕하세요
용주님 반갑습니다!
첫 리뷰이니 차근차근 사소한 코멘트로 리뷰를 시작했습니다.
간단한 의견 나누기
자바스크립트가 미숙하다보니 ES5+ 문법은 제대로 활용하지 못한 것 같습니다 😅
모듈 시스템에 대한 경험이 없다보니 익숙한 자바의 클래스 방식으로 접근을 해서 파일을 분리했습니다.
그렇다보니 자바에서 언어만 바뀐 느낌을 지울 수가 없습니다.
분명 JS에 익숙하지 못한 모습이 코드에서 보이긴합니다.
프론트엔드나 JS를 주로 다루는 개발자를 목표로하는게 아니라면 꼭 JS와 억지로 친해지거나 익숙해질 필요는 없습니다.
핵심적인 요구사항을 구현해내는 문제 해결 능력보다 중요한 것은 없으니까요!
나머지는 시간이 흐르며 저절로 따라올 수 있다고 생각해요
모든 클래스를 제거하고 함수로만 작성을 해볼까 생각이 들었는데요 리뷰어님의 의견은 어떠신지 궁금합니다!
이 또한 애매한데요.
현재 구성하신 볼륨만 봐서는 클래스가 굳이 필요하지는 않기때문에 함수로 구현하셔도 충분합니다.
하지만 클래스로 구현하면 더 배울 수 있는게 많기때문에 배움의 목적이라면 클래스로 하셔도 좋습니다.
클래스와 인터페이스로 설계하고 다양한 접근제어자와 함께 인스턴스를 구현하면 많은 점을 배울 수 있어요.
피드백
클래스 설계
Car, Position, Condition 이 3개의 클래스가 핵심 로직으로 보여요!
하지만 느슨하면서도 강결합되어있는 이 관계가.. 참 애매하게 느껴집니다.
어떤 의도를 드러내고 싶으셨을지 아쉬웠습니다.
도메인 로직인가?
domain이라는 것은 참 어려운데요.
도메인과 관련없는 로직이 domain 내부에 혼재되었으니 이를 분리해보세요!
사실 작은 앱이라 상관은 없겠지만 많은 입문 시절에 꼭 해야하는 훈련이랍니다
커밋 단위
의미없는 커밋이 많아보여요.
괴롭겠지만 커밋의 단위와 메세지를 신경써보시면서 좋은 습관을 만들어보세요!
|
|
||
| export function readLineAsync(query) { | ||
| return new Promise((resolve, reject) => { | ||
| if (arguments.length !== 1) { |
There was a problem hiding this comment.
더 넓은 에러 범위를 생각해보시는게 어떨까요?
숫자를 검사할때는 타입과 범위를 고민해보셔야합니다 1 이상 이하 모두 생각해볼 필요가 있는 것이죠
| @@ -0,0 +1,30 @@ | |||
| import { Car } from '../../src/domain/car.js'; | |||
|
|
|||
| describe('객체 생성', () => { | |||
There was a problem hiding this comment.
객체 생성을 테스트하는 것은 너무 좋습니다!
다만 사용자 입장에서의 시나리오를 고민해보셨으면 좋겠습니다 :)
|
|
||
| describe('객체 생성', () => { | ||
| it('생성', () => { | ||
| const car = new Car('name'); |
There was a problem hiding this comment.
테스트 코드이지만 name이라는 필드에 정말 존재할 것 같은 데이터를 넣어주는게 좋지 않을까요?!
| expect(car.name).toEqual('name'); | ||
| expect(car.position).toEqual(0); |
There was a problem hiding this comment.
toEqual()말고도 괜찮은 API가 많으니 한번 확인해보세요!
https://jestjs.io/docs/expect
| import { Name } from './name.js'; | ||
| import { Position } from './position.js'; | ||
|
|
||
| export class Car { |
There was a problem hiding this comment.
단일 파일에서 단 하나의 모듈만 내보내고 있으면 export default 또한 고려해볼법할 것 같은데 이유가 있으셨을까요?
| const RANDOM_BOUND = 10; | ||
|
|
||
| const randomNumber = () => Math.floor(Math.random() * RANDOM_BOUND); |
There was a problem hiding this comment.
랜덤의 바운더리가 최대 10까지라면 과연 이 함수는 randomNumber()로 정의하는게 맞을까요?
| const randomNumber = () => Math.floor(Math.random() * RANDOM_BOUND); | ||
|
|
||
| export const movable = () => { | ||
| const number = randomNumber(); |
There was a problem hiding this comment.
movable() 에서 randomNumber()의 의존성을 가지고 있는게 좋을까요?
| this.#position = new Position(position); | ||
| } | ||
|
|
||
| move(movable) { |
There was a problem hiding this comment.
Car.move(콜백 함수) 이동시켜주는 함수에서 콜백으로 함수를 받는다는 것이 일반적이지는 않아서? 그럴까요
사용하는 측에서는 인지부조화가 오는 것 같은데 어떤 이유로 이런 설계를 하셨는지 궁금합니다~
| #name; | ||
| #position; | ||
|
|
||
| constructor(name, position = 0) { |
There was a problem hiding this comment.
position = 0 기본 매개변수 처리 좋습니다 👍
|
|
||
| const actual = getWinners(cars); | ||
|
|
||
| expect(actual).toEqual(['k3', 'k8']); |
There was a problem hiding this comment.
세 대의 자동차라고 정의 되어있는 것 같은데 맞을까요..?
안녕하세요 리뷰어님 😃
자바스크립트가 미숙하다보니 ES5+ 문법은 제대로 활용하지 못한 것 같습니다 😅
모듈 시스템에 대한 경험이 없다보니 익숙한 자바의 클래스 방식으로 접근을 해서 파일을 분리했습니다.
그렇다보니 자바에서 언어만 바뀐 느낌을 지울 수가 없습니다.
모든 클래스를 제거하고 함수로만 작성을 해볼까 생각이 들었는데요 리뷰어님의 의견은 어떠신지 궁금합니다!
사소한 컨벤션에 대한 코멘트도 좋으니 많은 피드백 부탁드립니다. 😃