날짜 / 시간 및 기타 코드 컨벤션 통일 #78

pricelees · 2024-07-19T06:24:07Z

PR의 목적이 무엇인가요?

날짜 / 시간 형식 및 코드 컨벤션 통일

이슈 ID는 무엇인가요?

closed 날짜 / 시간 및 기타 코드 컨벤션 통일 #73

설명

코드 컨벤션은 7월 18일(목) 회의에서 결정된 내용 반영
날짜 형식은 yyyy-MM-dd, 시간 형식은 HH:mm을 사용하도록 수정

질문 혹은 공유 사항 (Optional)

구현 방법

직렬화 / 역직렬화시 날짜, 시간 형식 통일은 Custom ObjectMapper 빈 등록으로 구현하였고, 테스트는 RestAssured를 이용하였습니다.

논의하고 싶은 사항

이번 구현에서는 날짜 형식(yyyy-MM-dd)과 시간 형식(HH:mm)을application.yml에 작성하고 구현했는데, 지금 단계에서는 그냥 클래스 내 상수로 지정해도 크게 문제가 없다는 생각도 드네요. 이런 속성값을 다루는 방법을 논의하면 좋을 것 같습니다.

- Moim.currentPeople이 null로 초기화되는 문제 해결

- service에서 반환된 dto를 이용하는게 아닌, repository에서 반환된 domain을 이용하여 테스트

ay-eonii

고생 많으셨습니다 상돌~!

덕분에 코드가 전반적으로 깔끔해졌네요 👍
코멘트 확인 부탁드려용 ㅎㅅㅎ

ay-eonii · 2024-07-19T11:42:19Z

backend/src/main/java/mouda/backend/moim/controller/MoimController.java

+	public ResponseEntity<RestResponse<MoimDetailsFindResponse>> findMoimDetails(@PathVariable Long moimId) {
+		MoimDetailsFindResponse moimDetailsFindResponse = moimService.findMoimDetails(moimId);
+
+		return ResponseEntity.ok().body(new RestResponse<>(moimDetailsFindResponse));
 	}

 	@Override
 	@PostMapping("/join")
 	public ResponseEntity<RestResponse<Void>> joinMoim(@RequestBody MoimJoinRequest moimJoinRequest) {


Suggested change

public ResponseEntity<RestResponse<Void>> joinMoim(@RequestBody MoimJoinRequest moimJoinRequest) {

public ResponseEntity<Void> joinMoim(@RequestBody MoimJoinRequest moimJoinRequest) {

발견 👀

발견 👀

감사합니닷👍

ay-eonii · 2024-07-19T11:50:02Z

backend/src/main/java/mouda/backend/config/DateTimeFormat.java

+@ConfigurationProperties("datetime.format")
+@RequiredArgsConstructor
+@Getter
+public class DateTimeFormat {


(반영하지 않아도 좋습니다!)

현재 yml 파일에서 포맷을 관리하는데 이 클래스가 필요한 이유가 무엇인가요?? JacksonConfig에서 objectMapper를 빈으로 등록할 필요 없이 @Value로 값을 가져오는 방법도 깔끔할 것 같아요! pr에 남겨주신 것처럼 상수로 관리하는 것도 좋을 것 같습니다 👍

@Configuration public class JacksonConfig { @Value("${datetime.format.date}") private String dateFormat; @Value("${datetime.format.time}") private String timeFormat; ... }

(반영하지 않아도 좋습니다!)

현재 yml 파일에서 포맷을 관리하는데 이 클래스가 필요한 이유가 무엇인가요?? JacksonConfig에서 objectMapper를 빈으로 등록할 필요 없이 @Value로 값을 가져오는 방법도 깔끔할 것 같아요! pr에 남겨주신 것처럼 상수로 관리하는 것도 좋을 것 같습니다 👍

간단하게만 말씀드리면 계층 구조인 설정 값을 매핑하기 유리함 / final 지정 가능 / 타입 안정성 / 일일이 Value 안 붙여도 됨 / Bean Validation 등등 여러 장점도 있지만.. 객체로 값을 관리한다라는게 가장 큰 장점인 것 같네요😄

Mingyum-Kim

직렬화 역직렬화 테스트까지 꼼꼼히 해주신 것 잘 봤어요 👍 고생했습니다 상돌

제 의견을 반영해서 같이 논의해보면 좋을 만한 부분 리뷰 남겨놨어요!
주말 푹 쉬시고 월요일에 같이 작업해봐요 ~

Mingyum-Kim · 2024-07-20T04:28:30Z

backend/src/main/java/mouda/backend/moim/domain/Moim.java

+	private Integer currentPeople;

-	private int maxPeople;
+	private Integer maxPeople;


저희가 나눈 컨벤션 회의 결론을 파라미터를 래퍼 타입으로 통일한다라고 이해했어요.
Id는 NULL일 가능성이 있기 때문에 도메인에서도 Long 타입으로 작성하였어요. 그래서 컨트롤러 파라머티에서도 Long으로 가져왔다고 생각하는데, currentPeople과 maxPeople이 NULL인 상황이 있을까요? 🤔
이 부분에 대해서 같이 고민해보면 좋을 것 같습니다!

저희가 나눈 컨벤션 회의 결론을 파라미터를 래퍼 타입으로 통일한다라고 이해했어요. Id는 NULL일 가능성이 있기 때문에 도메인에서도 Long 타입으로 작성하였어요. 그래서 컨트롤러 파라머티에서도 Long으로 가져왔다고 생각하는데, currentPeople과 maxPeople이 NULL인 상황이 있을까요? 🤔 이 부분에 대해서 같이 고민해보면 좋을 것 같습니다!

파라미터를 래퍼 타입으로 통일한다면 모임을 생성할 때 지정하는 maxPeople 값 역시 포장하는 것으로 이해했습니다😄

(지극히 개인적인 생각임)
currentPeople 값이 NULL일 가능성이 지금 상황에서는 없다고 생각되나 개인적으로는 maxPeople값을 포장했으면 연관된 currentPeople값도 포장하는 것이 더 일관된다고 생각해요!

사용자 입력 값이 NULL인 상황은 충분히 생길 수 있으니 DTO에서는 래퍼 타입을 사용하는 것에 동의합니다 !!
그러나 하나의 모임 객체에서 maxPeople이 NULL 인 것이 허용되는 요구사항인가요? 그렇지 않다면 객체 생성 시 NULL 처리를 해주고 원시값으로 정의하는 게 좋다는 의견입니다 😄

따라서 제 의견은 maxPeople과 currentPeople 둘 다 엔티티에서는 원시값으로 사용되어야하지않을까 해요 ~

Mingyum-Kim · 2024-07-20T04:31:46Z

backend/src/main/java/mouda/backend/moim/dto/request/MoimCreateRequest.java

@@ -10,10 +10,11 @@ public record MoimCreateRequest(
 	LocalDate date,
 	LocalTime time,
 	String place,
-	int maxPeople,
+	Integer maxPeople,


[질문] 프론트엔드 측에서 maxPeople 값을 입력해주지 않으면 이 부분이 NULL로 들어오게 되는건가요?

[의견] 추후에 Bean Validation 과 LocalTime, LocalDate 포맷팅 에러를 핸들링하는 것도 필요하겠네요 !

[질문] 프론트엔드 측에서 maxPeople 값을 입력해주지 않으면 이 부분이 NULL로 들어오게 되는건가요?

이 부분은 어떻게 처리하는지에 따라 달라져요. 기본적으로는 Json에 maxPeople 필드가 없거나 빈 문자열이 들어오면 NULL이 입력되고, 숫자 이외의 값이 입력되면 HttpMessageNotReadableException이 발생합니다😀

[의견] 추후에 Bean Validation 과 LocalTime, LocalDate 포맷팅 에러를 핸들링하는 것도 필요하겠네요 !

조아요!👍

오호 그렇군요 굿굿 👍
나중에 숫자 이외의 입력에 대해서도 에러 핸들링을 할 필요가 있겠네요.

Mingyum-Kim · 2024-07-20T04:35:01Z

backend/src/main/java/mouda/backend/moim/dto/response/MoimFindAllResponse.java

@@ -8,16 +8,17 @@

 @Builder
 public record MoimFindAllResponse(
-	long moimId,
+	Long moimId,


요청 DTO에는 null 처리를 위해서 래퍼 타입을 사용하는 것은 맞지만

응답 DTO에서는 moimId가 null 인 상황이 있을까요? 앞서 엔티티에 래퍼 타입을 사용하는 것과 유사한 맥락의 의견입니다! 저는 null 일 수 없는 데이터라면 원시 타입을 사용해야한다고 생각해요.

요청 DTO에는 null 처리를 위해서 래퍼 타입을 사용하는 것은 맞지만

응답 DTO에서는 moimId가 null 인 상황이 있을까요? 앞서 엔티티에 래퍼 타입을 사용하는 것과 유사한 맥락의 의견입니다! 저는 null 일 수 없는 데이터라면 원시 타입을 사용해야한다고 생각해요.

(지극히 개인적인 관점)
원시 타입의 사용은 성능상의 이점이 있다는 이유가 크겠죠? 이 부분을 고려하지 않은 것은 아니지만 지금의 프로그램에서는 성능상의 이점보다 코드의 일관성이 더 큰 장점이라고 생각했어요. 원시 타입으로 바꾸는 것은 크게 어렵지 않지만.. 지금 단계에서 지정하는 것 보다 추후에 성능 이슈가 발생했을 때 바꿔보고 개선되는 것을 직접 확인하는 과정이 더 의미있다고 생각했습니다!

음음 좋은 의견이에요!
하지만 저는 성능상의 이점보다는 코드를 봤을 때 개발자가 아 이 데이터는 NULL이 들어올 수 있는 상황이구나를 래퍼 타입을 사용함으로써 오해를 불러일으킬 수 있다고 생각했어요.

성능 이슈는 아마 발생하지 않을 것 같고, 래퍼 타입 사용에 대한 일관성이

요청 DTO 2. 응답 DTO 3. 엔티티
세 가지 중 어디까지 영향을 미치는 지에 대해 논의해봅시다 ! 👍

Mingyum-Kim · 2024-07-20T04:37:40Z

backend/src/main/java/mouda/backend/moim/service/MoimService.java

+		Moim moim = moimCreateRequest.toEntity();
+		moim.initCurrentPeople();


서비스에서 엔티티의 필드를 일일이 초기화하고 있는 것은 객체의 캡슐화 관점에서 좋지 않은 것 같습니다 😄
MoimCreateRequest.toEntity() 에서 currentPeople을 초기화하는 건 어떨까요?

서비스에서 엔티티의 필드를 일일이 초기화하고 있는 것은 객체의 캡슐화 관점에서 좋지 않은 것 같습니다 😄 MoimCreateRequest.toEntity() 에서 currentPeople을 초기화하는 건 어떨까요?

이 부분은 추후에 논의하기로 하고 일단 진행한 부분이라.. 다음주에 자세하게 이야기해봐요!😄

이 부분에 대해 안나에게 짧게 컨텍스트 공유를 해드릴께요!

저, 상돌, 테바랑 잠깐 이야기 해보고 회의때 말해보자고 했던 내용인데요!

처음 모임을 만들 때 참여인원이 1이라는 것은 엄연한 우리 서비스의 특징이고 비지니스 로직인 것 같다! 때문에 Dto에 이러한 로직이 들어있는게 마음에 들지 않는다!

안나 말대로 엔티티의 필드를 서비스에서 매번 초기화 하는 것은 캡슐화 관점에서 어긋난다!

이 두 가지 관점에서 회의 때 이야기해보면 좋을 것 같아요!

이 부분에 대해 안나에게 짧게 컨텍스트 공유를 해드릴께요!

저, 상돌, 테바랑 잠깐 이야기 해보고 회의때 말해보자고 했던 내용인데요!

처음 모임을 만들 때 참여인원이 1이라는 것은 엄연한 우리 서비스의 특징이고 비지니스 로직인 것 같다! 때문에 Dto에 이러한 로직이 들어있는게 마음에 들지 않는다!

안나 말대로 엔티티의 필드를 서비스에서 매번 초기화 하는 것은 캡슐화 관점에서 어긋난다!

이 두 가지 관점에서 회의 때 이야기해보면 좋을 것 같아요!

설명 감사합니드앗!👍

좋습니다아아앗
고마워요 스피드 호기

pricelees

다들 주말에도 바쁘시군요😅
일단 남겨두신 의견들에 대해 답은 다 남겨놨고.. 이 부분은 다음주에 다시 보고 같이 마무리 해봅시다!

pricelees · 2024-07-20T06:03:35Z

backend/src/main/java/mouda/backend/config/DateTimeFormat.java

+@ConfigurationProperties("datetime.format")
+@RequiredArgsConstructor
+@Getter
+public class DateTimeFormat {


(반영하지 않아도 좋습니다!)

현재 yml 파일에서 포맷을 관리하는데 이 클래스가 필요한 이유가 무엇인가요?? JacksonConfig에서 objectMapper를 빈으로 등록할 필요 없이 @Value로 값을 가져오는 방법도 깔끔할 것 같아요! pr에 남겨주신 것처럼 상수로 관리하는 것도 좋을 것 같습니다 👍

간단하게만 말씀드리면 계층 구조인 설정 값을 매핑하기 유리함 / final 지정 가능 / 타입 안정성 / 일일이 Value 안 붙여도 됨 / Bean Validation 등등 여러 장점도 있지만.. 객체로 값을 관리한다라는게 가장 큰 장점인 것 같네요😄

pricelees · 2024-07-20T06:04:00Z

backend/src/main/java/mouda/backend/moim/controller/MoimController.java

+	public ResponseEntity<RestResponse<MoimDetailsFindResponse>> findMoimDetails(@PathVariable Long moimId) {
+		MoimDetailsFindResponse moimDetailsFindResponse = moimService.findMoimDetails(moimId);
+
+		return ResponseEntity.ok().body(new RestResponse<>(moimDetailsFindResponse));
 	}

 	@Override
 	@PostMapping("/join")
 	public ResponseEntity<RestResponse<Void>> joinMoim(@RequestBody MoimJoinRequest moimJoinRequest) {


발견 👀

감사합니닷👍

pricelees · 2024-07-20T06:14:55Z

backend/src/main/java/mouda/backend/moim/domain/Moim.java

+	private Integer currentPeople;

-	private int maxPeople;
+	private Integer maxPeople;


저희가 나눈 컨벤션 회의 결론을 파라미터를 래퍼 타입으로 통일한다라고 이해했어요. Id는 NULL일 가능성이 있기 때문에 도메인에서도 Long 타입으로 작성하였어요. 그래서 컨트롤러 파라머티에서도 Long으로 가져왔다고 생각하는데, currentPeople과 maxPeople이 NULL인 상황이 있을까요? 🤔 이 부분에 대해서 같이 고민해보면 좋을 것 같습니다!

파라미터를 래퍼 타입으로 통일한다면 모임을 생성할 때 지정하는 maxPeople 값 역시 포장하는 것으로 이해했습니다😄

(지극히 개인적인 생각임)
currentPeople 값이 NULL일 가능성이 지금 상황에서는 없다고 생각되나 개인적으로는 maxPeople값을 포장했으면 연관된 currentPeople값도 포장하는 것이 더 일관된다고 생각해요!

pricelees · 2024-07-20T06:21:45Z

backend/src/main/java/mouda/backend/moim/dto/request/MoimCreateRequest.java

@@ -10,10 +10,11 @@ public record MoimCreateRequest(
 	LocalDate date,
 	LocalTime time,
 	String place,
-	int maxPeople,
+	Integer maxPeople,


[질문] 프론트엔드 측에서 maxPeople 값을 입력해주지 않으면 이 부분이 NULL로 들어오게 되는건가요?

이 부분은 어떻게 처리하는지에 따라 달라져요. 기본적으로는 Json에 maxPeople 필드가 없거나 빈 문자열이 들어오면 NULL이 입력되고, 숫자 이외의 값이 입력되면 HttpMessageNotReadableException이 발생합니다😀

[의견] 추후에 Bean Validation 과 LocalTime, LocalDate 포맷팅 에러를 핸들링하는 것도 필요하겠네요 !

조아요!👍

pricelees · 2024-07-20T06:29:37Z

backend/src/main/java/mouda/backend/moim/dto/response/MoimFindAllResponse.java

@@ -8,16 +8,17 @@

 @Builder
 public record MoimFindAllResponse(
-	long moimId,
+	Long moimId,


요청 DTO에는 null 처리를 위해서 래퍼 타입을 사용하는 것은 맞지만

응답 DTO에서는 moimId가 null 인 상황이 있을까요? 앞서 엔티티에 래퍼 타입을 사용하는 것과 유사한 맥락의 의견입니다! 저는 null 일 수 없는 데이터라면 원시 타입을 사용해야한다고 생각해요.

(지극히 개인적인 관점)
원시 타입의 사용은 성능상의 이점이 있다는 이유가 크겠죠? 이 부분을 고려하지 않은 것은 아니지만 지금의 프로그램에서는 성능상의 이점보다 코드의 일관성이 더 큰 장점이라고 생각했어요. 원시 타입으로 바꾸는 것은 크게 어렵지 않지만.. 지금 단계에서 지정하는 것 보다 추후에 성능 이슈가 발생했을 때 바꿔보고 개선되는 것을 직접 확인하는 과정이 더 의미있다고 생각했습니다!

pricelees · 2024-07-20T06:30:40Z

backend/src/main/java/mouda/backend/moim/service/MoimService.java

+		Moim moim = moimCreateRequest.toEntity();
+		moim.initCurrentPeople();


서비스에서 엔티티의 필드를 일일이 초기화하고 있는 것은 객체의 캡슐화 관점에서 좋지 않은 것 같습니다 😄 MoimCreateRequest.toEntity() 에서 currentPeople을 초기화하는 건 어떨까요?

이 부분은 추후에 논의하기로 하고 일단 진행한 부분이라.. 다음주에 자세하게 이야기해봐요!😄

pricelees · 2024-07-20T06:35:50Z

컨벤션에 맞추는 것을 생각하다보니.. 테스트 클래스명이 다른것은 일단 수정했어요!

hoyeonyy · 2024-07-20T12:53:42Z

backend/src/test/java/mouda/backend/config/JacksonConfigTest.java

+import mouda.backend.moim.repository.MoimRepository;
+
+@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
+class JacksonConfigTest {


테스트가 꼼꼼해서 행복하네요

상돌 폼 미쳤다

상돌 미쳤다

hoyeonyy

안나랑 테니가 얘기 해볼 것들을 많이 적어놓은 것 같아서 다 같이 의논해보고 정해보시죠!

고생하셨습니다 형뉨~

pricelees · 2024-07-22T08:19:23Z

오늘 회의에서 결정된 대로 수정해서 다시 올려요 ~ 검토 부탁드립니닷!😄

Mingyum-Kim

멋진 코드에요 !
수고하셨습니다 🫡

ay-eonii

좋아요~👍

pricelees added 11 commits July 18, 2024 16:59

style: 클래스 선언부 아래 개행 추가

d499e8f

refactor: 반환 타입 void를 ResponseEntity<Void> 타입으로 수정

ee33702

style: return문 위 개행 추가

6f9192e

style: 모든 id 변수명을 moimId로 수정

31b0447

refactor: 모든 원시타입 변수 포장

2a5587a

refactor: 원시값 포장에서 발생하는 NPE 해결

7007393

- Moim.currentPeople이 null로 초기화되는 문제 해결

refactor: 테스트 Assertion에서 Service가 아닌 Repository를 사용하도록 수정

9102746

- service에서 반환된 dto를 이용하는게 아닌, repository에서 반환된 domain을 이용하여 테스트

refactor: 메서드 파라미터로 쓰이는 service 코드를 별도의 변수로 추출

a50b08f

refactor: 요청 및 응답에 사용되는 날짜 및 시간 형식 통일 및 테스트 추가

898a780

merge: 최신 상태 반영

23ff318

refactor: 반환 타입 수정 및 원시타입 변수 포장

c52c39b

pricelees requested review from Mingyum-Kim, ksk0605, ay-eonii and hoyeonyy July 19, 2024 06:27

pricelees added BE 백엔드 관련 이슈입니다. ⚒️ 리팩터링 refactor (기능이 변경되지는 않지만 코드를 수정) labels Jul 19, 2024

ay-eonii requested changes Jul 19, 2024

View reviewed changes

Mingyum-Kim reviewed Jul 20, 2024

View reviewed changes

refactor: 반환 타입 수정

10b4f6e

pricelees commented Jul 20, 2024

View reviewed changes

hoyeonyy reviewed Jul 20, 2024

View reviewed changes

hoyeonyy requested changes Jul 20, 2024

View reviewed changes

pricelees added 5 commits July 22, 2024 16:25

Merge branch 'develop-backend' into refactor/#73

1d99f69

refactor: property에 지정한 날짜, 시간 포맷 제거

237f9d6

refactor: 도메인에서의 wrapper type 언박싱 및 빌더 사용 생성자 추가

aa54ad6

refactor: 응답에서의 wrapper type 언박싱

3d6a9d6

style: 코드 컨벤션 통일

db12372

refactor: 테스트에서의 Optional 처리 수정

31a432c

Mingyum-Kim approved these changes Jul 22, 2024

View reviewed changes

hoyeonyy approved these changes Jul 22, 2024

View reviewed changes

ay-eonii approved these changes Jul 22, 2024

View reviewed changes

pricelees merged commit 739202a into develop-backend Jul 23, 2024
1 check passed

Mingyum-Kim mentioned this pull request Jul 23, 2024

서비스 테스트 시 레포지토리에서 잘못 호출되는 메서드를 수정 #99

Closed

1 task

ss0526100 deleted the refactor/#73 branch July 27, 2024 13:01

	public ResponseEntity<RestResponse<Void>> joinMoim(@RequestBody MoimJoinRequest moimJoinRequest) {
	public ResponseEntity<Void> joinMoim(@RequestBody MoimJoinRequest moimJoinRequest) {

		Moim moim = moimCreateRequest.toEntity();
		moim.initCurrentPeople();

날짜 / 시간 및 기타 코드 컨벤션 통일 #78

날짜 / 시간 및 기타 코드 컨벤션 통일 #78

Conversation

pricelees commented Jul 19, 2024 • edited Loading

PR의 목적이 무엇인가요?

이슈 ID는 무엇인가요?

설명

질문 혹은 공유 사항 (Optional)

구현 방법

논의하고 싶은 사항

ay-eonii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mingyum-Kim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mingyum-Kim Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pricelees Jul 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mingyum-Kim Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

pricelees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pricelees Jul 20, 2024 • edited Loading

Choose a reason for hiding this comment

pricelees commented Jul 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoyeonyy left a comment

Choose a reason for hiding this comment

pricelees commented Jul 22, 2024

Mingyum-Kim left a comment

Choose a reason for hiding this comment

ay-eonii left a comment

Choose a reason for hiding this comment

pricelees commented Jul 19, 2024 •

edited

Loading

Mingyum-Kim Jul 21, 2024 •

edited

Loading

pricelees Jul 20, 2024 •

edited

Loading

Mingyum-Kim Jul 21, 2024 •

edited

Loading

pricelees Jul 20, 2024 •

edited

Loading