-
Notifications
You must be signed in to change notification settings - Fork 106
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
[5기 최정은] Shorten-URL 과제 제출합니다. #68
base: JeongeunChoi
Are you sure you want to change the base?
[5기 최정은] Shorten-URL 과제 제출합니다. #68
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.
전반적으로 요구사항 잘 구현해주신 것 같습니다.
작성해주신 부분들 중 코멘트 몇 개 남겨두었으니 확인 부탁드리겠습니다.
- 테스트시 별도의 yml 파일을 설정해주지 않아 프로덕션과 동일한 yml을 사용할 것 같아요.
- 요구사항에 대한 테스트가 충분하지 않은 것 같아요.
- 레이아웃이 조금 틀어진 부분이 있었는데 수정해주시면 좋을 것 같아요.
과제 요구사항을 하나씩 구현하다가 "단축된 URL에 대한 요청 수 정보저장"은 어떤 경우에 활용되는지 의문이 들었습니다! 이번 과제에서는 요청 수를 저장하긴 하지만, 사용하는 일이 없는데 보통 요청 수는 어떻게 활용되는지 궁금합니다!
- 지표로서 활용되는 경우가 많을 것 같습니다. 현재는 요청 수만 받지만 나중을 고려한다면 디바이스 정보라던지, 요청 IP 정보 등 다양한 정보를 함께 수집하여 이를 내부적으로 활용할 수 있을 것 같습니다.
id 값을 1부터 생성되게 하니, 인코딩된 값이 짧다고 느껴져 id 값이 10000으로 시작되도록 하였는데, 일반적인 방법인지 궁금합니다!
- 짧다고 생각되면 패딩 처리도 고려해볼 수 있을 것 같습니다. id 값 자체를 변조하는 건 일반적인 방법은 아니라고 생각합니다.
서비스 테스트 코드를 작성 중, 레포지토리를 Mocking 하기 위해 @mock와 @MockBean 두가지 중 어떤 것을 사용해야할까 의문이 들어 찾아보았습니다. @mock은 스프링 컨텍스트와 관련이 없고 @MockBean은 스프링 컨텍스트에 생성된다.라고 이해하여 컨트롤러 테스트에서는 @MockBean을 서비스 테스트에서는 @mock을 사용하였는데 적절히 사용된 것인지 궁금합니다!
- 네 잘 설명해주신 것 같습니다. 컨트롤러에서는 WebMvcTest을 통해 어플리케이션 컨텍스트 내부에 빈으로 필요한 의존성들을 주입해줘야하기에 사용해주시는게 맞고 서비스에서는 별도로 어플리케이션 컨텍스트를 활용하지 않기때문에 필요없어 mock을 사용해주시는게 적절한 것 같습니다.
서비스 테스트를 단위테스트로 작성해보았는데, Base62Util의 경우는 직접적인 사용이 필요하다고 판단하여 @InjectMock를 통해 주입해주었는데 단위테스트에 어긋난건 아닌지 궁금합니다! Base62Util에 대한 테스트를 따로 생성하는게 일반적인가요?
- InjectMock과 Mock의 차이가 단위테스트와는 직접적으로 연관이 있지 않은 것 같습니다.
InjectMock과 Mock의 차이점을 조금 더 학습해보시면 좋을 것 같습니다.
@Test | ||
void contextLoads() { | ||
} | ||
|
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.
불필요한 공백이 존재하네요.
|
||
private static final String ORIGIN_URL_PATTERN = "^(https?://).*"; | ||
@Id | ||
@GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "indexGenerator") |
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.
Mysql로 사용하는데 SEQUENCE 전략으로 설정하신 이유가 있나요?
@Component | ||
public class Base62Util { |
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.
- 왜 Base64가 아닌 Base62를 선택했나요?
- 유틸성 클래스가 꼭 스프링 빈으로 등록되어 관리되어야할까요?
- Util성 클래스임에도 Base62로 인코딩/디코딩을 진행할때 long 값만 받을 수 있는 것 같아요. 관련해서 고민해봐주시면 좋을 것 같아요.
- 여러개의 알고리즘을 지원해야한다면 어떻게 구현해볼 수 있을까요?
public class Base62Util { | ||
|
||
private final String BASE62_CHARS = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; | ||
private final int BASE62_CHARS_SIZE = 62; |
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.
BASE62_CHARS_SIZE와 BASE62_CHARS.length()가 별도로 관리되고 있는 것으로 보이는데 하나로 관리하는 걸 고려해봐주세요.
- 임의로 BASE62_CHARS가 변경되었을때 길이는 62로 고정되어있을 것 같아요.
private final int BASE62_CHARS_SIZE = 62; | ||
|
||
public String encoding(long urlId) { | ||
StringBuffer sb = new StringBuffer(); |
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.
StringBuilder, StringBuffer의 선택지가 있었을 것 같은데 StringBuffer를 사용한 이유는 뭔가요?
|
||
@InjectMocks | ||
private UrlService urlService; | ||
@InjectMocks |
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.
InjectMocks를 사용하는게 맞을까요?
|
||
@Test | ||
@DisplayName("기존 url로 리다이렉트 성공") | ||
void Success_RedirectToOriginUrl() throws Exception { |
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.
메서드명이 대문자로 되어있어요.
@BeforeEach | ||
void setUp() { | ||
urlService = new UrlService(urlRepository, base62Util); | ||
} |
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.
테스트마다 초기화되어야하는 대상일까요?
@Column(name = "originUrl") | ||
private String originUrl; |
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.
originUrl는 유니크가 보장되지 않아도 되는건가요? 여러개의 originUrl에 대한 값이 생길 수 있을 것 같네요.
@NoArgsConstructor | ||
@Getter | ||
@Slf4j | ||
public class Url { |
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.
생성/수정 일시는 따로 필요없을까요?
📌 과제 설명
👩💻 요구 사항과 구현 내용
controller
service
domain
exception
repository
Base62Util
UI
단축할 URL 입력 후 단축 요청
단축된 URL 확인
단축된 URL로 접근 시, 원래 URL로 리다이렉트
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점