[FE] 우리 - 크리스마스 프로모션 미션 코드리뷰용！ #1
Conversation
| } catch (error) { | ||
| this.outputView.print(error.message); | ||
| menuInput = await this.#getMenuInput(); | ||
| } |
There was a problem hiding this comment.
해당 부분은 같은 로직이 date, menu 에 따라 반복되고 있어서, 하나의 메서드로 구현하면 좋을 것 같습니다.
| this.outputView.print(HEADER.EVENT_BADGE); | ||
| const badge = event.getBadge(totalBenefits); | ||
| this.outputView.print(badge); | ||
| return; |
There was a problem hiding this comment.
클래스를 분리하는 것의 연습이 미션의 주된 목표였으니
할인을 적용받는 로직 , 적용받은 할인을 print해주는 로직 으로
(예를들어, Discount 클래스 파일, DiscountPrint클래스 파일)
분리해 보는 것은 어떨까요??
| export const WEEK_EVENT_DISCOUNT = 2023; | ||
| export const GIVEAWAY_EVENT_MIN_PRICE = 120000; | ||
| export const MIN_AMOUNT = 1; | ||
| export const MAX_AMOUNT = 20; |
There was a problem hiding this comment.
해당 부분에 UI로직에 쓰이는 상수가 대부분인 것 같습니다.
이것도 하나의 객체로 묶는 것이 어떨까요??
const UICONSTANTS = {
MONTH: 12,
FIRST_DAY_OF_MONTH = 1,
......
}
| } | ||
| this.#throwError(); | ||
| } | ||
| getName() { |
There was a problem hiding this comment.
3주차 공통 피드백에 보면
Lotto에서 데이터를 꺼내지(get) 말고 메시지를 던지도록 구조를 바꿔 데이터를 가지는 객체가 일하도록 한다.
는 내용이 있습니다!
가지고 있는 메소드가 더 일하도록 만들면 좋을거 같습니다!
There was a problem hiding this comment.
3주차 공통 피드백에 보면
Lotto에서 데이터를 꺼내지(get) 말고 메시지를 던지도록 구조를 바꿔 데이터를 가지는 객체가 일하도록 한다.는 내용이 있습니다! 가지고 있는 메소드가 더 일하도록 만들면 좋을거 같습니다!
안그래도 그 부분을 고민했었는데, 혹시 다른 방법 떠오르는 게 있으실까요？ㅠ 저는 아무리 생각해봐도 다른 방법이 떠오르지 못해서 저렇게 사용해버렸습니다.. 좋은 아이디어 있으시다면 공유 부탁드려욥
| @@ -0,0 +1,113 @@ | |||
| export const NOTHING = ''; | |||
There was a problem hiding this comment.
밑에 COURSE 객체 처럼 하나의 객체로 모아두는건 어떨까요!
| import { MissionUtils } from '@woowacourse/mission-utils'; | ||
|
|
||
|
|
||
| const InputView = class { |
There was a problem hiding this comment.
InputView를 더 간단히 쓸 수 있네요! 배워갑니다!!
| CHAMPAGNE: '샴페인', | ||
| }; | ||
|
|
||
| export const EVENT_DAYS = { |
There was a problem hiding this comment.
Date('yyyy-mm--dd').getDay() 하시면 요일을 좀 더 편하게 가져올 수 있습니다!
There was a problem hiding this comment.
특수문자가 포함된 케이스라던지, 소숫점이 들어간 숫자 등 혹시 모르니 여러 케이스를 테스트 해보는건 어떨까 싶어요!
저도 여러 케이스로 테스트코드를 작성하다가 버그를 수정할 수 있던 적이 몇번 있더라구요
| import Date from './Model/Date.js'; | ||
| import Menu from './Model/Menu.js'; | ||
| import Event from './Model/Event.js'; | ||
| import { AMOUNT, ASK, DIVIDER_HYPHEN, GREETING, HEADER, MENU, SEE_MORE_ABOUT_EVENTS } from './constants.js'; |
There was a problem hiding this comment.
/Model/index.js
import {default as Date} from './Model/Date.js';
import {default as Menu} from './Model/Menu.js';
import {default as Event} from './Model/Event.js';src/App.js
import {Date, Menu, Event} from './Model/index.js'Model 폴더 아래에 index.js 파일을 위와 같이 작성해서 만들면 import가 조금 더 간단하게 될 수 있더라구요!
저도 다른 분들이 알려줘서 이렇게 적용해보고 있습니다 ㅎㅎ
| } catch (error) { | ||
| this.outputView.print(error.message); | ||
| menuInput = await this.#getMenuInput(); | ||
| } |
There was a problem hiding this comment.
제가 혹시 코드에 대한 이해가 부족해서 드리는 질문일 수도 있는데,
다음과 같은 상황에서도 계속 입력을 받나요??
1. 잘못된 date 값 입력 -> 에러 출력
2. 한번 더 잘못된 date 값 입력
뭔가 코드상에선 1번만 예외처리를 하는 것 같이 보여서요!
직접 실행해볼 수 없으니 질문드릴 수 밖에 없군요 ㅜㅜ
There was a problem hiding this comment.
제가 혹시 코드에 대한 이해가 부족해서 드리는 질문일 수도 있는데, 다음과 같은 상황에서도 계속 입력을 받나요??
1. 잘못된 date 값 입력 -> 에러 출력 2. 한번 더 잘못된 date 값 입력뭔가 코드상에선 1번만 예외처리를 하는 것 같이 보여서요! 직접 실행해볼 수 없으니 질문드릴 수 밖에 없군요 ㅜㅜ
음 그렇네요..！ 생각해보니 이런 형태면 두번째 입력 때는 에러로 아예 프로그램이 끝날 것 같아요..！ 저도 한번 다시 테스트를 해봐야 하는 부분이네요. try&catch를 getInput 함수 안에 넣었으면 괜찮았을까요？？
| #validateRange(number) { | ||
| if (number >= FIRST_DAY_OF_MONTH && number <= LAST_DAY_OF_MONTH) { | ||
| return number; | ||
| } |
There was a problem hiding this comment.
#validateInteget(), #validateRange() 두 함수가 예외 범위가 아닌 경우, number를 return하도록 되어있는데,
#validateDate()에서 이 두 함수의 리턴값을 활용하고 있지 않는 것 같아요
return값을 사용하는 부분이 없다면, 아래와 같이 작성해보는건 어떨까 싶습니다!
#validateInteger(number) {
if (!Number.isInteger(number)) {
this.#throwError();
}| return AMOUNT.ZERO; | ||
| } | ||
| return AMOUNT.ZERO; | ||
| } |
There was a problem hiding this comment.
메소드의 길이가 15자를 넘어가서, dessert관련 계산 로직과 food관련 계산 로직을 따로 분리해보는 건 어떨까 싶어요!
다른 더 좋은 방법이 있다면 그것도 좋을 것 같습니다 :)
No description provided.