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

Update message에 message_type 필드 추가 #12

Merged
merged 2 commits into from
May 26, 2024
Merged

Conversation

mokhs00
Copy link
Contributor

@mokhs00 mokhs00 commented May 26, 2024

Summary

  • Message class에 message_type을 추가합니다.
  • 일부 코드를 리팩토링합니다.
    • html 코드 중 불피요한 svg 정리
    • 반복되는 system_message 전송 코드를 ConnectionManager에 응집

@falconlee236 새벽 작업이라 리뷰 시간이 안 맞을 것 같아서 미리 merge해둘게요! 코드 보고 후 리뷰 부탁합니다!

mokhs00 added 2 commits May 27, 2024 01:46
@mokhs00 mokhs00 self-assigned this May 26, 2024
@mokhs00 mokhs00 requested a review from falconlee236 May 26, 2024 16:52
@mokhs00 mokhs00 merged commit cad6c09 into main May 26, 2024
1 check passed
@mokhs00 mokhs00 deleted the feat/add-message-type branch May 26, 2024 17:02

async def disconnect(self, connection: UserConnection):
self.manager.disconnect(connection)
self.users.remove(connection) # 사용자 제거
if not self.users:
Copy link
Member

Choose a reason for hiding this comment

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

이 조건문 별로라고 생각했는데 수정해주셨네요
감사합니다

from pydantic import BaseModel


class MessageType(Enum):
Copy link
Member

Choose a reason for hiding this comment

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

열거형을 사용하는 경우를 잘 못봤는데 텍스트로 사용하는 경우 대신에 쓰는거군요

하나 궁금한게, http request할때 각 header에 persistent connection, set cookies할때도 보통 단순 텍스트가 아니라 열거형을 쓰나요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

음.. 잘 아시겠지만 보통 문자열을 그냥 사용하면 실수할 가능성이 많아서 상수로 사용할 수 있을만한 도구를 쓰는 편이에요! 도구는 언어마다 달라서 적절한 걸 사용하면 되고 여기서는 Enum을 선택했다고 보면 됩니다 ㅎㅎ
어떤 도구를 사용하던 SSOT(Single Source Of Truth)를 두는 방향으로 선택하면 좋은 것 같아용

하나 궁금한게, http request할때 각 header에 persistent connection, set cookies할때도 보통 단순 텍스트가 아니라 열거형을 쓰나요

이런 경우도 보통 다 선언해서 사용해요! Spring 같은 경우는 HttpHeader에 Enum이 모두 정의되어있던 걸로 기억합니당

Copy link
Member

Choose a reason for hiding this comment

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

문자열로 사용하면 데이터 변경이 이루어지기 쉬우니까 ENUM을 사용하자는 것은 이해했습니다.
방금 찾아보니까 단일 진실 공급원에 의해서만 데이터가 수정, 변경이 이루어져야 하는것이 SSOT라고 하는데,
문자열이 변경이 쉬워서 열거형을 사용하는것과 하나의 공급원로 데이터가 전달되어야 한다는 점이 무슨 관계인지 설명해 주실 수 있을까요

Copy link
Contributor Author

@mokhs00 mokhs00 May 27, 2024

Choose a reason for hiding this comment

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

환경 변수 개념으로 하나의 포인터를 바라본다 라고 보면 좋을 것 같은데 사실 아래처럼 문자열을 선언해두고 상수로 사용해도 돼요!

MESSAGE_TYPE_USER_MESSAGE = "USER_MESSAGE"
MESSAGE_TYPE_SYSTEM_MESSAGE = "SYSTEM_MESSAGE"

다만 이렇게하면 코드가 물리적으로 붙어있는 게 강제되진 않고 Enum을 사용하면 물리적으로 붙어있는 게 강제되고 reference로 코드를 찾아가기 쉬워져서 코드를 관리하기 깔끔해지는 것 같아용
python은 잘 모르겠지만 다른 언어에서는 Enum의 필드는 unique함이 보장되는 등 부가적인 기능도 있구요!

여기서 포인트는 아래처럼 static하게 값을 사용하는 경우를 방지하고 모두 하나의 포인터를 바라보게 하기 위함이라고 보면 됩니당

Message(message_type="USER_MESSAGE")

Copy link
Member

Choose a reason for hiding this comment

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

오 하나 또 배워 갑니다 자세한 설명 감사해요


ws.onmessage = function (event) {
const message = JSON.parse(event.data);
if (message.username === "Root") {
Copy link
Member

Choose a reason for hiding this comment

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

이 코드는 위에서 username을 삭제 했는데 작동 안되는거 아닌가요..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

앗 맞습니다 주석을 안 달아뒀네요 ㅠㅠ 체크 감사합니다! 👍
기존 로직에서 room_name을 넘기는 용도였던 것 같아서 이제 사용하지 않는 로직이라 그냥 두었는데 개선 작업에서 챙겨질 예정이에요!
화면상에서 전체 로직은 잘 동작하는 걸로 확인했습니당

Copy link
Member

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update 채팅 메시지를 type enum으로 구분하여 시스템 메시지 등을 분리하여 UI/UX 개선
2 participants