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

전남대 BE_지연우 4주차 과제(step1,2,3) #180

Open
wants to merge 23 commits into
base: speciling
Choose a base branch
from

Conversation

speciling
Copy link

@speciling speciling commented Jul 17, 2024

리뷰 잘부탁드립니다!

@speciling speciling changed the base branch from main to speciling July 17, 2024 10:17
@speciling speciling changed the title 전남대 BE_지연우 4주차 과제(step2) 전남대 BE_지연우 4주차 과제(step1) Jul 18, 2024
@speciling speciling changed the title 전남대 BE_지연우 4주차 과제(step1) 전남대 BE_지연우 4주차 과제(step1,2) Jul 20, 2024
@speciling speciling changed the title 전남대 BE_지연우 4주차 과제(step1,2) 전남대 BE_지연우 4주차 과제(step1,2,3) Jul 21, 2024
Copy link

@seungy0 seungy0 left a comment

Choose a reason for hiding this comment

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

연우님 고생하셨습니다~
몇가지 코멘트 남겼으니 참고해주세요.

+) xxForm, xxParam 보다는 xxRequest, xxResponse 사용을 제안드립니다.
추가적으로 궁금하신점 있으면 멘토링 신청 또는 댓글 작성해주세요.

Comment on lines +49 to +52
public void changeCategoryName(@RequestBody CategoryForm categoryForm, @PathVariable Long id) {
categoryForm.setId(id);
categoryService.updateCategory(categoryForm);
}
Copy link

Choose a reason for hiding this comment

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

Suggested change
public void changeCategoryName(@RequestBody CategoryForm categoryForm, @PathVariable Long id) {
categoryForm.setId(id);
categoryService.updateCategory(categoryForm);
}
public void changeCategoryName(@RequestBody CategoryForm categoryForm, @PathVariable Long id) {
categoryService.updateCategory(id, categoryForm);
}

이런 방식은 어떨까요?

@NotBlank
private String name;

@JsonCreator
Copy link

Choose a reason for hiding this comment

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

JsonCreator는 필요하지 않습니다~

Comment on lines +11 to +14
public CategoryParam(Category category) {
this.id = category.getId();
this.name = category.getName();
}
Copy link

Choose a reason for hiding this comment

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

정적 팩토리 메서드 사용을 추천드립니다.

.orElseThrow(CategoryNotFoundException::new);
}

public void updateCategory(CategoryForm categoryForm) {
Copy link

Choose a reason for hiding this comment

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

@Transactional을 사용하는게 좋겠습니다.

Copy link

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.

옵션만을 조회하는 경우는 없을까요?
개발자 도구를 사용해 실제 카카오 선물하기에서 API를 어떻게 구성하고 있는지 살펴보시는 것도 좋겠습니다.

Comment on lines +40 to +41
@OneToMany(mappedBy = "product", cascade = CascadeType.ALL)
private List<Option> options = new ArrayList<>();
Copy link

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.

트랜잭션을 걸지 않으면 예상과는 다른 결과가 나올 수 있습니다.
일부에는 걸고, 일부에는 걸지 않은 이유가 궁금합니다.

Comment on lines +64 to +68
private void validateDuplicatedName(Long productId, OptionForm optionForm) {
if (optionRepository.existsByProductIdAndName(productId, optionForm.getName())) {
throw new IllegalArgumentException("옵션 이름은 중복될 수 없습니다.");
}
}
Copy link

Choose a reason for hiding this comment

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

db에서 unique 키를 거는 방법도 있습니다.
현재와 같이 구성하면 validation 이후에 option을 수정하기 전에 같은 이름의 옵션이 추가될 수 있습니다.

@Service
public class CategoryService {

private final JpaCategoryRepository categoryRepository;
Copy link

Choose a reason for hiding this comment

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

Suggested change
private final JpaCategoryRepository categoryRepository;
private final CategoryRepository categoryRepository;

이렇게 작성해도 잘 작동되게 해주세요~
스프링의 핵신 기능 DI/IoC를 올바르게 사용하지 않고 계세요.

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.

2 participants