-
Notifications
You must be signed in to change notification settings - Fork 10
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
[STEP2-1] 리다이렉트 기능 개발 및 코드베이스 리팩토링 #61
base: Sung-Heon
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리팩토링에서 의도하지 않은 성능 오류가 발생하면 신규 기능도 롤백 해야 되나요?
@PostMapping("/admin/shorten-urls") | ||
public ResponseEntity<String> postShortenUrls(@RequestBody PostShortenUrlsRequest request) { | ||
return ResponseEntity.ok(adminUrlShortenService.addToShortenUrls(request)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CreateShortenUrlRequest <-> PostShortenUrlsRequest
매우 비슷한 역할을 하는 request dto 인데 prefix 가 다릅니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네. 비슷한 역할을 하지만 맥락이 다르다고 생각했습니다.
- create는 없던걸 새로이 만드는거고
2)post는 기존에 있었던거를 migration으로 수동으로 밀어넣는거라고 생각했습니다.
migration이라고 하기에는 먼가 이미 migration 끝단계에 해당하는 거라 post가 나을거라고 생각했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전 post 가 POST 메서드를 뜻하는 것이라고 생각했는데, after 의 뜻이었군요.
- postShortenUrls (afterShortenUrls)
- 전 여전히 어색하다고 느끼지만 운영자의 의도가 있음으로 넘어가겠습니다.
실무에서도 이렇듯 모호한 네이밍이 사용되는 경우가 있습니다.
- 이 경우 충분한 설명이나 주석을 권장 합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 생각하신 post맞구요.
직관적이지 않은 네이밍같아서 수정했습니다.
private final AdminUrlShortenService adminUrlShortenService; | ||
|
||
@GetMapping("/admin/shorten-urls") | ||
public ResponseEntity<HashMapResponse> getAllShortenUrls() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming 에 자료구조를 명시한다면 dto 를 감쌀 이유가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 수정했습니다.
이정도 수준의 pr에서는 pr 단위안에서 수정하면 될것 같아서요. 좀더 대규모라면 리팩토링과 신규기능을 분리할필요는 있겠습니다. |
주요 변경 내용
URL 리다이렉트 기능 개발
ResponseEntity 적용
관리자 기능 및 일반 사용자 기능 분리
패키지 구조 정리