2단계 - 서비스 리팩터링 #81
2단계 - 서비스 리팩터링 #81
Conversation
|
|
||
| import kitchenpos.menu.domain.Menu; | ||
|
|
||
| public interface MenuDao extends JpaRepository<Menu, Long> { |
There was a problem hiding this comment.
JPA를 사용하셨네요 👍
이제 Dao가 아니라 Repository로 이름이 변경되어야 할 것 같네요 :)
| Menu save(Menu entity); | ||
|
|
||
| Optional<Menu> findById(Long id); | ||
|
|
||
| List<Menu> findAll(); |
There was a problem hiding this comment.
기본적으로 제공되어 선언하지 않아도 사용할 수 있는 메서드와 아래 countByIdIn() 처럼 선언해야 사용할 수 있는 메서드가 있어요. 한번 JpaRepository와 그 상위 클래스들을 확인해보세요! :)
| @Service | ||
| public class MenuService { | ||
| private final MenuDao menuDao; | ||
| private final MenuGroupDao menuGroupDao; |
| BigDecimal sum = BigDecimal.ZERO; | ||
| for (MenuProduct menuProduct : menuProducts) { | ||
| final Product product = productDao.findById(menuProduct.getProductId()).orElseThrow(IllegalArgumentException::new); | ||
| sum = sum.add(product.getPrice().multiply(BigDecimal.valueOf(menuProduct.getQuantity()))); | ||
| savedMenu.addMenuProduct(new MenuProduct(savedMenu, product, menuProduct.getQuantity())); | ||
| } | ||
| savedMenu.validateSumOfPrice(sum); |
There was a problem hiding this comment.
하나씩 다 더한 후에 외부에서 총 합계에 대한 validation을 하는게 이상해보이는데요.
새로운 MenuProduct list를 만들어서 한번에 넣고 내부에서 sum을 만들어서 validation을 하면 어떨까요? :)
| * @description : | ||
| **/ | ||
| @DisplayName("메뉴그룹") | ||
| class MenuGroupServiceTest extends IntegrationTest { |
There was a problem hiding this comment.
통합테스트를 잘 작성해주셨네요 👍
한가지 첨언을 드리자면, 각 테스트에서 사용한 테스트 fixture에 대한 clean 작업이 필요할 것 같아요 ㅎㅎ
제가 사용하는 통합테스트 포맷을 한번 남겨드릴테니, 참고해서 수정해봐주세요 :)
@SpringBootTest
public class LineServiceTest {
@Autowired
private LineService lineService;
@Autowired
private LineRepository lineRepository;
@Autowired
private SectionRepository sectionRepository;
@Autowired
private StationRepository stationRepository;
@AfterEach
public void cleanup() {
sectionRepository.deleteAllInBatch();
lineRepository.deleteAllInBatch();
stationRepository.deleteAllInBatch();
}
@DisplayName("Line에 구간을 등록한다")
@Test
void addSection() {
// given
Line line = new Line("2호선", "green");
lineRepository.save(line);
// 이런 식으로 테스트에 필요한 데이터 생성 후 저장
// when
// lineService 행위
// then
// 행위에 대한 결과 검증
}
}| public void setId(final Long id) { | ||
| this.id = id; | ||
| } | ||
|
|
||
| public String getName() { | ||
| return name; | ||
| } | ||
|
|
||
| public void setName(final String name) { | ||
| this.name = name; | ||
| } |
| return Objects.equals(getId(), menu.getId()) && Objects.equals(getName(), menu.getName()) | ||
| && Objects.equals(getPrice(), menu.getPrice()) && Objects.equals(getMenuGroupId(), | ||
| menu.getMenuGroupId()) && Objects.equals(getMenuProducts(), menu.getMenuProducts()); |
There was a problem hiding this comment.
저는 Entity의 동등성 비교는 식별자인 id 기반으로 이루어져야한다고 생각하는데 어떻게 생각하시나요? :)
There was a problem hiding this comment.
저도 동의합니다! getter을 만들고 값 비교를 하면서 무의식적으로 생성한 코드인데, 의식적인 프로그래밍이 필요한 이유를 여기서 다시 느낍니다 😢
|
|
||
| } | ||
|
|
||
| @org.springframework.transaction.annotation.Transactional(readOnly = true) |
| public class OrderLineItem { | ||
| @Id | ||
| @GeneratedValue(strategy = GenerationType.IDENTITY) | ||
| private Long seq; |
There was a problem hiding this comment.
이름을 seq라고 붙이신 이유가 있을까요? 관용적으로 id라고 명명하긴 합니다 :)
There was a problem hiding this comment.
저도 id가 익숙하게 느껴집니다!
개선하였습니다!
안녕하세요 우빈님! 리뷰요청드릴게요.
DM으로 같이 얘기했던 부분에 대해서 고민을 많이 했었고, 덕분에 ReflectionTestUtils을 사용했습니다.
수정할 부분이 많을것같은데 구조적인 변경이 필요하거나 내용이 너무 많다면 DM으로 주셔도 됩니다!
감사합니다 🙇