-
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기-이경민] SprintBoot URL Shortener 구현 과제 제출입니다. #58
base: tidavid1
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.
경민님 수고하셨어요!
각 레이어마다 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' |
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 UrlServiceResponseDto createShortUrl(UrlServiceRequestDto requestDto) { | ||
String longUrl = requestDto.getLongUrl().getValue(); | ||
Url url = urlRepository.findByLongUrlAndEncodeType(longUrl, requestDto.getEncodeType()) | ||
.orElseGet(() -> urlRepository.save(Url.builder() |
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.
UrlServiceRequestDto 에서 바로 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.
dto에서 Entity 매핑기능까지 필요할까? 라는 생각이 있었는데, 생각해보니 외부에서 값을 계속 집어넣어주는 것보다 안전한 방향일 것 같네요! 한번 수정해보도록 하겠습니다.
return UrlServiceResponseDto.of(url); | ||
} | ||
|
||
public UrlServiceResponseDto findLongUrl(UrlServiceRequestDto requestDto) { |
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.
내부에서 updateHit() 도 해준다면 함수 이름에 이것을 반영해야 할 것 같아요.
@RequestMapping("/") | ||
public class UrlController { | ||
private final UrlService urlService; | ||
private static final String SERVER_URL = "http://localhost:8080/"; |
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.
환경변수로 안쓰는 이유가 있나요?
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.
@Value
를 활용한 생성자 주입방식으로 수정해보도록 하겠습니다 :D
import lombok.NoArgsConstructor; | ||
|
||
@Getter | ||
@NoArgsConstructor(access = AccessLevel.PRIVATE) |
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.
얘의 역할은 몬가용..?
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
접근권한의 기본 생성자를 생성해줍니다!
this.encodeType = encodeType; | ||
} | ||
|
||
public void updateHit() { |
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.
이런식으로 구현하면 동시성 문제때문에 동시에 가져온 값에 대해서 2번 증가되야할 값이 한번만 증가될 것 같습니다.
hit 이 정교해야하는 값이라면 동시성을 고려하는것이 좋겠지요
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.
어떤 방식으로 동시성을 고려하면 좋을까요....?
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' |
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.
테스트 컨테이너라는 기술을 도입하셨군요.!
데이터베이스와 같은 미들웨어를 사용할 때, 테스트가 실 데이터에 영향을 줄 수 있다는 문제로 인해서
인메모리 디비인 H2, 테스트 컨테이너 또는 특수한 경우 검증계 용 데이터베이스를 별도로 구축하여 데이터에 대한 테스트를 진행하기도 합니다.
이번 기회로 기존 문제점이 무엇이었고, 이를 해소하기 위해 또는 개선하기 위해
테스트 컨테이너를 사용했는지, 최종적으로 얻을 수 있는 이점들을 공유해보는 것도 좋을 듯 합니다.
import java.util.regex.Pattern; | ||
|
||
@Getter | ||
public class LongUrl { |
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.
값 객체를 별도로 정의하여 사용하시는 군요.!
Value Object라는 개념을 적용하게 되신 이유를 한번 들어보고 싶네요.!
어떤 이점이 있었는지?
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 검증을 좀 더 명확하게 하고자 VO를 사용했습니다! 객체 생성만으로 검증까지 가능하다는 이점을 갖고 있는 것 같습니다!
} | ||
|
||
@Override | ||
public String toString() { |
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.
toString과 getter의 차이가 혹시 있나요?
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.
없습니다! 둘 중 하나는 삭제해도 될거같아요..
📌 과제 설명
👩💻 요구 사항과 구현 내용
요구 사항
구현 내용
Docker MySql Command
docker run --name ShortUrlDB -e MYSQL_ROOT_PASSWORD=root! -e MYSQL_DATABASE=ShortUrl -d -p 3306:3306 mysql
✅ PR 포인트 & 궁금한 점
UrlControllerTest
에 작성해보았습니다. 이 방식 이외에도 추가로 MVC에 대해 테스트 해볼만한 요소들이 어떤 것들이 있을까요?BASE62
,BASE58
클래스 모두 static 메서드를 사용하여 인코딩을 진행하므로 이를 묶어서 한번에 활용하고자 Enum을 활용해서 구현을 진행했는데, 생각해보니 인코딩 관련 기능이EncodeHelper
,BASE62
,BASE58
,EncodeType
에 나눠져 있어 코드의 응집도가 떨어지지 않을까? 라는 질문이 머리속에 남았습니다. 이에 대한 의견이 궁금합니다!