-
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 과제 제출합니다. #73
base: letskuku
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.
구현시느라 고생많으셨어요 👍
간단하게 코멘트 남겼으니 확인해주세요!
@EntityListeners(AuditingEntityListener.class) | ||
@Getter | ||
@MappedSuperclass | ||
public class BaseEntity { |
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.
abstract class로 표현 가능할 것 같아요.
@@ -0,0 +1 @@ | |||
|
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 설정이 없는건가요?
|
||
Url url = urlRepository.findByShortUrl(shortUrl) | ||
.orElseThrow(() -> new UrlException(UrlErrorCode.URL_NOT_FOUND, shortUrl)); | ||
url.increaseRequestCount(); |
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.
조회는 빈번하게 일어남으로 동시에 요청이 들어오면 업데이트시 동시성 이슈가 발생할 수 있을 것 같아요.
어떻게 해결할 수 있을 것 같은지 간단한 생각 남겨봐주시면 좋을 것 같아요.
Url url = urlRepository.save(createShortUrlRequest.toUrl()); | ||
|
||
EncodeType encodeType = EncodeType.getEncodeTypeByName(createShortUrlRequest.getEncodeType()); | ||
ShortUrlEncoder shortUrlEncoder = ShortUrlEncoderMapper.createShortUrlEncoder(encodeType); | ||
|
||
String shortUrl = shortUrlEncoder.createShortUrl(url.getId()); | ||
url.updateShortUrl(shortUrl); |
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.
Url 생성 save -> shorUrl 업데이트 흐름보다는 Url + shortUrl 생성 후 저장을 한 번만 하는게 더욱 인지하기 쉬울 것 같다는 생각이 들었어요.
|
||
public static EncodeType getEncodeTypeByName(String name) { | ||
return Arrays.stream(EncodeType.values()) | ||
.filter(encodeType -> encodeType.getName().equalsIgnoreCase(name)) |
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.
케이스 고려 좋네요 👍
이런 부분이 테스트에 녹여져도 좋은 것 같아요.
BASE58, | ||
BASE62; |
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 / BASE58 중 BASE64는 고려하시지 않았나요?
Url url = new Url(createShortUrlRequest.getOriginalUrl()); | ||
Long urlId = 1234567L; | ||
|
||
Field id = url.getClass().getDeclaredField("id"); | ||
id.setAccessible(true); | ||
id.set(url, urlId); |
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.
테스트코드에서 위 부분을 중복해서 사용하고 다른 테스트 코드에서도 활용할 수 있기에 Fixture로 별도로 분리해봐도 좋을 것 같아요.
public class CreateShortUrlRequest { | ||
|
||
private final String originalUrl; | ||
private final String encodeType; |
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.
클라이언트에게 인코딩 타입을 입력받는게 일반적인 유스케이스는 아닐 것 같다는 생각도 들어요.
동적으로 변경하기 위해서 클라이언트에게 입력받기보다 동적으로 값을 가져와 이를 적용할 수 있는 방법을 고려해보면 좋을 것 같네요.
public class EncodeTypeTest { | ||
|
||
@Test | ||
@DisplayName("잘못된 EncodeType 입력은 예외를 발생시킨다.") |
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.
이미 작성해주셨군요 👍
import static org.mockito.Mockito.when; | ||
|
||
@ExtendWith(MockitoExtension.class) | ||
public class UrlServiceTest { |
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.
조회시 카운트가 1 증가하는걸 테스트해봐도 좋았을 것 같네요.
import lombok.RequiredArgsConstructor; | ||
import org.springframework.http.HttpStatus; | ||
import org.springframework.http.ResponseEntity; | ||
import org.springframework.web.bind.annotation.*; |
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.
와일드 카드를 사용하지 않는 방향으로 해보시면 좋을 것 같아요.
📌 과제 설명
👩💻 요구 사항과 구현 내용
domain
global.common
repository
application
presentation
dto
exception
encoder
mapper
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점