[5기 박유진] Shorten-URL 과제 제출합니다. #70
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
SangHyunGil
reviewed
Dec 18, 2023
There was a problem hiding this comment.
작성해주시느라 너무 고생많으셨어요.
간단한 코멘트 남겼으니 확인부탁해요.
- 테스트 코드가 없는 것 같은데요. 적어도 도메인 로직과 서비스 로직과 같은 중요한 비즈니스 로직을 다루는 영역에 대한 테스트는 작성하는걸 권장해요.
- 레이아웃이 조금 틀어진 부분이 있었는데 수정해주시면 좋을 것 같아요.
- 라이브러리를 다룰때 단순 인코딩이라면 성능적인 부분을 고려하기 위해서는 부하테스트 같은걸 통해서 검증을 해야할 것 같아 추후에 한 번 관련하여 진행해보시면 좋을 것 같아요.
pom.xml
Outdated
Comment on lines
20
to
23
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-data-redis</artifactId> | ||
| </dependency> |
src/main/resources/application.yml
Outdated
Comment on lines
26
to
30
| servlet: | ||
| encoding: | ||
| charset: UTF-8 | ||
| enabled: true | ||
| force: true |
|
|
||
| @NonNull | ||
| @Column(name = "orgin_url") | ||
| private String originUrl; |
There was a problem hiding this comment.
originUrl은 유니크하지 않아도 되는건가요?
만약, 유니크해야한다면 유니크 제약 조건을 걸어보면 좋을 것 같네요.
| @Getter | ||
| @Entity | ||
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
| public class Url { |
|
|
||
| import java.util.Optional; | ||
|
|
||
| public interface UrlRepository extends JpaRepository<Url, String> { |
| .originUrl(originUrl).build()); | ||
| } | ||
|
|
||
| public static String shortenUrlByMurMur(@NonNull Url url) { |
There was a problem hiding this comment.
인코딩 관련 책임들이 여러곳에 분산되어있는 것 같아요.
- 실제 인코딩하는 로직은 서비스에 존재해요.
- 인코딩 관련 관리는 EncodingType을 통해서 관리해요.
- 이넘을 사용해 변환하는건 EnumConverter을 통해 처리해요.
코드를 응집도 있게 관리하는 부분을 고민해보시면 좋을 것 같아요.
인코딩 수행하는 로직은 Url 서비스가 가져야할 책임이 아닌 것 같아요. 별도의 인코딩 로직만 수행하는 객체가 존재(별도 클래스 혹은 이넘)하고 UrlService는 이를 호출만하면 될 것 같아요.
서비스 레이어에서 하는 역할이 무엇일지 고민해봐주시면 더욱 좋을 것 같아요.
| } | ||
|
|
||
| public static String shortenUrlByMurMur(@NonNull Url url) { | ||
| return Hashing.murmur3_32_fixed().hashString(url.getOriginUrl(), Charset.defaultCharset()).toString(); |
| if (isValidUrl(originUrl)) { | ||
| this.originUrl = originUrl; | ||
| } else { | ||
| throw new IllegalArgumentException("Invalid URL: " + originUrl); |
There was a problem hiding this comment.
간단의견) 나중에는 CustomException을 적용해봐도 좋을 것 같아요.
| public String getUrlByKey(@NonNull String key) { | ||
| Url url = urlRepository.findByShortenKey(key) | ||
| .orElseThrow(UrlNotFoundException::new); | ||
| url.updateCount(); |
There was a problem hiding this comment.
동시에 요청이 들어오면 동시성 이슈가 생길 수 있을 것 같아요.
어떻게 처리할지 고민하고 생각을 공유해주세요.
| @@ -0,0 +1,27 @@ | |||
| package com.prgrms.shortenurl.service; | |||
|
|
|||
| public class Base62Encoding { | |||
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.
📌 과제 설명
domain
service
controller
👩💻 요구 사항과 구현 내용
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점