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

[앨런] 치킨집 미션 제출합니다. #7

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

hongbin-dev
Copy link
Member

와 정말!

커피사기 싫어서 되는 코드 제출했어요..

중간에 리팩토링 될수도 있는데.....! 잘모르겠어여...

감사합니다

Copy link

@YerinCho YerinCho left a comment

Choose a reason for hiding this comment

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

☺️

table.clear();
} else if (PosMenu.EXIT == posMenu) {
System.exit(0);
}

Choose a reason for hiding this comment

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

함수(또는메서드)의 길이가 15라인을 넘어가지 않도록 구현한다.

  • 함수(또는메서드)가 한가지 일만 잘 하도록 구현한다.
  • else예약어를 쓰지 않는다.
    +힌트: if조건절에서 값을 return하는 방식으로 구현하면 else를 사용하지 않아도 된다.
  • else를 쓰지 말라고 하니 switch/case로 구현하는 경우가 있는데 switch/case도 허용하지 않는다.

public static PosMenu of(int functionNumber) {
return Arrays.stream(values())
.filter(posMenu -> posMenu.function == functionNumber)
.findAny()

Choose a reason for hiding this comment

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

findFirst()가 더 좋지 않을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

1개의 요소를 찾을때라서 순서가 상관없는데요.
findany()가 성능이 안좋은건가요?


public int calculateChickenCount() {
int sum = 0;
for (Menu menu : orderByCount.keySet()) {

Choose a reason for hiding this comment

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

depth?

Copy link

Choose a reason for hiding this comment

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

스트림을 활용해볼까요?

Comment on lines +24 to +30
public void printTables(final Tables tables) {
System.out.println("## 테이블 목록");
final int size = tables.getTableById().size();
printLine(TOP_LINE, size);
printTableNumbers(tables.getTableById().values());
printLine(BOTTOM_LINE, size);
}

Choose a reason for hiding this comment

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

사용하고 있는 테이블은 표시가 안될꺼 같네요!?

Copy link

Choose a reason for hiding this comment

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

표시가 안되네요 ㅠ

Copy link

@kimevanjunseok kimevanjunseok left a comment

Choose a reason for hiding this comment

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

안녕하세요 앨런!!!! 반갑습니다!!
정말 간단한 피드백 남겼습니다.
controller 코드 refactoring 하시고 또 피드백 남겨드리겠습니다!!!😂😂(많이 바빴던 모습이 보입니다😂😂)

Menus menus = new Menus(MenuRepository.menus());
Tables tables = new Tables(TableRepository.tables());

while (true) {

Choose a reason for hiding this comment

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

하핫....말 안해도...아시죠??😂😂
depth, indent...화이팅 하십쇼!!!

public long calculateTotalMoney() {
long sum = 0;

for (Menu menu : orderByCount.keySet()) {

Choose a reason for hiding this comment

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

stream을 사용하지 않은 이유가 있을까요?

return amount * price;
}

static final class OrderCountCache {

Choose a reason for hiding this comment

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

저에게 피드백 주신 것을 구현한 부분이군요!!!

this.price = price;
}

public long calculatePrice(OrderCount orderCount) {

Choose a reason for hiding this comment

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

이 부분을 구현하면서 생각했었습니다.
앨런은 Order에서 OrderCount를 get해서 menu.calculatePrice(orderCount)처럼 orderCount를 Menu에 보냅니다.
그리고 Menu에서 price를 orderCount.multiply(price)처럼 OrderCount에 보냅니다.

저도 이 과정처럼 했다가 Menu클래스가 Count를 알 필요가 있을까?, 그리고 Menu에서 계산되서 나오는 것이 아니라 한 번 더 OrderCount에 price를 보내준다? 이 과정이 어색하다 생각하였고 결국 Order에서 menu의 price와 Count의 amount를 get하여 둘이 곱해주는 방식으로 하였습니다.

앨런은 어떻게 생각하시는지 궁금합니다!!!

Copy link

@minuyim minuyim left a comment

Choose a reason for hiding this comment

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

안녕하세요 앨런!
치킨집 구현하시느라 정말 고생 많으셨습니다.
간단하게 리뷰드리면서 생각할 거리 몇 개 남겨봤습니다. 감사합니다.

CACHE(2, Arrays.asList(new ChickenDiscountStrategy()), Arrays.asList(new CacheDiscountStrategy()));

private final int type;
private final List<DiscountStrategy> totalDiscountStrategies;
Copy link

Choose a reason for hiding this comment

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

제 생각에는 PaymentType에서 totalDiscountStrategies를 가지는 것보다 afterDiscountStrategies만 가지고 있는 편이 좋을 것 같다는 생각이 드는데 이 의견에 대해서는 어떻게 생각하시나요? PaymentType에 따라서 totalDiscountStrategies가 바뀌는게 아닐 것 같아서요.

import java.util.List;
import java.util.Map;

public class Tables {
Copy link

Choose a reason for hiding this comment

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

기존 Repository를 좀 더 효율적으로 바꾸셨군요!

this.orderByCount = new HashMap<>();
}

public void add(Menu menu, OrderCount orderCount) {
Copy link

Choose a reason for hiding this comment

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

map 기능 중 merge를 써보는 건 어떨까요?


public int calculateChickenCount() {
int sum = 0;
for (Menu menu : orderByCount.keySet()) {
Copy link

Choose a reason for hiding this comment

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

스트림을 활용해볼까요?

return amount * price;
}

static final class OrderCountCache {
Copy link

Choose a reason for hiding this comment

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

내부 클래스에 접근 제어자가 없는 이유가 있을까요?

Comment on lines +24 to +30
public void printTables(final Tables tables) {
System.out.println("## 테이블 목록");
final int size = tables.getTableById().size();
printLine(TOP_LINE, size);
printTableNumbers(tables.getTableById().values());
printLine(BOTTOM_LINE, size);
}
Copy link

Choose a reason for hiding this comment

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

표시가 안되네요 ㅠ

Menus menus = new Menus(MenuRepository.menus());
Tables tables = new Tables(TableRepository.tables());

while (true) {
Copy link

Choose a reason for hiding this comment

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

😉

void calculateChickenCount() {
Order order = new Order();
Menu 후라이드 = new Menu(1, "후라이드", Category.CHICKEN, 10_000);
Menu 양념 = new Menu(2, "양념이드", Category.CHICKEN, 10_000);
Copy link

Choose a reason for hiding this comment

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

😆

Copy link
Member Author

Choose a reason for hiding this comment

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

양념이드 뭐죠ㅋㅋㅋ

class OrderTest {
@DisplayName("메뉴를 추가하여 갯수를 확인한다.")
@Test
void name() {
Copy link

Choose a reason for hiding this comment

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

메소드 명을 수정해볼까요?

Comment on lines +37 to +48
public long calculatePrice(Order order) {
long money = order.calculateTotalMoney();
money = preDiscount(order, money);
return afterDiscount(order, money);
}

private long afterDiscount(Order order, long money) {
for (DiscountStrategy discountStrategy : afterDiscountStrategies) {
money = discountStrategy.calculate(money, order);
}
return money;
}
Copy link

Choose a reason for hiding this comment

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

현재 DiscountStrategy를 사용하고 있는 방향성이 두가지로(할인액 자체를 구하는 경우, 할인 후 총가격을 구하는 경우) 나뉜 것 같습니다. 사용자에게 혼란을 줄 수 있을 것 같은데 어떻게 생각하시나요?

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.

5 participants