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

불필요한 필드 삭제 및 start_time, end_time 기능 수정 #42

Merged
merged 4 commits into from
Nov 29, 2024

Conversation

ahyeonkong
Copy link
Member

#️⃣ 연관된 이슈

#41

📝 작업 내용

  • fetch join을 통해 코드를 개선했습니다.
  • start_time, end_time 입력 받는 기능을 추가하고 형식을 지정했습니다.
  • 공동 참여자 수를 나타내는 필드를 신청자 포함/미포함으로 분리해서 저장하도록 수정했습니다.

📷 스크린샷 (선택)

image
image
시간과 참여자 수를 수정한 예시입니다.

image
클라이언트가 필요로 하는 응답 값입니다.(피그마 참고)

💬 리뷰 요구사항 (선택)

피드백 환영합니다 🤗

@ahyeonkong ahyeonkong added 🐞 BugFix 버그 수정 🔨 Refactor 코드 리팩토링 labels Nov 27, 2024
@ahyeonkong ahyeonkong self-assigned this Nov 27, 2024
@ahyeonkong ahyeonkong linked an issue Nov 27, 2024 that may be closed by this pull request
3 tasks
Copy link

Test Coverage Report

Overall Project 12.35% -3.93% 🍏
Files changed 0% 🍏

File Coverage
ApplicationService.java 0% -18.01% 🍏
ApplicationResponse.java 0% -47.62% 🍏
ApplicationController.java 0% -37.84% 🍏
Application.java 0% 🍏

Comment on lines +42 to +46
@Column(name = "count_cp_with_applicant") // 신청자 포함 사용 인원 수
private Integer countCpWithApplicant;

@Column(name = "count_cp_only") // 신청자 제외 사용 인원 수
private Integer countCpOnly;
Copy link
Member

Choose a reason for hiding this comment

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

변수명이 길어지더라도 풀네임으로 적어주시는 게 명확할 것 같습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

의견 감사합니다~
너무 길어질 것 같아서 주석을 달아두었는데 풀네임으로 작성하는게 나중에 다시 봤을 때 알아보기 쉬울 것 같네요!

Copy link
Contributor

Choose a reason for hiding this comment

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

저는 신청자 제외 사용 인원 수 가 꼭 필요한 게 아니면 그냥 기존대로 해도 괜찮을 것 같아요!

사실 웬만하면 신청자 제외 사용 인원 수 = 신청자 포함 사용 인원 수 - 1 일 것 같아서 ㅋㅋㅋㅋ 필요하면 그냥 서비스 로직에서 처리해도 괜찮을 듯요

Copy link
Member Author

Choose a reason for hiding this comment

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

피드백 감사합니다 !!

ㅋㅋㅋ 그렇긴 한데 필드명을 똑같이 하니까 DB에서 꼬여서 따로 만들었습니다 😂😂
서비스 로직에서 잘 처리하면 안 꼬이고 할 수 있을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

네 뭔가 기존처럼

@Column(name = "participant_count")
private Integer participantCount;

요렇게 하고 신청자 제외 사용 인원 수는 participantCount - 1 요렇게 하면 될 것 같습니당

혹시 필드명이 DB에서 꼬인다는 게 어떤 뜻인가유?

Copy link
Member Author

Choose a reason for hiding this comment

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

신청서를 신청, 수정, 조회하는 과정에서 DB에 데이터가 잘못 계산되어 들어갔다는 뜻이었습니다!
예를 들면 participantCount가 3이 나와야 하는데 어떤 이유로 2가 들어가 있었다는 얘기입니다.

변수를 2개 사용한 이유가 DB에 데이터가 잘못 저장되어서 그랬는데,
다시 생각을 해보니 변수 1개로도 충분히 구현할 수 있을 것 같습니다. 그리고 이렇게 에러가 난 부분을 고치는 게 맞는 방식인 것 같습니다.

아니면 서버 쪽에서는 신청자를 포함한 사용 인원만 저장해두고, 프론트엔드에서 필요에 의해 participantCount - 1로 데이터를 바꿔 화면에 보여주는 방법은 어떤가요?? 사실 제가 이런 협업을 안 해봐서 잘 모르겠습니다...

아직 기초가 부족해서 조금 비효율적으로 코드를 작성한 것 같습니다..🥲
혼자 공부하다가 이렇게 피드백 받고 다시 생각해볼 수 있게 해주셔서 너무 감사하네요🤩🤩
충분히 고민해보고 말씀하신 대로 코드 수정하겠습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

아아 이해했습니다 ㅋㅋㅋㅋ

신청자 제외 사용 인원 같은 경우는 백엔드 측에서 처리하면 좋을 것 같긴 합니다. 보통 변경이 잦은 값이면 프론트엔드 측에서 알아서 가공해서 사용하게 되는데, 신청자 제외 사용 인원 = 신청자 포함 사용 인원 수 - 1 라는 부분에 딱히 변경의 여지가 없어보여서 백엔드에서 전부 처리해서 보여주면 더 좋을 것 같아요!

제 생각엔 Application 엔티티에 있는 필드는 말씀하신대로 1개로 유지하고, DTO에만 신청자 제외 사용 인원, 신청자 포함 사용 인원 을 모두 추가해서 반환하도록 하면 좋을 것 같습니다.

도움이 됐다니 다행이네요 ^ㅡ^

Copy link
Member Author

Choose a reason for hiding this comment

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

오호 좋습니다!

DTO에 모두 추가해서 반환하는 방법이 있었군요 ㅋㅋㅋ
의견 반영해서 백엔드 측에서 처리하도록 하겠습니다!

친절한 답변 감사합니다🙇🏻🙇🏻

Comment on lines +42 to +46
@Column(name = "count_cp_with_applicant") // 신청자 포함 사용 인원 수
private Integer countCpWithApplicant;

@Column(name = "count_cp_only") // 신청자 제외 사용 인원 수
private Integer countCpOnly;
Copy link
Contributor

Choose a reason for hiding this comment

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

저는 신청자 제외 사용 인원 수 가 꼭 필요한 게 아니면 그냥 기존대로 해도 괜찮을 것 같아요!

사실 웬만하면 신청자 제외 사용 인원 수 = 신청자 포함 사용 인원 수 - 1 일 것 같아서 ㅋㅋㅋㅋ 필요하면 그냥 서비스 로직에서 처리해도 괜찮을 듯요

Comment on lines +29 to +32
@Column(name = "start_time", nullable = false, columnDefinition = "TIMESTAMP(0)") // 밀리초 자릿수를 0으로 지정
private LocalDateTime startTime;

@Column(name = "end_time", nullable = false)
@Column(name = "end_time", nullable = false, columnDefinition = "TIMESTAMP(0)") // 밀리초 자릿수를 0으로 지정
Copy link
Contributor

Choose a reason for hiding this comment

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

columnDefinition 디테일 좋네요 ㄷㄷ

Copy link
Member Author

Choose a reason for hiding this comment

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

감사합니다 ㅎㅎ

@Muokok Muokok merged commit 60cb292 into develop Nov 29, 2024
1 check passed
@holyPigeon holyPigeon deleted the feat/#41 branch December 1, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 BugFix 버그 수정 🔨 Refactor 코드 리팩토링
Projects
None yet
Development

Successfully merging this pull request may close these issues.

불필요한 필드 삭제 및 start_time, end_time 기능 수정
3 participants