findAllWithMenuProducts도 그렇지만 countByIdIn도 사용되고 있는 곳이 없는 것 같은데 확인해주세요.

MenuRepository도 더이상 사용하는 곳이 없네요. 이번 과제를 진행하면서도 사용하지 않는 코드가 생겨났죠. 마찬가지로 프로젝트가 오래되면 될 수록 그런 코드들이 많아집니다. 미사용 코드가 생길때마다 제거해주지 않으면 레거시를 다루기 더 힘들어 집니다. 단적으로 이번 과제를 진행하면서 이 프로젝트를 처음 분석할 때 사용하지 않는 코드들이 많았다면 어땠을까요? 🤔

그럼 이해해야 하는 코드들이 더 생기겠네요... ㅎㅎ
리팩토링을 코드를 바꾸는것뿐만 아니라 쓰지 않는 코드를 삭제하는 것도 중요하다는걸 느끼게 되었습니다 :)

참고로 메서드나 변수 등의 코드도 그렇지만 API는 더더욱 확인이 힘들 수 있습니다.
(일이주 access 로그를 보고 호출하지 않는걸 확인했지만 월 배치로 한달에 한번 호출 한다면?)

직접적인 규칙은 없긴하지만 개행이 너무 빈번하게 되어 있는거 같아요. 많이들 프로그래밍을 글 쓰기에 비유하듯이 문단을 나눈다고 생각하면 좋을 것 같아요.

저는 변수선언과 실제 로직에 대해서 개행을 주는 편입니다.
로직에서도 같은 로직이 아닌(예를들면 도메인을 변경하고 리포지토리에 저장할때 개행)때 개행을 사용하는편인데, 혹시 휴님께서는 어떤 규칙을 가지고 개행을 사용하는지 알수 있을까요?

사실 크게 규칙을 가지고 하진 않고 필요한 시점에 변수를 선언하여 사용한다정도입니다.(문학적 허용 느낌..? 🙄)
대략적으로 럿고와 비슷한데 변수선언과 로직 둘 다 1~3줄 정도라면 붙여서 사용하는 편인것 같네요.

int sum = 0;
sum = sum + 100;

int sum = 0;

sum = sum + 100;

정리하자면 아래와 같이?

        final List<OrderTable> orderTables = tableGroup.getOrderTables();
        final List<Long> orderTableIds = orderTables.stream()
                .map(OrderTable::getId)

혹은

        if (orderRepository.existsByOrderTableIdInAndOrderStatusIn(
                orderTableIds, Arrays.asList(OrderStatus.COOKING, OrderStatus.MEAL))) {
            throw new IllegalArgumentException("요리중이거나 식사 중에는 테이블 그룹을 해제할 수 없습니다.");
        }
        tableGroupRepository.deleteById(tableGroup.getId());

deleteById에서 메서드가 끝나고 위 if문도 심플한 편이니 붙이기도 할 것 같습니다. (이건 제코드에서도 살짝씩 다를 수 있을 것 같네요.)

감사합니다 :)

이건 참고만 해주세요. 지금 사이즈의 프로젝트에서 도메인별로 패키지를 나누는게 도리어 복잡도를 높이는 것 같기도 합니다. 🤔 혹은 각 도메인 별로 모두 나누기보다는 크게 상품과 주문으로 나눌수 있을 것 같아요. 이건 적용한다기 보단 참고만 해주세요.

DDD에 대한 정확한 지식이 없다보니... 좀더 공부해야겠네요 ㅠㅠ

이 부분은 솔직히 말씀드리자면 저도 DDD를 본격적으로 적용해보진 않아서 아직은 페키지가 많이 분리되는게 어색(?)한 감이 있네요.😋 제이슨이 DDD를 고려해서 만든 과제제로 알고 있는데 그 측면에서 럿고가 하신 방향이 맞을 것 같네요.

이제보니 56번째 줄에 있는 savedTableGroup.setOrderTables(savedOrderTables);에서 orderTables을 테이블 그룹을 생성할 때 필요한 형태(empty true)로 초기화 해 줄 수 있을 것 같네요.

혹시... 좀 더 힌트를 주실수 있을까요??

setOrderTables을 사용하고 있는 곳이 여기밖에는 없는걸로 보여요. 그러면 setter로 사용하지 말고 테이블 그룹을 만든다는 측면에서
현실에서 테이블 그룹을 만들때 그룹이 될 테이블을 치우고 준비할테니 이에 맞춰 setter가 아닌 init(간간히 사용하는데 탁탁치는 않네요. 참고만하고 좀더 좋은 네이밍이 있다면 사용하시면 될 것 같아요)

public class TableGroupService {
    ...
    public TableGroupResponse create(final TableGroupRequest tableGroupRequest) {
        ...
        savedTableGroup.init(savedOrderTables);

        for (final OrderTable savedOrderTable : savedOrderTables) {
            orderTableRepository.save(savedOrderTable);
        }
    }
}

public class TableGroup {
    public void setOrderTables(final List<OrderTable> orderTables) {
        validateByOrderTables(orderTables);
        for (final OrderTable orderTable : orderTables) {
            orderTable .clean();
        }
        this.orderTables = orderTables;
    }
}

과 같이 정리할 수 있을것 같아요.

혹은

        for (final OrderTable savedOrderTable : savedOrderTables) {
            savedOrderTable.clean();
            orderTableRepository.save(savedOrderTable);
        }

제가 너무 생략을 해서 리뷰를 드린것 같은데 changeEmpty자체가 setter의 이름만 바꾼 형태로 보여 드린 피드백이였습니다. 보통 다른 타입은 실제 setter와 큰 차이가 없어도 이름만 잘지어줘도 큰의미를 가지는데 boolean은 setter의 느낌을 지우기 힘들더라고요.

ex) changeName("name"), changeEmpty(true)
전자는 이름을 바꾼다는 느낌이 강하지만 후자는 setter의 느낌이 나죠.

메서드 안에 책임이 더 잘 되어 잇는 것 같아서 좋은거 같아요!
근데 개인저긍로 애매했던 부분은 orderTable은 true가 되기도 하고 false로 변경해야 하는 경우가 있는데 clean으로 하게 되면 false를 true로 변경하는 역할만 잇는거 같아서 고민이네요.(각각의 메서드를 만들어 주는것이 더 좋은지가 잘 모르겟어요 ㅠㅠ )
혹시 이 부분에 대해서는 어떻게 생각하시는지가 궁금합니다. ㅠㅠ