Step3 pull request #82

umanking · 2020-02-13T05:33:04Z

언제나 좋은 피드백 감사드립니다. 😃

commit message Angular guide 맞출려고 했습니다.
BDDMockito 패키지의 given()을 활용했습니다.
fixture를 아주 조금 활용했지만, 중복된 코드가 여전히 보이는 것 같습니다.
Controller 테스트는 아직 진행하지 못했습니다.
displayName를 썼지만, 테스트 메서드에 대해서도 직관적인 name convention이 필요해 보입니다.

wotjd243

테스트 코드를 잘 작성해 주셨습니다. 👍
고민해 보면 좋을 것 같은 의견을 남겨 놓았으니 충분히 고민하고 도전해 보세요!
진행 과정에서 궁금한 점은 댓글이나 Slack을 통해 물어보세요.

wotjd243 · 2020-02-20T01:50:01Z

README.md

+    - [ ] 주문 테이블 목록을 조회 할 수 있다.
+    - [ ] 주문 테이블의 상태를 비움으로 업데이트 할 수 있다. 
+        - [ ] 주문 테이블이 정상값 이어야 한다. 
+        - [ ] 반드시 식사가 끝난 경우에 상태를 비움으로 업데이트 할 수 있다.
    - [ ] 특정 테이블의 인원수를 업데이트 한다.


"빈 테이블은 방문한 손님 수를 입력할 수 없다."라는 요구 사항이 필요해 보여요. 아래의 코드를 참고해 주세요.

if (savedOrderTable.isEmpty()) { throw new IllegalArgumentException(); }

네 수정했습니다.

wotjd243 · 2020-02-20T01:50:15Z

README.md

-
-
+        - [ ] 인원수는 정상값만 입력 받는다.
+        - [ ] 주문 테이블이 반드시 존재해야 인원수 없데이트가 가능하다.


오타 발견!

Suggested change

- [ ] 주문 테이블이 반드시 존재해야 인원수 없데이트가 가능하다.

- [ ] 주문 테이블이 반드시 존재해야 인원수 업데이트가 가능하다.

wotjd243 · 2020-02-20T01:50:24Z

README.md

-
-
+        - [ ] 인원수는 정상값만 입력 받는다.
+        - [ ] 주문 테이블이 반드시 존재해야 인원수 없데이트가 가능하다.


위의 요구 사항은 아래의 코드와 일치해요.

final OrderTable savedOrderTable = orderTableDao.findById(orderTableId) .orElseThrow(IllegalArgumentException::new);

@wotjd243 이건 어떤 걸 수정해야 하나요?

오타만 수정하면 됩니다.

네 수정했습니다.

wotjd243 · 2020-02-20T01:55:32Z

README.md

-    - [ ] 테이블 그룹을 삭제한다.  
-    - [ ] 테이블 그룹을 삭제할때는 반드시 OrderStatus가 COMPLETION 이어야 한다.
+    - [ ] 테이블 그룹을 만들 수 있다.
+        - [ ] 주문테이블이 1개 이상인 경우에 만들 수 있다.


아래 요구 사항에 대해 고민해 봐요.

Suggested change

- [ ] 주문테이블이 1개 이상인 경우에 만들 수 있다.

- [ ] 주문 테이블이 2개 이상인 경우에 만들 수 있다.

이때 table Group을 짓는다라는 개념을 정확히 이해하지 못하고 요구사항을 분석했네요!

wotjd243 · 2020-02-20T01:57:41Z

README.md

-    - [ ] 주문 상태는 MEAL, COOKING, COMPLETION 으로 구성된다. 
+    - [ ] 주문을 할 수 있다.
+        - [ ] 아이템이 존재해야 주문을 할 수 있다.
+        - [ ] 주문한 아이템의 메뉴 개수와 메뉴DB에서 개수와 동일해야 한다.


아래 코멘트에 대해 고민해 봐요.
#35 (comment)

네 코드 중심으로 나열했었네요! 수정하겠습니다.

아직 반영되지 않았어요. 확인 부탁드려요.

wotjd243 · 2020-02-20T02:29:12Z

src/test/java/kitchenpos/bo/Fixtures.java

+ */
+public class Fixtures {
+
+    public static final long FIRST_ID = 1l;


FIRST_ID 대신 의미를 담고 있는 상수 이름이 좋겠죠?

@wotjd243
이건 여러 군데서 단순히 id값을 지정하는 부분이라 의미를 담고 있는 변수명이 어떤게 있을까요 ?

상수 이름은 PRODUCT_ID, MENU_GROUP_ID, MENU_ID 등이 적절해 보여요.

네 이 부분도 수정했습니다.

wotjd243 · 2020-02-20T03:40:04Z

src/test/java/kitchenpos/bo/Fixtures.java

+
+    @BeforeAll
+    public static void setUp() {
+        /* 상품 등록 */


주석 대신 메서드 분리를 통해 설명하면 다른 개발자도 쉽게 이해할 수 있을 것 같아요.

네 메서드로 따로 빼서 작업했습니다.

wotjd243 · 2020-02-20T03:42:02Z

src/test/java/kitchenpos/bo/MenuBoTest.java

+        assertThat(savedMenu).isNotNull();
+        assertThat(savedMenu.getId()).isEqualTo(menu.getId());


더 많은 단언문이 필요해 보여요.

네 수정해서 반영했습니다.

wotjd243 · 2020-02-20T03:44:38Z

src/test/java/kitchenpos/bo/MenuBoTest.java

+        assertThatExceptionOfType(IllegalArgumentException.class)
+                .isThrownBy(() -> menuBo.create(menu));
+    }
+}


아래 코멘트에 대해 고민해 봐요.
#27 (comment)

@wotjd243 isThrownBy 메서드를 가독성을 좋게 할려고 개행을 했는데, 이렇게 마지막에 오는 경우에는 프로그램의 종료를 의미하니까 붙여서 쓰고, 개행을 취소하는게 맞는 건가요 ?
제가 제대로 이해했나요?

assertThatExceptionOfType(IllegalArgumentException.class).isThrownBy(() -> menuBo.create(menu));

질문에 대한 답변은 Slack을 통해 남겼어요. 👍

wotjd243 · 2020-02-20T03:46:18Z

src/test/java/kitchenpos/bo/MenuGroupBoTest.java

+    @InjectMocks
+    private MenuGroupBo menuGroupBo;
+
+


아래 코멘트에 대해 고민해 봐요.
#27 (comment)

확인 했습니다. 코드를 습관적으로 빨리 타이핑 치다 보니 이런 부분이 자주 노출이 되네요
intellij 의 save action 플러그인을 설치해서 주로 실무에서 사용 하는데 저장시 자동 포매팅을 해준다는 믿음을 갖고 있어서 제대로 확인 안하는 것 같습니다! 감사합니다.

wotjd243

피드백을 잘 반영해 주셨습니다. 👍
추가로 고민해 보면 좋을 것 같은 의견을 남겨 놓았으니 충분히 고민하고 도전해 보세요!
진행 과정에서 궁금한 점은 댓글이나 Slack을 통해 물어보세요.

wotjd243 · 2020-02-26T06:38:52Z

README.md

-    - [ ] 주문 상태는 MEAL, COOKING, COMPLETION 으로 구성된다. 
+    - [ ] 주문을 할 수 있다.
+        - [ ] 아이템이 존재해야 주문을 할 수 있다.
+        - [ ] 주문한 아이템의 메뉴 개수와 메뉴DB에서 개수와 동일해야 한다.


아직 반영되지 않았어요. 확인 부탁드려요.

wotjd243 · 2020-02-26T06:41:45Z

src/main/java/calculator/Number.java

