feat : 결제 기능 #15

bominjang · 2021-07-26T14:53:47Z

TODO : test 케이스 작성, 코드 함수화 하기..

- TODO : test 케이스 작성, 코드 함수화 하기..

Gobukgol · 2021-08-01T15:11:55Z

안녕하세요 보민님! PR 주셨는데 리뷰가 많이 늦었네요 ㅠㅠ 간단하게 리뷰 남길 수 있는거 남기고 계속해서 리뷰하겠습니다!

예전에 스터디했을때 어디까지 했었는지, 객체지향 생활체조 원칙 기억하시는지 잘 모르겠지만 일단 간간히 else 구문이 보이는 것 같아요! Application 쪽 제외하고 다른 비즈니스 로직에 있는 else 구문부터 제거해보시죠!
또 메서드에 indent가 2개 들어간 즉, 2중 for문이나, for 문안에 if문이 있는 것들도 제거해봐요!

그리고 요청사항이 있는데 각 클래스가 어떤 역할을 하는지, 어떤 책임을 가지고있는지 간단하게 정리해서 코멘트로 남겨주세요!

궁금하신거나 얘기할게 있다면 부담없이 코멘트 남겨주세요!

bominjang · 2021-08-03T06:09:10Z

안녕하세요 !! 리뷰 달아주셔서 감사합니다 :)
객체지향 생활 체조 원칙부터 지켜서 코드 수정하고, 각 클래스의 역할과 책임도 정리해서 곧 코멘트 남기겠습니다 !

감사합니다 !!

NDjust · 2021-08-03T12:35:24Z

@bominjang 님 안녕하세요!

리뷰가 늦어서 죄송합니다 ㅠㅠ

Step 1 부터 코드를 보고 있는데, 인텔리제이 사용하시면, 클래스 다이어그램 간단하게 캡쳐해서 민형이형 말해주신 책임과 역할을 같이 적어주시면 의도를 좀 더 파악하기 쉽고, 코드 리뷰 드리기도 수월할 거 같습니다!

사소한 거지만 코드 포맷팅도 안되어 있는 부분이 조금 있어서, 수정해주시면 감사드리겠습니다. (_ _)

늦었지만 올려주시는대로 정리해서 지난 PR 포함해서 코멘트 드리겠습니다.

BangKiHyun · 2021-08-03T13:45:03Z

안녕하세요! 리뷰가 많이 늦어서 죄송합니다 ㅠㅠ 전체적인 코드보고 간단하게 리뷰 남겨요!

객체지향 설계의 핵심은 협력을 구성하기 위한 적절한 객체를 찾고, 적절한 책임을 할당하는 것이라고 생각해요. 그래서 각각의 객체들의 역할, 책임, 협력을 한 번 더 생각해 보시면 더 깔끔한 코드를 만들 수 있을거 같아요!

예를 들어, 존재하는 테이블인지 검사하는 책임은 테이블들을 알고 있는 객체의 역할이면 좋을거 같아요!
객체가 알고 있고, 할 수 있는 것이 무엇인지 생각해 보는게 책임을 할당하는데 힌트가 될 것 같습니다!

늦었지만 올려주시는대로 계속해서 리뷰 남기겠습니다!

bominjang · 2021-08-05T04:03:28Z

@NDjust , @BangKiHyun
안녕하세요 ! 바쁘신 와중에 리뷰 주셔서 감사합니다 :)
말씀해주신 것들 모두 반영하여 다시 PR 하겠습니다 !

감사합니다!!

- 1차

bominjang · 2021-08-07T15:05:24Z

안녕하세요 :)
클래스 다이어그램 첨부하여 코멘트 남깁니다!!
객채의 책임을 다시 생각해보고 코드 수정해서 다시 PR 하겠습니다!

감사합니다:)

NDjust · 2021-08-12T12:31:55Z

src/main/java/repository/OrderRepository.java

