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 과제 제출합니다. #68

Open
wants to merge 15 commits into
base: JeongeunChoi
Choose a base branch
from

Conversation

JeongeunChoi
Copy link

@JeongeunChoi JeongeunChoi commented Dec 16, 2023

📌 과제 설명

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

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

controller

  • 기존 Url 입력 시, 단축 Url을 반환
  • 단축 Url로 요청이 들어오면, 기존 Url로 리다이렉트

service

  • 기존 Url로 단축 Url을 생성하여 반환하는 로직
  • 단축 Url을 기존 Url로 반환해주는 로직

domain

  • Url 엔터티
    • id, originUrl, requestCnt
    • shortUrl은 db에서 저장하지 않아도 id를 encoding하여 알 수 있으므로 속성 값으로 넣지 않았습니다!

exception

  • 유효하지 않은 originUrl 예외 처리
  • 존재하지 않는 shortUrl 예외 처리

repository

  • JPA로 구현하여 MySQL에 저장

Base62Util

  • 단축 알고리즘으로 Base62 사용

UI

단축할 URL 입력 후 단축 요청

단축된 URL 확인

단축된 URL로 접근 시, 원래 URL로 리다이렉트

✅ 피드백 반영사항

✅ PR 포인트 & 궁금한 점

  • 과제 요구사항을 하나씩 구현하다가 "단축된 URL에 대한 요청 수 정보저장"은 어떤 경우에 활용되는지 의문이 들었습니다! 이번 과제에서는 요청 수를 저장하긴 하지만, 사용하는 일이 없는데 보통 요청 수는 어떻게 활용되는지 궁금합니다!
  • id 값을 1부터 생성되게 하니, 인코딩된 값이 짧다고 느껴져 id 값이 10000으로 시작되도록 하였는데, 일반적인 방법인지 궁금합니다!
  • 서비스 테스트 코드를 작성 중, 레포지토리를 Mocking 하기 위해 @mock와 @MockBean 두가지 중 어떤 것을 사용해야할까 의문이 들어 찾아보았습니다. @mock은 스프링 컨텍스트와 관련이 없고 @MockBean은 스프링 컨텍스트에 생성된다.라고 이해하여 컨트롤러 테스트에서는 @MockBean을 서비스 테스트에서는 @mock을 사용하였는데 적절히 사용된 것인지 궁금합니다!
  • 서비스 테스트를 단위테스트로 작성해보았는데, Base62Util의 경우는 직접적인 사용이 필요하다고 판단하여 @InjectMock를 통해 주입해주었는데 단위테스트에 어긋난건 아닌지 궁금합니다! Base62Util에 대한 테스트를 따로 생성하는게 일반적인가요?

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.

전반적으로 요구사항 잘 구현해주신 것 같습니다.
작성해주신 부분들 중 코멘트 몇 개 남겨두었으니 확인 부탁드리겠습니다.

  • 테스트시 별도의 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() {
}

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

Choose a reason for hiding this comment

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

Mysql로 사용하는데 SEQUENCE 전략으로 설정하신 이유가 있나요?

Comment on lines +5 to +6
@Component
public class Base62Util {

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;

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

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

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 {

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 +38
@BeforeEach
void setUp() {
urlService = new UrlService(urlRepository, base62Util);
}

Choose a reason for hiding this comment

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

테스트마다 초기화되어야하는 대상일까요?

Comment on lines +24 to +25
@Column(name = "originUrl")
private String originUrl;

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 {

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