-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: member 상태관리를 위한 member.status 추가에 따른 수정 #200
base: develop
Are you sure you want to change the base?
Conversation
@@ -39,6 +39,9 @@ public class Member extends BaseTimeEntity { | |||
@Column(nullable = true) | |||
private String job; | |||
|
|||
@Column(nullable = true, length = 1) | |||
private Integer status; |
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.
1 일 때 탈퇴, null 일 때 일반 회원이라는 의미로 정한다면 우려되는점이 몇 가지 있는데요.
우선 1이라는 magic number 가 탈퇴 회원인 상태라는 걸 나타낸다고 알고있어야 코드를 이해할 수 있어서 가독성면에서 단점이 있는것같습니다. 상태가 늘어난다면 2,3,4 마다 각각 다른 의미를 가질텐데, 숫자와 의미를 매핑해야해서요.
그리고 nullable 한 타입을 사용할 때 null
에 다른 의미를 부여하게되면, 실수로 null 을 입력한것과 제대로 null 을 입력한 것을 구분하기 어려워서 이슈가 생겼을 때 데이터를 추적하기에 어려울 것 같습니다.
MemberStatus 등의 enum 을 만들고, ACTIVE, WITHDRAWAL 등의 상수로 회원의 상태를 구분하는건 어떠쎄요?
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.
처음에 생각했던 방법인데요~
상수로 한다면, 컬럼 속성 및 데이터 변경 등의 DDL 작업과 merge 작업을 최대한 맞춰서 해야할 것 같고요~
enum converter 사용도 한번 고려해보면 좋을 것 같아요~
장단점이 있을 것 같은데~ 현재 구조를 유지하면서 간단하게 변경할 수 있는 것으로 우선 적용해 놓은 것입니다~
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.
새 컬럼을 추가하는거라면 코드 머지 시점과 맞출 필요는 없고, 배포 이전에만 DDL 먼저 적용해도됩니다.
컬럼 데이터 migration 이 필요한데요.
추가된 컬럼의 null 을 수정하기 전에, null 도 일반 회원이라는 등의 로직을 작성하기 위해 AttributeConverter 말씀하신거라면, 굳이 필요하지 않을 것 같습니다.
꼭 무중단으로 운영할필요도 없고, 데이터도 많지않으니새 코드 적용하기전에 서버 잠깐 내리고 db 작업하면 두 번 배포하지 않아도 되어서요.
enum 대비 null 로 처리해서 얻을 수 있는 코드작성시점의 시간적 이득보다, 앞으로 member 코드를 읽으면서 의문점이 남거나 불편한 단점이 더 크다고 생각해서 enum 으로 처리하면 좋을 것 같습니다-
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.
제가 converter 사용을 고려한 이유는 다음글들을 참고해서 db 공간 낭비를 생각해서였습니다.
https://pamyferret.tistory.com/6
https://nomadlee.com/mysql-%EC%B5%9C%EC%A0%81%EC%9D%98-%EB%8D%B0%EC%9D%B4%ED%84%B0-%ED%83%80%EC%9E%85-%EC%84%A0%ED%83%9D-%EB%B0%A9%EB%B2%95/
말씀하신 대로 서비스 규모가 작으니.. 이런 것들을 고려할 필요할 있을가? 라는 생각도 하게 됐고..
그러다 보니 다시 enum 사용 없이 제일 단순한 구조로 오게 되었습니다.
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.
서비스 규모가 작으니 바이트 단위로 용량 아끼는건 생략해도 좋겠다는 의견에 동의하고요.
다만 null 에 의미 부여하는 코드는 가독성면에서 받아들이기 어렵네요
enum 을 사용할때 숫자로 저장할지 문자로 저장할지는 크게 중요한 이슈 아닌거같습니다.
다른사람이 코드를 읽었을 때 '왜 null 정상인 멤버이지?'라고 고민하게되는 비용이 제일 크게 작용할거같아요
migration 및 코드 수정하는 비용은 1시간 미만일거같은데, 앞으로 코드 읽으면서 헷갈린다면 그렇게 낭비되는 시간이 더 오래걸릴 것 같습니다.
@@ -0,0 +1,2 @@ | |||
alter table member |
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.
flyway 거의 신경 못쓰고있었는데 감사합니다 👍
b530e5f
to
75758fa
Compare
75758fa
to
2fef692
Compare
Kudos, SonarCloud Quality Gate passed! |
📌Linked Issues
✏Change Details
💬Comment
📑References
✅Check List