-    public Map<Menu, Long> getBill(int tableNum){
-        List<Order> orders = findByTableNumber(tableNum);
+    //해당 테이블에서 주문한 메뉴와 수량을 return한다.
+    public Map<Menu, Long> getBill(int tableNumber){


Map으로 들고 있는 것보단 객체로 빼서 책임을 할당하는건 어떨까요?

아래 코드에도 bill을 의미하는 맵을 가지고 로직을 처리하는 것보단, 가독성이나 추후 확장성에도 좋아보입니다.

개인적인 견해이니, 참고만 하시면 좋을 거 같습니다!

NDjust · 2021-08-12T12:40:33Z

src/main/java/repository/OrderRepository.java

        int count= (int)orders.stream()
-                .filter(order->order.isEqualTable(tableNum))
-                .filter(order->menuNum==order.getMenuNum())
+                .filter(order->order.isEqualTable(tableNumber))


요 스트림은 인라인으로 빼서 바로 리턴해도 될 거 같습니다!

NDjust · 2021-08-12T12:46:16Z

src/main/java/domain/PayType.java

+
+//지불 수단
+public enum PayType {
+    CARD(1, 0), CASH(2,10);


만약 이렇게 구현해놓은 이후에 요구사항이 생겨 할인 수단이 다양해진다면 어떻게 구성하실걸까요?!

추후에 충분히 카드에만 해당하는 할인수단, 현금에만 해당하는 할인수단이 다르게 적용될 수도 있을 거 같은데, 확장성을 좀 더 고려해보셔도 좋을 거 같습니다!

NDjust · 2021-08-12T12:47:13Z

src/main/java/repository/OrderRepository.java


+    //해당 테이블이 주문된 테이블인지 검사한다.
+    public boolean isOrderedTable(int tableNumber){
+        if(getBill(tableNumber).isEmpty()){


요런 if( 컨벤션이 맞지 않은게 많은신 거 같은데, IDE를 통해 한번에 컨벤션 정리하셔도 좋을 거 같습니다 ㅎㅎ

NDjust · 2021-08-12T12:47:50Z

@bominjang 고생하셨습니다!

코멘트는 참고용으로 보시면 좋을 듯합니다. 의견 있으시면 편하게 말씀해주세요 ㅎㅎ

BangKiHyun

@bominjang 고생 많으셨습니다! 소소한 코멘트 남깁니다!

개인적으로 OrderRepository가 하는 일이 조금 많아보였습니다!
책임을 할당할 수 있는 객체를 좀 더 고려해보시는 것도 좋을 거 같습니다!

BangKiHyun · 2021-08-14T11:12:13Z

src/main/java/domain/Table.java

+    public boolean isEqualNumber(int tableNumber){ return this.number==number;}
+


입력받은 파라미터가 사용되지 않고있어요! 이렇게 되면 항상 true를 리턴할 거 같아요!

BangKiHyun · 2021-08-14T11:50:13Z

src/main/java/service/CafeOrderService.java

+    //해당 수량만큼의 주문을 추가한다.
+    public void orderMenu(int menuNumber, int count, int tableNumber)
    {
        for(int i=0;i<count;i++){
-            orderRepository.addOrder(new Order(menuNum, tableNum));
+            orderRepository.addOrder(new Order(menuNumber, tableNumber));
        }
    }


주문한 메뉴의 수량만큼 주문을 추가하는데, 수량만큼 추가하는게 아닌 메뉴의 수량 자체를 알고 있는 객체를 추가하면 어떨까요?

한 번의 주문에 메뉴의 수량만큼 주문이 추가되는게 어색하게 느껴져서 말씀드려봅니다!

Gobukgol

리뷰 간단하게 남겼습니다!
리턴값이 boolean 메소드들 중에 좀 더 간략하게 할 수 있는 메소드들이 보입니다!
어떻게 하면 간략하게 할 수 있을지 고민해보시면 좋을 것 같습니다!

Gobukgol · 2021-08-22T03:48:39Z

src/main/java/Application.java

        if(isValidTblNum==false){
            orderMenu(tables, cafeOrderService);
        }
        final List<Menu> menus = MenuRepository.menus();
        OutputView.printMenus(menus);
        int menuNum = InputView.inputMenu();
-        boolean isValidMenuNum = cafeOrderService.checkMenuNum(menuNum);
+        boolean isValidMenuNum = cafeOrderService.isValidMenuNumber(menuNum);
        if(isValidMenuNum==false){


isValidMenuNum 이 이미 boolean 이니 false와 비교하지 않고 바로 if(!isValidMenuNum) 으로 사용하셔도 좋을 것 같아요!

Gobukgol · 2021-08-22T03:50:58Z

src/main/java/Application.java

+        boolean isValidMenuCount = cafeOrderService.checkMaximumCount(tableNum, menuNum, menuCount);
        if(isValidMenuCount==false){


위와 마찬가지인 부분!
추가로 cafeOrderService.checkMaximumCount 메소드는 맥시멈 카운터인지 체크한다~ 라는 의미로 읽히는데 isValidMenuCount 변수명과 의미가 조금 다른 것 같아요!

Gobukgol · 2021-08-22T03:55:44Z

src/main/java/repository/OrderRepository.java

+        if(existedMenusCount+menuCount<=MAX_NUMBER_PER_MENU){
            return true;
-        }else{
-            return false;
        }
+        return false;


return existedMenusCount + menuCount <= MAX_NUMBER_PER_MENU

로 바로 리턴하는건 어떨까요?

bominjang · 2021-08-22T04:40:37Z

리뷰 감사합니다!!
피드백 반영하여 다시 커밋하겠습니다 :)
감사합니다 !

- Bill 객체를 생성 - 주문 넣는 방식 변경 (기존 : 메뉴의 수량 만큼 order 객체 추가 -> 변경 후 : 메뉴의 수량을 알고있는 order 객체 하나만 추가) - boolean 처리 단순화 등등..!!

bominjang · 2021-08-30T09:55:20Z

안녕하세요 !
기한이 조금 많이 지났지만,,ㅠㅠ 리팩토링 완료하여 PR드립니다!!
시간 나실 때 리뷰 주시면 정말 감사하겠습니다 :)
그리고 바뀐 클래스 다이어그램도 함께 첨부하겠습니다!

Gobukgol

일이 바빠서 많이 늦어졌네요... 죄송합니다 ㅠㅠ

리뷰 남겼습니다! 객체 지향 생활 체조 원칙을 지키고 코드들 개선한게 많이 보여요!
많이 도움 못드리는 것 같은데도 열심히 참여해주셔서 감사합니다!
그리고 언제든지 이해가 안가거나 다르게 생각하는 점이 있다면 언제든지 편하게 의견 남겨주세요!
서로에게 도움이 많이 될 것 같아요!

만약에 PR로 의견 남기거나 요청했는데도 너무 늦어서 진행에 차질이 생기면 찬인이형이나 민정이 통해서 알려주시면 감사하겠습니다!

src/main/java/Application.java

Gobukgol · 2021-09-12T05:29:19Z

src/main/java/Application.java

+	final static int ORDER = 1;
+	final static int PAY = 2;
+	final static int STOP = 3;


상수 👍

Gobukgol · 2021-09-12T05:34:40Z

src/main/java/Application.java

-        long amountOfPayment = cafeOrderService.getAmountOfPayment(tableNum, payType);
-        OutputView.printAmountOfPayment(amountOfPayment);
-    }
+	private static void payOrders(List<Table> tables, CafeOrderService cafeOrderService) {


밑에 orderMenu 메소드와 똑같이 생각해주시면 될 것 같아요!
지금 application에 Repository가 노출되어있어요! 그 의미는 application에 도메인 로직이 노출되어있다고 생각해요!
만약 이번 카페 미션을 Spring boot로 옮긴다면 어떻게 구성할까? 생각해보시는 것도 좋을 것 같아요!

리뷰 감사합니다!!
application에 repository가 없어도 되더라구요..!ㅎㅎ 앞으로는 로직 구성할 때, 그 부분도 고려하겠습니다!!

Gobukgol · 2021-09-12T05:42:57Z

src/main/java/repository/OrderRepository.java

-        return numberOfCakes;
-    }
+	//해당 테이블에서 주문한 케익의 갯수를 계산한다.
+	public int getNumberOfCakes(Bill bill) {


OrderRepository가 Bill 객체를 꼭 알고있어야하나용??
OrderRepository의 역할은 Order 객체를 저장하는 관리하는 역할이라고 생각해요!
Bill 객체가 OrderRepository를 알고있고, OrderRepository가 Bill 객체를 알고있어서 서로에 대한 싸이클이 생겨요! 서로 강하게 결합되어 있어서 한 객체에 수정이 생겼을 때 다른 객체 또한 수정해야할 상황이 생길 것 같아요!
이런 갯수를 계산하거나 총 합을 계산하는 역할을 다른 클래스에게 책임을 전달해도 좋다고 생각해요!
보민님은 어떻게 생각하시나요??

바쁘신 와중에 리뷰 남겨주셔서 감사합니다 !! :)
orderRepository가 order 객체만 관리하는 게 맞다고 생각이 들어서, order객체 관리 이외의 것들은 Bill 객체로 책임을 전달했습니다! cafeOrderService의 계산 메서드들도 Bill 객체로 전달했습니다. 그리고 사이클을 제거하려고 노력했습니다!!
그런데 Bill이 금액을 계산하면 할인율 정보를 bill이 갖고있는 게 맞을까요..? 아니면 cafeOrderService가 알고있어야 할까요..?!
원래 cafeOrderService가 할인율에 대한 정보를 갖고 있는 게 맞다고 생각했는데, 그러다보니 최종 금액을 계산할 때 cafeOrderService가 Bill에게 정보를 다 전달해줘야하더라구요..! 그래서 어차피 다 전달해줄거면 할인율 상수를 Bill이 갖고있어야하는 게 맞다고 생각이 들어서 이렇게 리팩토링 했습니다..!!
아직 객체에 알맞은 책임을 부여하는 게 어려운 것 같아요ㅠㅠ 어떤 게 적절한 건지 조언해주시면 감사하겠습니다!!

할인율을 계산하는 주체가 Bill 이라면 할인율 정보를 Bill이 가지고 있어야한다고 저도 생각합니다!

- OrderRepository와 bill 사이의 사이클을 제거함. - OrderRepository가 갖고있던 책임을 변경.

bominjang · 2021-09-14T07:33:18Z

@Gobukgol
바쁘실텐데 리뷰 주셔서 정말정말 감사합니다 :)!
리뷰 달아주시는 덕분에 제가 생각하지 못했던 부분도 고려하게 되고, 몰랐던 것들도 많이 배우고 있습니다!!ㅎㅎ
시간 나실 때 리뷰 주시면 감사하겠습니다 ☺️☺️!!

Gobukgol

메일 주신 것 확인했습니다! 오래 기다리셨네요.... ㅠㅠ
이번에는 간단하게 남겼습니다. 객체의 역할이나 책임에 대해 많이 고민하시고 적용한 것 같아요!
확인해주시고 다른 의견이나 궁금한 점이 있다면 코멘트나 메일 남겨주세요!!
계속 리뷰가 늦어지는 점 다시 한 번 죄송합니다 ㅠㅠ

Gobukgol · 2021-09-27T03:24:16Z

src/main/java/domain/Bill.java

+		long totalSumOfPayment = this.getSumOfPayment();
+		int numberOfCakes = this.getNumberOfCakes();
+		if (numberOfCakes >= DISCOUNT_FOR_CAKES) {
+			totalSumOfPayment -= DISCOUNT_MONEY_PER_NUM_OF_CAKES * (numberOfCakes / 3);


요부분 만들어주신 상수 DISCOUNT_FOR_CAKES 로 변경해주시면 될 것 같아요!

Gobukgol · 2021-09-27T03:28:49Z

src/main/java/domain/Bill.java

+	}
+
+	//생성
+	public Bill(OrderRepository orderRepository, int tableNumber) {


Bill 객체가 orderRepository를 인자로 받아서 생성하지 않고 Bill 객체를 생성하는 주체가 orderRepository에서 order List를 조회해서 넘겨주는 방식은 어떻게 생각하시나요???

Gobukgol · 2021-09-27T03:34:31Z

src/main/java/domain/Bill.java

+		for (Menu key : orderedMenus.keySet()) {
+			if (key.getNumber() != menuNumber)
+				continue;
+			menuCount = orderedMenus.get(key);
+			break;
+		}


stream의 findFirst를 사용해서 찾아보는건 어떨까요???
잘못된 menuNumber에 대한 validation도 체크할 수 있고, indent 규칙도 지킬 수 있을 것 같아요!

feat : 결제 기능

243d011

- TODO : test 케이스 작성, 코드 함수화 하기..

Gobukgol requested review from Gobukgol, BangKiHyun and NDjust August 1, 2021 15:12

Gobukgol assigned bominjang Aug 1, 2021

edit : 객체지향 생활체조원칙 적용

def2667

- 1차

refactor : 클래스 책임 할당

cd63784

NDjust reviewed Aug 12, 2021

View reviewed changes

BangKiHyun reviewed Aug 14, 2021

View reviewed changes

Gobukgol reviewed Aug 22, 2021

View reviewed changes

refactor : Bill 객체 생성, 객체의 책임 분할

cf1e207

- Bill 객체를 생성 - 주문 넣는 방식 변경 (기존 : 메뉴의 수량 만큼 order 객체 추가 -> 변경 후 : 메뉴의 수량을 알고있는 order 객체 하나만 추가) - boolean 처리 단순화 등등..!!

bominjang requested review from Gobukgol, BangKiHyun and NDjust August 31, 2021 06:32

Gobukgol reviewed Sep 12, 2021

View reviewed changes

bominjang added 2 commits September 14, 2021 15:46

refactor : 사이클 제거 및 객체 책임 변경

315dc17

- OrderRepository와 bill 사이의 사이클을 제거함. - OrderRepository가 갖고있던 책임을 변경.

refactor : application에 repository 노출 없앰.

d171c80

Gobukgol reviewed Sep 27, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat : 결제 기능 #15

feat : 결제 기능 #15

bominjang commented Jul 26, 2021

Gobukgol commented Aug 1, 2021

bominjang commented Aug 3, 2021

NDjust commented Aug 3, 2021

BangKiHyun commented Aug 3, 2021

bominjang commented Aug 5, 2021

bominjang commented Aug 7, 2021

NDjust Aug 12, 2021

NDjust Aug 12, 2021

NDjust Aug 12, 2021

NDjust Aug 12, 2021

NDjust commented Aug 12, 2021

BangKiHyun left a comment

BangKiHyun Aug 14, 2021

BangKiHyun Aug 14, 2021

Gobukgol left a comment

Gobukgol Aug 22, 2021

Gobukgol Aug 22, 2021

Gobukgol Aug 22, 2021

bominjang commented Aug 22, 2021

bominjang commented Aug 30, 2021

Gobukgol left a comment

Gobukgol Sep 12, 2021

Gobukgol Sep 12, 2021

bominjang Sep 14, 2021

Gobukgol Sep 12, 2021

bominjang Sep 14, 2021 •

edited

Loading

Gobukgol Sep 27, 2021

bominjang commented Sep 14, 2021

Gobukgol left a comment

Gobukgol Sep 27, 2021

Gobukgol Sep 27, 2021

Gobukgol Sep 27, 2021

		public boolean isEqualNumber(int tableNumber){ return this.number==number;}

		boolean isValidMenuCount = cafeOrderService.checkMaximumCount(tableNum, menuNum, menuCount);
		if(isValidMenuCount==false){

feat : 결제 기능 #15

Are you sure you want to change the base?

feat : 결제 기능 #15

Conversation

bominjang commented Jul 26, 2021

Gobukgol commented Aug 1, 2021

bominjang commented Aug 3, 2021

NDjust commented Aug 3, 2021

BangKiHyun commented Aug 3, 2021

bominjang commented Aug 5, 2021

bominjang commented Aug 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NDjust commented Aug 12, 2021

BangKiHyun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gobukgol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bominjang commented Aug 22, 2021

bominjang commented Aug 30, 2021

Gobukgol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bominjang Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bominjang commented Sep 14, 2021

Gobukgol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bominjang Sep 14, 2021 •

edited

Loading