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기-이경민] SprintBoot URL Shortener 구현 과제 제출입니다. #58

Open
wants to merge 9 commits into
base: tidavid1
Choose a base branch
from

Conversation

tidavid1
Copy link
Member

📌 과제 설명

  • url을 입력받아서 단축 url을 생성해주는 서비스입니다.

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

요구 사항

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

구현 내용

  • 전달받은 원본 URL 기반 Short Url 생성 후 전달 (Base62, Base58 인코딩 구현)
    • 원본 URL -> SHA-256 해싱 -> 인코딩 -> Short URL 전달.
  • 생성되지 않은 단축 URL 요청시 예외 페이지 이동

Docker MySql Command

docker run --name ShortUrlDB -e MYSQL_ROOT_PASSWORD=root! -e MYSQL_DATABASE=ShortUrl -d -p 3306:3306 mysql

✅ PR 포인트 & 궁금한 점

  1. 과제를 시작하면서 단축 URL을 생성하기 위한 인코딩 방식에 대해서 조사를 했을때, 많은 자료들이 DB에 저장하는 pk 값(Long)을 기반으로 인코딩을 수행하는 것을 확인했습니다. 그런데 위와 같은 방식은 Url에 대한 단축을 진행했다고 보기 어려워서 해싱 이후 인코딩하는 방식을 활용했습니다.
  2. Web MVC에 대한 테스트코드를 저번 코드 리뷰때 알려주신 방식에 기반해서 UrlControllerTest에 작성해보았습니다. 이 방식 이외에도 추가로 MVC에 대해 테스트 해볼만한 요소들이 어떤 것들이 있을까요?
  3. BASE62, BASE58 클래스 모두 static 메서드를 사용하여 인코딩을 진행하므로 이를 묶어서 한번에 활용하고자 Enum을 활용해서 구현을 진행했는데, 생각해보니 인코딩 관련 기능이 EncodeHelper, BASE62, BASE58, EncodeType에 나눠져 있어 코드의 응집도가 떨어지지 않을까? 라는 질문이 머리속에 남았습니다. 이에 대한 의견이 궁금합니다!

Copy link

@happyjamy happyjamy left a comment

Choose a reason for hiding this comment

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

경민님 수고하셨어요!
각 레이어마다 dto 만드신게 인상 깊었습니다.
꼼꼼하신 점 배우고싶어요 굳굳 LGTM 👍

testImplementation 'org.springframework.boot:spring-boot-starter-test'
testImplementation 'org.springframework.boot:spring-boot-testcontainers'
testImplementation 'org.testcontainers:junit-jupiter'
testImplementation 'org.testcontainers:mysql'

Choose a reason for hiding this comment

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

이런게 있군요! 배워갑니다 🚣

public UrlServiceResponseDto createShortUrl(UrlServiceRequestDto requestDto) {
String longUrl = requestDto.getLongUrl().getValue();
Url url = urlRepository.findByLongUrlAndEncodeType(longUrl, requestDto.getEncodeType())
.orElseGet(() -> urlRepository.save(Url.builder()

Choose a reason for hiding this comment

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

UrlServiceRequestDto 에서 바로 Url 을 만들지 않는데, 이유가 있나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

dto에서 Entity 매핑기능까지 필요할까? 라는 생각이 있었는데, 생각해보니 외부에서 값을 계속 집어넣어주는 것보다 안전한 방향일 것 같네요! 한번 수정해보도록 하겠습니다.

return UrlServiceResponseDto.of(url);
}

public UrlServiceResponseDto findLongUrl(UrlServiceRequestDto requestDto) {

Choose a reason for hiding this comment

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

내부에서 updateHit() 도 해준다면 함수 이름에 이것을 반영해야 할 것 같아요.

@RequestMapping("/")
public class UrlController {
private final UrlService urlService;
private static final String SERVER_URL = "http://localhost:8080/";

Choose a reason for hiding this comment

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

환경변수로 안쓰는 이유가 있나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Value를 활용한 생성자 주입방식으로 수정해보도록 하겠습니다 :D

import lombok.NoArgsConstructor;

@Getter
@NoArgsConstructor(access = AccessLevel.PRIVATE)

Choose a reason for hiding this comment

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

얘의 역할은 몬가용..?

Copy link
Member Author

Choose a reason for hiding this comment

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

private 접근권한의 기본 생성자를 생성해줍니다!

this.encodeType = encodeType;
}

public void updateHit() {

Choose a reason for hiding this comment

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

이런식으로 구현하면 동시성 문제때문에 동시에 가져온 값에 대해서 2번 증가되야할 값이 한번만 증가될 것 같습니다.
hit 이 정교해야하는 값이라면 동시성을 고려하는것이 좋겠지요

Copy link
Member Author

Choose a reason for hiding this comment

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

어떤 방식으로 동시성을 고려하면 좋을까요....?

runtimeOnly 'com.mysql:mysql-connector-j'
annotationProcessor 'org.projectlombok:lombok'
testImplementation 'org.springframework.boot:spring-boot-starter-test'
testImplementation 'org.springframework.boot:spring-boot-testcontainers'
Copy link

@SeokRae SeokRae Dec 13, 2023

Choose a reason for hiding this comment

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

테스트 컨테이너라는 기술을 도입하셨군요.!

데이터베이스와 같은 미들웨어를 사용할 때, 테스트가 실 데이터에 영향을 줄 수 있다는 문제로 인해서
인메모리 디비인 H2, 테스트 컨테이너 또는 특수한 경우 검증계 용 데이터베이스를 별도로 구축하여 데이터에 대한 테스트를 진행하기도 합니다.

이번 기회로 기존 문제점이 무엇이었고, 이를 해소하기 위해 또는 개선하기 위해
테스트 컨테이너를 사용했는지, 최종적으로 얻을 수 있는 이점들을 공유해보는 것도 좋을 듯 합니다.

import java.util.regex.Pattern;

@Getter
public class LongUrl {
Copy link

Choose a reason for hiding this comment

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

값 객체를 별도로 정의하여 사용하시는 군요.!

Value Object라는 개념을 적용하게 되신 이유를 한번 들어보고 싶네요.!
어떤 이점이 있었는지?

Copy link
Member Author

Choose a reason for hiding this comment

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

URL 검증을 좀 더 명확하게 하고자 VO를 사용했습니다! 객체 생성만으로 검증까지 가능하다는 이점을 갖고 있는 것 같습니다!

}

@Override
public String toString() {
Copy link

Choose a reason for hiding this comment

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

toString과 getter의 차이가 혹시 있나요?

Copy link
Member Author

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

3 participants