-    public Number(int value) {
-        this.value = value;
+    public Number(final int value) {
+        Objects.requireNonNull(value, "can not be null");
        validateNumber();


validateNumber()는 현재 의도한 대로 동작하고 있나요? 아래 코멘트에 대해 고민해 봐요.
#82 (comment)

생성자의 파라미터를 검증하지 않고, 멤버 변수에 대한 값을 검증 하니까
제대로 동작하지 않았겠네요.

wotjd243 · 2020-02-26T06:53:34Z

src/test/java/kitchenpos/bo/Fixtures.java

+ */
+public class Fixtures {
+
+    public static final int QUANTITY_ONE = 1;


Fixtures 외에 사용하는 곳이 없어 보이네요. public이 아닌 private으로 바꾸면 어떨까요?

Suggested change

public static final int QUANTITY_ONE = 1;

private static final int QUANTITY_ONE = 1;

수정 했습니다.

wotjd243 · 2020-02-26T07:48:13Z

src/test/java/kitchenpos/bo/MenuBoTest.java

+ * @since 2020-02-15
+ */
+@ExtendWith(MockitoExtension.class)
+class MenuBoTest extends Fixtures {


"메뉴를 조회할 수 있다."에 대한 테스트 코드를 작성해 보면 어떨까요?

누락 되었네요 감사합니다.

wotjd243 · 2020-02-26T07:48:36Z

src/test/java/kitchenpos/bo/MenuBoTest.java

+        assertThat(savedMenu.getId()).isEqualTo(menu.getId());
+        assertThat(savedMenu.getName()).isEqualTo(menu.getName());
+        assertThat(savedMenu.getPrice()).isEqualTo(menu.getPrice());
+        assertThat(savedMenu.getMenuGroupId()).isEqualTo(menu.getMenuGroupId());
+        assertThat(savedMenu.getMenuProducts().size()).isEqualTo(menu.getMenuProducts().size());
+        assertThat(savedMenu.getMenuProducts().get(0).getProductId())
+                .isEqualTo(menu.getMenuProducts().get(0).getProductId());
+        assertThat(savedMenu.getMenuProducts().get(0).getQuantity())
+                .isEqualTo(menu.getMenuProducts().get(0).getQuantity());
+        assertThat(savedMenu.getMenuProducts().get(0).getMenuId())
+                .isEqualTo(menu.getMenuProducts().get(0).getMenuId());
+        assertThat(savedMenu.getMenuProducts().get(0).getSeq())
+                .isEqualTo(menu.getMenuProducts().get(0).getSeq());


Assertions#assertAll(Executable...)을 찾아보고 그 기능을 사용해 보면 어떨까요?

그렇군요. assertAll을 사용했을때, 하나라도 실패하면 나머지 값을 굳이 계산하지 않기 때문에 훨씬 좋겠네요
https://stackoverflow.com/questions/40796756/assertall-vs-multiple-assertions-in-junit5

오히려 그 반대예요. 실패 횟수에 관계없이 전달된 모든 단언문을 항상 확인합니다.

wotjd243 · 2020-02-26T08:04:37Z

src/test/java/kitchenpos/bo/OrderBoTest.java

+    }
+
+    @Test
+    @DisplayName("주문이 존재 하지 않는 경우")


처음 보는 사람도 쉽게 이해할 수 있는 이름으로 변경한다면 어떻게 변경할 수 있을까요?

변경했습니다.

wotjd243 · 2020-02-26T08:05:06Z

src/test/java/kitchenpos/bo/OrderBoTest.java

+        final Order order = new Order();
+        order.setId(1l);
+
+        given(orderDao.findById(order.getId())).willThrow(IllegalArgumentException.class);


아래의 코드로 개선해 보면 어떨까요?

Suggested change

given(orderDao.findById(order.getId())).willThrow(IllegalArgumentException.class);

given(orderDao.findById(order.getId())).willReturn(Optional.empty());

수정했습니다.

wotjd243 · 2020-02-26T08:10:38Z

src/test/java/kitchenpos/bo/TableBoTest.java

+ * @since 2020-02-15
+ */
+@ExtendWith(MockitoExtension.class)
+class TableBoTest extends Fixtures {


"주문 테이블이 비어 있는 상태에서는 인원수 업데이트가 불가능하다."에 대한 테스트 코드를 작성해 보면 어떨까요?

추가했습니다.

wotjd243 · 2020-02-26T08:11:28Z

src/test/java/kitchenpos/bo/TableBoTest.java

+
+        given(orderTableDao.findById(orderTable.getId())).willReturn(Optional.of(orderTable));
+        given(orderDao.existsByOrderTableIdAndOrderStatusIn(resolvedOrderTable.getId(),
+                Arrays.asList(OrderStatus.COOKING.name(), OrderStatus.MEAL.name()))).willThrow(IllegalArgumentException.class);


아래의 코드로 개선해 보면 어떨까요?

Suggested change

Arrays.asList(OrderStatus.COOKING.name(), OrderStatus.MEAL.name()))).willThrow(IllegalArgumentException.class);

Arrays.asList(OrderStatus.COOKING.name(), OrderStatus.MEAL.name()))).willReturn(true);

wotjd243 · 2020-02-26T08:14:22Z

src/test/java/kitchenpos/bo/TableGroupBoTest.java

+
+        given(orderTableDao.findAllByTableGroupId(tableGroup.getId())).willReturn(Fixtures.orderTables);
+        given(orderDao.existsByOrderTableIdInAndOrderStatusIn(orderTableIds,
+                Arrays.asList(OrderStatus.COOKING.name(), OrderStatus.MEAL.name()))).willThrow(IllegalArgumentException.class);


아래의 코드로 개선해 보면 어떨까요?

Suggested change

Arrays.asList(OrderStatus.COOKING.name(), OrderStatus.MEAL.name()))).willThrow(IllegalArgumentException.class);

Arrays.asList(OrderStatus.COOKING.name(), OrderStatus.MEAL.name()))).willReturn(true);

given 조건이라서 throw exception 을 날리는게 아니라, 명시적으로 표시했어야 했네요

wotjd243

조금 더 고민해 보면 좋을 것 같은 의견을 남겨 놓았으니 충분히 고민하고 도전해 보세요!
진행 과정에서 궁금한 점은 댓글이나 Slack을 통해 물어보세요.

wotjd243 · 2020-02-28T13:48:40Z

src/test/java/kitchenpos/bo/MenuBoTest.java

+
+        assertAll(
+                () -> assertThat(savedMenu).isNotNull(),
+                () -> assertThat(savedMenu.getId()).isEqualTo(menu.getId()),


만약 savedMenu가 null이라면 어떤 위험이 발생할까요? 그 위험을 피할 수 있는 방법도 찾아보면 좋을 것 같아요.

wotjd243 · 2020-02-28T13:49:07Z

src/test/java/kitchenpos/bo/MenuBoTest.java

+                () -> assertThat(savedMenu.getMenuProducts().size())
+                        .isEqualTo(menu.getMenuProducts().size()),
+                () -> assertThat(savedMenu.getMenuProducts().get(0).getProductId())
+                        .isEqualTo(menu.getMenuProducts().get(0).getProductId()),
+                () -> assertThat(savedMenu.getMenuProducts().get(0).getProductId())
+                        .isEqualTo(menu.getMenuProducts().get(0).getProductId()),
+                () -> assertThat(savedMenu.getMenuProducts().get(0).getQuantity())
+                        .isEqualTo(menu.getMenuProducts().get(0).getQuantity()),
+                () -> assertThat(savedMenu.getMenuProducts().get(0).getMenuId())
+                        .isEqualTo(menu.getMenuProducts().get(0).getMenuId()),
+                () -> assertThat(savedMenu.getMenuProducts().get(0).getSeq())
+                        .isEqualTo(menu.getMenuProducts().get(0).getSeq())


아래 코멘트에 대해 고민해 봐요.
#82 (comment)

wotjd243 · 2020-02-28T13:56:07Z

src/test/java/kitchenpos/bo/MenuBoTest.java

+        assertAll(
+                () -> assertThat(list).isNotEmpty(),
+                () -> assertThat(list.size()).isEqualTo(menus.size()),
+                () -> assertThat(list.get(0).getMenuProducts().get(0).getSeq())
+                        .isEqualTo(menuProducts.get(0).getSeq()),
+                () -> assertThat(list.get(0).getMenuProducts().get(0).getMenuId())
+                        .isEqualTo(menuProducts.get(0).getMenuId()),
+                () -> assertThat(list.get(0).getMenuProducts().get(0).getQuantity())
+                        .isEqualTo(menuProducts.get(0).getQuantity()),
+                () -> assertThat(list.get(0).getMenuProducts().get(0).getProductId())
+                        .isEqualTo(menuProducts.get(0).getProductId())
+        );


아래 코멘트에 대해 고민해 봐요.
#82 (comment)

wotjd243 · 2020-02-28T13:56:41Z

src/test/java/kitchenpos/bo/MenuGroupBoTest.java

+        assertThat(result).isNotEmpty();
+        assertThat(result.size()).isEqualTo(menuGroupList.size());
+        assertThat(result).containsExactlyInAnyOrderElementsOf(menuGroupList);


아래의 코드로 개선해 보면 어떨까요?

Suggested change

assertThat(result).isNotEmpty();

assertThat(result.size()).isEqualTo(menuGroupList.size());

assertThat(result).containsExactlyInAnyOrderElementsOf(menuGroupList);

assertThat(result).containsExactlyInAnyOrderElementsOf(menuGroupList);

wotjd243 · 2020-02-28T13:57:11Z

src/test/java/kitchenpos/bo/OrderBoTest.java

+        final OrderLineItem orderLineItem = new OrderLineItem();
+        orderLineItem.setMenuId(1l);
+
+        final OrderLineItem orderLineItem1 = new OrderLineItem();


아직 반영되지 않았어요. 확인 부탁드려요.

wotjd243 · 2020-02-28T13:59:12Z

src/test/java/kitchenpos/bo/ProductBoTest.java

+
+        final Product result = productBo.create(product);
+
+        Assertions.assertThat(result.getId()).isEqualTo(product.getId());


아래 코멘트에 대해 고민해 봐요.
#82 (comment)

wotjd243 · 2020-02-28T14:00:03Z

src/test/java/kitchenpos/bo/TableBoTest.java

+        final OrderTable orderTable = Fixtures.orderTable;
+        orderTable.setNumberOfGuests(0);
+
+        assertThatExceptionOfType(IllegalArgumentException.class)
+                .isThrownBy(() -> tableBo.changeNumberOfGuests(orderTable.getId(), orderTable));


의도한 테스트와는 다른 테스트 코드입니다.

umanking added 14 commits February 13, 2020 10:33

refactor (Number): 파라미터 검증 추가

d6ed4da

refactor (StringCalculator): 메소드 레퍼런스로 변경

e993690

test (ProductBoTest): 상품 등록,조회 테스트

c85780e

refactor (README): 요구사항 중심으로 수정

d90d43b

refactor (Numbers): pass by reference 수정

3b4ab4f

test (MenuGroupBoTest): 테스트 케이스 작성

7a401cb

test (MenuTest): 테스트 케이스 작성

a76c968

test (MenuTest): 테스트 케이스 작성

f641567

refactor (Fixtures): 공통 영역 Fixture로 빼기

b650506

test (TableBoTest): 테스트 케이스 작성

e5bb6a9

test (TableGroupBoTest): 테스트 케이스 작성

1b18873

test (OrderBoTest): 테스트 케이스 작성

ba515e2

style (TestPackage): formatting & indent

c427f24

delete bin directory

d430573

wotjd243 requested changes Feb 20, 2020

View reviewed changes

umanking added 2 commits February 20, 2020 14:51

refactoring: 코드 리뷰 요청 사항 반영

f0802f7

refactoring: ID 변수명 변경 및 클래스별 개행 수정

0d5f61a

wotjd243 requested changes Feb 26, 2020

View reviewed changes

umanking added 4 commits February 27, 2020 14:24

refactor(Fixtures): 접근제어자 수정

1066496

refactor(MenuBoTest): 메뉴를 조회할 수 있다. 테스트 케이스

dddfc08

refactor(MenuBoTest): assertAll 로 변경

cee64c3

refactor(Test): 누락 된 부분이 많았습니다. (꾸벅)

7a372fe

wotjd243 requested changes Feb 28, 2020

View reviewed changes



		- [ ] 인원수는 정상값만 입력 받는다.
		- [ ] 주문 테이블이 반드시 존재해야 인원수 없데이트가 가능하다.

	- [ ] 주문 테이블이 반드시 존재해야 인원수 없데이트가 가능하다.
	- [ ] 주문 테이블이 반드시 존재해야 인원수 업데이트가 가능하다.

	- [ ] 주문테이블이 1개 이상인 경우에 만들 수 있다.
	- [ ] 주문 테이블이 2개 이상인 경우에 만들 수 있다.

		assertThat(savedMenu).isNotNull();
		assertThat(savedMenu.getId()).isEqualTo(menu.getId());

	public static final int QUANTITY_ONE = 1;
	private static final int QUANTITY_ONE = 1;

	given(orderDao.findById(order.getId())).willThrow(IllegalArgumentException.class);
	given(orderDao.findById(order.getId())).willReturn(Optional.empty());

	Arrays.asList(OrderStatus.COOKING.name(), OrderStatus.MEAL.name()))).willThrow(IllegalArgumentException.class);
	Arrays.asList(OrderStatus.COOKING.name(), OrderStatus.MEAL.name()))).willReturn(true);


		final Product result = productBo.create(product);

		Assertions.assertThat(result.getId()).isEqualTo(product.getId());

Step3 pull request #82

Are you sure you want to change the base?

Step3 pull request #82

Conversation

umanking commented Feb 13, 2020 • edited Loading

wotjd243 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wotjd243 Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wotjd243 Feb 20, 2020 • edited Loading

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

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

Choose a reason for hiding this comment

wotjd243 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

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

wotjd243 Feb 28, 2020 • edited Loading

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

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

Choose a reason for hiding this comment

wotjd243 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umanking commented Feb 13, 2020 •

edited

Loading

wotjd243 Feb 20, 2020 •

edited

Loading

wotjd243 Feb 20, 2020 •

edited

Loading

wotjd243 Feb 28, 2020 •

edited

Loading