[4기 유원우] Shorten URL 과제 PR입니다. #51
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- SpringBoot 프로젝트 생성 - .gitignore 설정
- 테스트를 통과하지만 서비스 추가시 실패 예정 - 서비스 동작을 통해 성공하도록 구현
- request에서 간략한 검증하도록 수정 - response 반환시 json으로만 받을 수 있게 강제
- DefaultUrlShortenerService에 UrlLink를 생성하는 로직 구현 - UrlLink 도메인은 원본 Url과 Hash 값을 저장
- 잘못된 id 값이 들어올 경우 Exception 발생하도록 수정
- Hash 값을 가진다는 것을 명확하게 표현하기 위해 이름 변경
- Url에 대한 Hash를 생성하고 Repository를 통해 존재 여부를 확인하여 유일성 보장
- yaml 형식으로 작성된 내용을 properties 형식으로 수정
- `LocalUrlLinkRepository` 클래스를 통해 `UrlLink` 객체를 Local Memory에서 저장하고 관리 - `UrlLink` 객체를 관리하는 클래스명을 명확하게 표현하도록 수정
- UrlShortenerApiController와 UrlShortenerService에서 수행하는 작업이 유사하나, 함수명이 다르므로 동일한 작업을 수행하는 것처럼 보이지 않았음 두 클래스에서의 작업이 유사함을 명확히 나타내기 위해 create 함수명을 통일함
- URL 단축 생성 로직에서, 이미 단축된 URL이 존재하는 경우 해당 URL을 반환하도록 로직 수정 - 이 변경으로 인해 불필요한 리소스 사용을 줄이고, 동일한 원본 URL에 대해 항상 동일한 단축 URL을 제공함
- 중복된 테스트 클래스 제거(UrlShortenerApiControllerMockTest) - 함수명 given_when_then 사용 - private으로 선언되지 않은 필드 private 선언
- get은 못 찾을 시 에러가 발생해야 한다고 생각하지만 현재 로직은 발생하지 않는다. find가 존재하지 않더라도 오류가 발생하지 않는다고 생각하여 변경
- ShortUrl을 받아 원본 URL로 리다이렉트하는 로직 구현 - 사용자가 ShortUrl을 접근할 때, 해당 ShortUrl에 매핑된 원본 URL로 브라우저가 리다이렉트됨
- `UrlLinkRepository`에서 `UrlHash`를 사용하여 `UrlLink`를 조회한 후, 원본 URL을 반환하도록 로직 수정(이전에는 `UrlHash` 값을 반환하였으나, 이제는 원본 Url 값을 반환하도록 변경)
devYSK
approved these changes
Oct 10, 2023
Comment on lines
+7
to
+10
|
|
||
| private static final UrlValidator URL_VALIDATOR = new UrlValidator(); | ||
| public static final int URL_MAX_LENGTH = 2_000; | ||
|
|
Comment on lines
+11
to
+20
| public Url { | ||
| if (!URL_VALIDATOR.isValid(value)) { | ||
| throw new IllegalArgumentException("잘못된 URL 주소입니다. Current URL: " + value); | ||
| } | ||
|
|
||
| if (StringUtils.length(value) > URL_MAX_LENGTH) { | ||
| throw new IllegalArgumentException( | ||
| String.format("URL의 길이는 %d를 넘어갈 수 없습니다 Current Length: %d", URL_MAX_LENGTH, value.length())); | ||
| } | ||
| } |
There was a problem hiding this comment.
정적 팩토리 메소드로 URL_VALIDATOR를 주입받아 검증하면서 생성한다면 다양한 검증 시나리오 테스트에 용이해질것 같아요
Comment on lines
+18
to
+19
| } | ||
| if (StringUtils.length(value) > MAX_LENGTH) { |
| throw new IllegalArgumentException("ID는 0보다 크고 null이 아니어야 합니다. Input ID: " + newId); | ||
| } | ||
| if (this.id != null) { | ||
| throw new IllegalStateException("ID가 이미 할당되어 있습니다. Current ID: " + newId); |
Comment on lines
+58
to
+62
|
|
||
| private long generateNewId() { | ||
| return idGenerator.getAndIncrement(); | ||
| } | ||
|
|
There was a problem hiding this comment.
이 메소드는 LocalUrlLinkRepository의 책임에 맞지 않는것 같아요
idGenerator가 generateNewId를 해주면 안될까요?
AtomicLong란걸 알 필요는 없는것 같아요
| } | ||
| } | ||
|
|
||
| throw new UniqueHashCreationException("유니크한 URL 해시를 생성하는 데 실패했습니다."); |
There was a problem hiding this comment.
유니크한 URL 해시를 생성하는 데 실패했단거는 여기서는 사실 서버쪽의 잘못이라고 볼 수 있어요.
사용자의 잘못이 아니라면, 우리 서버가 잘못한 이부분을 사용자한테 보여줄 필요가 있을까 고민해보면 좋을것 같아요.
재시도 하면 성공할까요?
| import com.prgrms.wonu606.shorturl.domain.UrlHash; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| /** |
Comment on lines
+28
to
+30
|
|
||
| @Test | ||
| void whenShortUrlExists_thenRedirect() throws Exception { |
Comment on lines
+30
to
+35
| @Autowired | ||
| MockMvc mockMvc; | ||
|
|
||
| @Autowired | ||
| ObjectMapper objectMapper; | ||
|
|
There was a problem hiding this comment.
이 친구들은 왜 default로 하셨는지 궁금합니다.
통일성을 맞춰보면 어떨까여
Comment on lines
+22
to
+40
| @Override | ||
| public Long save(UrlLink urlLink) { | ||
| validateUrlLink(urlLink); | ||
|
|
||
| Long newId = generateNewId(); | ||
|
|
||
| urlLink.initializeId(newId); | ||
|
|
||
| storage.put(newId, urlLink); | ||
|
|
||
| hashedUrlIndex.compute(urlLink.getUrlHash(), (key, existingUrlLink) -> { | ||
| if (existingUrlLink != null) { | ||
| throw new IllegalArgumentException("UrlHash가 이미 인덱싱 되어 있습니다. Current UrlHash: " + urlLink.getUrlHash()); | ||
| } | ||
| return urlLink; | ||
| }); | ||
|
|
||
| return newId; | ||
| } |
There was a problem hiding this comment.
UrlLink가 밖에서 생성 되어서 아쉽게도 initializeId()라는 메소드를 만들었네요 .
@Override
public UrlLink save(UrlLink urlLinkToSave) {
Long newId = generateNewId();
UrlLink newUrlLink = new UrlLink(newId, urlLinkToSave.getOriginalUrl(), urlLinkToSave.getUrlHash());
storage.put(newId, newUrlLink);
hashedUrlIndex.compute(newUrlLink.getUrlHash(), (key, existingUrlLink) -> {
if (existingUrlLink != null) {
throw new IllegalArgumentException("UrlHash가 이미 인덱싱 되어 있습니다. Current UrlHash: " + newUrlLink.getUrlHash());
}
return newUrlLink;
});
return newUrlLink;
}이렇게 하면 객체의 불변성을 유지하면서 필요한 정보를 캡슐화할 수 있어요
그러나 몇가지 주의해야해요
- 메모리 사용량 증가
- 추가적인 생성자 관리 방법
- 객체 동일성
WooSungHwan
approved these changes
Oct 15, 2023
| ) | ||
| public interface UrlShortenerApiResponseMapper { | ||
|
|
||
| @Mapping(target = "shortenUrl", expression = "java(baseUrl + result.hashedShortUrl())") |
There was a problem hiding this comment.
추후 미션으로 expression = "java()" 를 활용하지 않는 방법은 없을까를 고민해보시길 ㅎ
Comment on lines
+4
to
+8
|
|
||
| public record UrlHash( | ||
| String value | ||
| ) { | ||
|
|
There was a problem hiding this comment.
hash라는 방식을 활용하지만 굳이 클래스명, 변수명에 적을 필요는 없을듯. (내 알빠는 아니고 무언가 담겨있어. 난 열쇠는 모르지만 열쇠를 아는 놈이 알아서 열든지 말든지 하겄지.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📌 과제 설명
Local Memory에 저장하는 URL Shortener
2023-10-10.5.09.52.mov
👩💻 요구 사항과 구현 내용
✅ PR 포인트 & 궁금한 점
저는 Url이 이미 hash값으로 생성되어 있다면 재사용하고 생성되지 않았다면 생성하려고 하였습니다.
그러다 보니
UrlShortenerApiController.findOrCreateShortenedUrl와DefaultUrlShortenerService .findOrCreateShortenUrlHash같이 2가지 기능을 하는 로직이 생겨났습니다.이 로직들을 어떻게 분리하면 좋을지 궁금합니다.