Skip to content
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

Open
wants to merge 24 commits into
base: letskuku
Choose a base branch
from

Conversation

letskuku
Copy link

@letskuku letskuku commented Dec 17, 2023

📌 과제 설명

  • URL 입력폼 제공 및 결과 출력
  • URL Shortening Key는 8 Character 이내로 생성
  • 단축된 URL 요청시 원래 URL로 리다이렉트
  • 단축된 URL에 대한 요청 수 정보저장 (optional)
  • Shortening Key를 생성하는 알고리즘 2개 이상 제공하며 애플리케이션 실행중 동적으로 변경 가능 (optional)

👩‍💻 요구 사항과 구현 내용

image
image

domain

  • originalUrl, shortUrl, shortUrl 요청 수 정보를 저장하는 Url
  • 인코딩 알고리즘을 구분하는 EncodeType

global.common

  • 생성시각, 수정시각 정보를 저장하는 BaseEntity

repository

  • Url 엔티티의 수정/삭제/저장/조회 관리

application

  • Url 관련 비즈니스 로직 구현

presentation

  • 예외 처리를 담당하는 ExceptionController
  • view 관련 기능 요청을 관리하는 UrlViewController
  • view 제외 Url 관련 기능 요청을 관리하는 UrlRestController

dto

  • 계층 간 데이터 이동 관리

exception

  • 에러 코드, 메시지 관리하는 UrlErrorCode
  • UrlErrorCode 활용하여 예외 생성하는 UrlException

encoder

  • encoder에 공통적으로 필요한 로직 관리 위한 ShortUrlEncoder 인터페이스
  • 인코딩 알고리즘 구현한 Base58Encoder, Base62Encoder

mapper

  • EncodeType에 알맞은 encoder 생성

✅ 피드백 반영사항

✅ PR 포인트 & 궁금한 점

  • 테스트 코드가 적절히 작성되었는지 궁금합니다.

Copy link

@SangHyunGil SangHyunGil left a 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 {

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 @@

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();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

조회는 빈번하게 일어남으로 동시에 요청이 들어오면 업데이트시 동시성 이슈가 발생할 수 있을 것 같아요.
어떻게 해결할 수 있을 것 같은지 간단한 생각 남겨봐주시면 좋을 것 같아요.

Comment on lines +35 to +41
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);

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))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

케이스 고려 좋네요 👍
이런 부분이 테스트에 녹여져도 좋은 것 같아요.

Comment on lines +10 to +11
BASE58,
BASE62;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

왜 BASE64 / BASE62 / BASE58 중 BASE64는 고려하시지 않았나요?

Comment on lines +38 to +43
Url url = new Url(createShortUrlRequest.getOriginalUrl());
Long urlId = 1234567L;

Field id = url.getClass().getDeclaredField("id");
id.setAccessible(true);
id.set(url, urlId);

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;

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 입력은 예외를 발생시킨다.")

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 {

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.*;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

와일드 카드를 사용하지 않는 방향으로 해보시면 좋을 것 같아요.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants