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

Open
wants to merge 34 commits into
base: eunchannam
Choose a base branch
from

Conversation

EunChanNam
Copy link

📌 과제 설명

  • URL을 입력받아 단축된 URL을 제공하는 서비스입니다
  • BASE_62 를 기반으로 한 shorten url 을 제공합니다

데이터베이스

image

URL 입력

image

URL 단축 결과

image

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

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

✅ PR 포인트 & 궁금한 점

  • URL 단축기 흐름을 생각하며 디코딩에 대한 코드는 필요없을 것 같아서 따로 구현하지 않았는데 구현이 필요한지 궁금합니다?
  • 인코딩하는 로직에 대해서(Base62Encoder) 저는 비즈니스 성격이라고 풀어내서 domain 패키지 안에 녹여서 풀었는데, 이에 대해서 맞는 판단인지 궁금합니다
  • IdGenerator 대해서는 인터페이스는 domain 패키지에 넣고 구현체 자체는 Infrastructure 에 넣고 풀어냈는데, IdGenerator 인터페이스가 domain 패키지에 위치하는게 맞는지 궁금합니다

* IdGenerator 는 아직 구현하지 않음
Copy link

@ordilov ordilov left a comment

Choose a reason for hiding this comment

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

과제 고생하셨습니다!!
코드는 하시는 프로젝트랑 같이 올려주신 것 같아요 😭
그래서 질문주신 부분들 위주로 답변 드리겠습니다.

  1. 말씀주신대로 디코딩에 대한 부분은 없어도 될 것 같습니다.
    다만 현재 인코딩 부분에서 디코딩 지원이나 url이 많아지면 문제가 있을 것 같습니다.
    "A" 로 패딩 처리를 하게되면 실제로 "A"가 들어가게 되었을 때 구분하는 로직이 없어보입니다.

  2. 모든 부분에는 합당한 이유가 있다면 패키지 구조에 대해선 정답이 없다고 생각해서 비즈니스 부분이라고 생각하신다면 맞는 판단으로 보입니다.

  3. 2번과 마찬가지로 패키지 구조는 판단에 따르는거지만, 인터페이스와 구현체가 다른 패키지에 있는 건 응집력이 떨어지는 구조라고 생각합니다. 그럴 이유도 없고, 의존 그래프가 어색하게 엮일 것으로 우려됩니다.

@ordilov ordilov self-requested a review December 23, 2023 13:40
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