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

Refactor 코드, 디렉토리 구조 개선 #16

Merged
merged 6 commits into from
May 27, 2024

Conversation

mokhs00
Copy link
Contributor

@mokhs00 mokhs00 commented May 27, 2024

Summary

  • 전체적으로 코드와 디렉토리 구조를 정리합니다.
    • api 로직 분리
    • web js api 관련 로직 분리
    • 의존성을 싱글톤으로 관리

테스트 완료해서 이것도 바로 merge 할게요! @falconlee236 나중에 리뷰 부탁드립니당 디렉토리 구조는 느낌대로 해봤는데 의견 공유해줘도 좋을 것 같아요!

@mokhs00 mokhs00 requested a review from falconlee236 May 27, 2024 13:35
@mokhs00 mokhs00 self-assigned this May 27, 2024
@mokhs00 mokhs00 changed the title Refactor 코드 구조 개선 Refactor 코드, 디렉토리 구조 개선 May 27, 2024
@mokhs00 mokhs00 merged commit 8a5cae9 into main May 27, 2024
1 check passed
@mokhs00 mokhs00 deleted the refactor/arrangement-codes branch May 27, 2024 13:40

@router.get("/rooms")
async def get_rooms():
return ListChatRoomsResponse(
Copy link
Member

Choose a reason for hiding this comment

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

뜬금없는데
ChatRoomsResponseList로 바꾸면 더 좋을거같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

헛 이건 약간 컨벤션 서로 맞춰서 가면 좋을 것 같은데 함수명을 list_rooms로 바꾸는 건 어떨까요?

보통 {API이름}Response 이런 식으로 네이밍이 하는 게 자주 보였던 것 같아용

Copy link
Member

Choose a reason for hiding this comment

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

get_room_list 이거는 어떤가요
항상 함수 이름 정하는게 어렵네요

from service.chat.connection_manager import ConnectionManager
from service.emotion_analysis.emotion_classifier import EmotionClassifier

# DI 구조를 고민하다가 지금은 단순하게 여기에 의존성을 Singleton으로 선언해둡니다.
Copy link
Member

Choose a reason for hiding this comment

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

싱글톤이 외부에서 정적 메소드만 참조할수있는걸로 알고있는데, 파이썬에서는 static, private개념이 없는데 싱글톤을 어떻게 정의하나요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

좀 특이한데 이렇게 변수로 선언해두고 이것만 사용하면 싱글톤의 효과입니다 ㅠㅠ,, 생성자에서 싱글톤 패턴을 구현하는 방법도 있는데 이렇게 하는 게 좀 더 단순한 방법이라고 보면 좋을 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fyi) 아마 파이썬에 의존성 주입(DI) 패키지가 있을 것 같고 보통 DI 패키지를 사용해요!

Copy link
Member

Choose a reason for hiding this comment

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

파이썬 class는 아무리 봐도 잘못 설계되었어요
접근 제한자도 없고 self가 뭐야 self가

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

Successfully merging this pull request may close these issues.

2 participants