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

Feature/admin concert place #2

Merged
merged 10 commits into from
Oct 2, 2024
Merged

Feature/admin concert place #2

merged 10 commits into from
Oct 2, 2024

Conversation

plere
Copy link
Collaborator

@plere plere commented Sep 21, 2024

개요

관리자 관련 장소 API 개발

상세정보

  • 새로운 장소 등록 API
  • 장소 조회 API 개발

Copy link

@jinspark-lab jinspark-lab left a comment

Choose a reason for hiding this comment

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

전 괜찮아보입니다..! 이전 버전을 접근하는 use case 는 없을까요?

public ResponseEntity<ResponseDto<String>> customException(CustomException exception) {
ResponseDto<String> responseDto = new ResponseDto<>(exception.getStatus().toString(),
exception.getMessage());
log.warn("CustomRuntimeException 발생 : code: {}, message: {}", responseDto.getCode(), responseDto.getBody());

Choose a reason for hiding this comment

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

error 로 찍어도 괜찮을 것 같습니다.

public ResponseEntity<ResponseDto<String>> customException(MethodArgumentNotValidException exception) {
HttpStatus badRequest = HttpStatus.BAD_REQUEST;
ResponseDto<String> responseDto = new ResponseDto<>(badRequest.toString(), "요청 값이 잘 못 됐습니다");
log.warn("MethodArgumentNotValidException 발생 : message: {}", exception.getMessage());

Choose a reason for hiding this comment

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

같은 피드백입니다.

public class Place {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;

Choose a reason for hiding this comment

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

기존 장소에 대한 수정이 들어가는 경우 Update 로 하실 거인거죠?

})
@DynamicInsert
@EntityListeners(AuditingEntityListener.class)
public class PlaceVersion {

Choose a reason for hiding this comment

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

버전을 별도 엔티티로 관리하시려는 이유가 있으실까요?

Copy link

@jinspark-lab jinspark-lab left a comment

Choose a reason for hiding this comment

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

피드백드렸습니다.

.address(address)
.identifier(identifier)
.version(beforePlace.getNextVersion())
.last(true)

Choose a reason for hiding this comment

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

get 에서 last 를 셋팅하는 write operation 이 같이 있는데요. 분리하면 좋을 것 같습니다.
함수는 getNextPlace 로 DB 변경이 없을 것이 기대되지만 실재로는 beforePlace 의 last 가 Update 되는 함수라 안정적이지 않아보입니다.
차라리 last 를 별도 루틴에서 셋팅하게해서 read / write 를 분리하는게 좋을것 같습니다.

public Place getLastVersion(String identifier) {
return placeRepository.findTopByIdentifierOrderByVersionDesc(identifier)
.orElseThrow(() -> new ResourceNotFoundException("place", identifier));
}

Choose a reason for hiding this comment

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

사소한 부분이지만 나중에는 Service 레벨에서는 Entity 를 직접 다루기 보다 VO 를 사용하는 연습해보시면 좋을것 같습니다.

Copy link

sonarcloud bot commented Sep 30, 2024

@plere plere merged commit 083f07d into main Oct 2, 2024
2 checks passed
@plere plere deleted the feature/admin-concert-place branch October 2, 2024 10:30
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