코드리뷰 PR by cxxxtxxyxx · Pull Request #42 · I5E1/I5E1-Server

cxxxtxxyxx · 2023-08-17T11:31:59Z

No description provided.

This reverts commit 43c3ff3.

This reverts commit 3e9a5aa.

This reverts commit 82be823.

This reverts commit 913966c.

This reverts commit 170eed4.

This reverts commit d04a58f.

This reverts commit 781bb82.

This reverts commit 4839594.

This reverts commit df5cada.

This reverts commit 14516a9.

…to develop

기존에는 이메일만 일치하면 패스워드와 상관없이 로그인되었음 이를 수정했고, 추후 요청 바디로 전달된 패스워드를 암호화하여 이를 비교하는 기능도 구현할 예정임.

같으면 login하여 토큰 반환, 틀리면 status code 400과 함께 에러메시지 반환

…to develop

MemberService.signUp() 메서드 내에서 builder를 이용해 createAt컬럼을 채우도록 했습니다.

Member.updatedAt에 대한 setter정의했고, password 업데이트 시 encoding하도록 수정했습니다. 또한 MemberController.updateMyPage() 메서드에서 MemberInfoDTO를 data로 응답하도록 수정했습니다.

당직, 연차 예외처리

Member엔티티 생성 및 수정 시 created_at, updated_at 갱신되도록 수정

설정한 default value가 스키마에는 적용되나 실제 값으로는 반영이 안되던 이슈가 있어 해당 필드를 Wrapper클래스로 수정했습니다.

YangSooHyun0 · 2023-08-21T10:42:11Z

안녕하세요. 다른 조는 연차, 당직 테이블 합쳐서 코드 작성했는데 7조도 저희 조 와 똑같이 연차, 당직 테이블을 나눠서 코드 작성해 주셔서 보기가 편했습니다.👍 특히 연차,당직 서비스에서 바로 에러 메시지를 작성해 주신 건 무엇이 잘못되었는지를 즉시 알 수 있으니깐 좋은 것 같습니다. 그러나 앞으로 프로젝트가 커지고 코드의 규모가 커진다면 일관성 유지가 필요하게 될 것 같은데 그때 중앙화된 에러 메시지를 이용하는 것도 방법일 것 같습니다!! 전체적으로 코드가 깔끔해서 잘 봤습니다👏🏻👏🏻👏🏻

YangSooHyun0 · 2023-08-21T10:59:01Z

src/main/java/fc5/i5e1server/domain/duty/DutyCreateReqDTO.java

+import java.time.LocalDateTime;
+
+@Getter
+public class DutyCreateReqDTO {


DutyCreateReqDTO와 DutyUpdateReqDTO 이 유사한 필드 구조를 갖고 있는데 혹시 두개로 나눈 이유가 있을까요?

leaveds5415

씨큐리티보다는 로직 관점에서 리뷰를 드렸어요.

팀의 전체적인 컨벤션을 맞추는 것
각 객체들이 필요한 계층 위치하고, 올바른 의존 방향을 갖는지
도메인 로직은 올바른 곳에 위치하는지

고민해보면 좋을 것 같습니다 🙇

수고하셨어요 👍

leaveds5415 · 2023-08-22T19:55:45Z

src/main/java/fc5/i5e1server/domain/annual/AnnualController.java

@@ -18,8 +16,25 @@ public AnnualController(AnnualService annualService) {
        this.annualService = annualService;
    }


Nit:
의존성 주입을 롬복으로 간단히 할건지, 생성자로 주입을 명시적으로 보여줄건지 통일이 되면 좋을 것 같습니다. 👍

leaveds5415 · 2023-08-22T20:09:23Z

src/main/java/fc5/i5e1server/domain/annual/AnnualActionReqDTO.java

+import lombok.Getter;
+
+@Getter
+public class AnnualActionReqDTO extends AnnualUpdateReqDTO{


중복을 줄이려는 의도인 것으로 이해했어요.
그렇지만, api 에서 RequestBody 를 받기위해서 생성한 객체 ReqDto 와 Annual 을 update 하기 위해 다루는 객체 간에 의존성을 갖게 두는 것은 다시 한 번 생각해보면 좋을 것 같습니다 🙇
또한, Annual 이라는 도메인 객체가 update 하기 위해 받는 updateReqDto 의 네이밍도 고려해보면 좋을 것 같습니다 🙇

leaveds5415 · 2023-08-22T20:10:34Z

src/main/java/fc5/i5e1server/domain/annual/AnnualCreateReqDTO.java

+@Getter
+public class AnnualCreateReqDTO {
+    private LocalDate startDate;
+    private LocalDate endDate;
+    private String summary;
+    private String reason;
+}


validation 이 필요해보여요

leaveds5415 · 2023-08-22T20:14:14Z

src/main/java/fc5/i5e1server/domain/annual/AnnualPageListDTO.java

+public class AnnualPageListDTO {
+    private List<AnnualPageDTO> annuals;
+}


컬렉션을 객체로 Wrapping 하여 Response 만든 것 👍

leaveds5415 · 2023-08-22T20:15:07Z

src/main/java/fc5/i5e1server/domain/annual/AnnualService.java

+        AnnualPageListDTO annualPageListDTO = new AnnualPageListDTO();
+        annualPageListDTO.setAnnuals(pageList);
+        return annualPageListDTO;
+    }


생성자나 정적 팩토리 메소드를 사용하여 생성하는 것도 고려해보면 좋을 것 같습니다.

leaveds5415 · 2023-08-22T20:23:48Z

src/main/java/fc5/i5e1server/domain/member/MemberController.java

+        return APIDataResponse.empty(HttpStatus.OK, memberService.isDuplicateEmail(email) ?
+                "중복된 이메일입니다." :
+                "사용가능한 이메일입니다.");


삼항연산자를 사용하는 것보다는 로직으로 구성해두는 것도 고려해보면 좋을 것 같습니다

leaveds5415 · 2023-08-22T20:27:02Z

src/main/java/fc5/i5e1server/domain/member/MemberUpdateReqDTO.java

+
+    public boolean isTelUpdated() {
+        return tel != null && !tel.trim().isEmpty();
+    }
+
+    public boolean isPasswordUpdated() {
+        return password != null && !password.trim().isEmpty();
+    }


이 부분도 로직에 해당하는 것 같아요.
Dto 내부에서 처리하는게 맞는지 생각해보면 좋을 것 같습니다.

leaveds5415 · 2023-08-22T20:28:57Z

src/main/java/fc5/i5e1server/domain/model/Annual.java

+                    return dayOfWeek != DayOfWeek.SATURDAY && dayOfWeek != DayOfWeek.SUNDAY;
+                }).count();
+
+        return (int) numOfDaysWithoutWeekends;


날짜가 들어가는 값이니 확률은 극히 적겠지만, long 을 int 로 캐스팅하는 것에 대해서 고민해보면 좋을 것 같습니다

leaveds5415 · 2023-08-22T20:31:19Z

src/main/java/fc5/i5e1server/domain/model/Member.java

+@AllArgsConstructor
+@NoArgsConstructor
+@Builder


@entity 로 다루고 있는 JPA Entity에 @AllArgsConstructor 과 @builder 를 한 번에 사용하는 것 vs id 가 빠진 생성자를 직접 만들고 @builder 사용에 어떤 차이가 있는지 생각해보시면 좋을 것 같아요

leaveds5415 · 2023-08-22T20:32:45Z

src/main/java/fc5/i5e1server/domain/util/ServiceUtil.java

+        for (Annual annual : annuals) {
+            if ((tempStartDate.isEqual(annual.getStartDate()) || tempStartDate.isAfter(annual.getStartDate())) &&
+                    (tempStartDate.isEqual(annual.getEndDate()) || tempStartDate.isBefore(annual.getEndDate()))) {
+                return true;
+            }
+            if ((tempEndDate.isEqual(annual.getStartDate()) || tempEndDate.isAfter(annual.getStartDate())) &&
+                    (tempEndDate.isEqual(annual.getEndDate()) || tempEndDate.isBefore(annual.getEndDate()))) {
+                return true;
+            }
+            if ((annual.getStartDate().isEqual(tempStartDate) || annual.getStartDate().isAfter(tempStartDate)) &&
+                    (annual.getStartDate().isEqual(tempEndDate) || annual.getStartDate().isBefore(tempEndDate))) {
+                return true;
+            }
+            if ((annual.getEndDate().isEqual(tempStartDate) || annual.getEndDate().isAfter(tempStartDate)) &&
+                    (annual.getEndDate().isEqual(tempEndDate) || annual.getEndDate().isBefore(tempEndDate))) {
+                return true;
+            }
+        }
+        return false;


이런 로직들은 작은 메소드 단위로 분해해볼 수 있을 것 같아요

cxxxtxxyxx and others added 30 commits July 25, 2023 17:22

[#1] feat: 프로젝트 생성

301a1cb

[#1] feat: Entity 파일 생성 및 연관관계 매핑

325a934

[#3] feat: 공통 응답 클래스 및 글로벌 예외 처리 클래스 생성

10a6db9

[#3] feat: 레포지토리 클래스 생성

6634123

[#3] feat: Admin 엔티티, 레포 클래스 추가

3cca57e

[#3] feat: Annual, Duty 삭제시 제약조건 및 컬럼 추가

b813cb2

[#3] feat: 더미데이터 추가

c10e679

[#3] fix: @DynamicInsert 어노테이션 추가

bc51801

[#3] chore: plain jar 파일 생성하지 않도록 수정

fc9223e

[#3] feat: SecurityConfiguration 파일 생성

ef9820d

[#11] fix: 공통Member, main 수정

030281b

[#11] feat : 마이페이지 조회 구현

8e390ce

[#11] feat : MemberUpdateDTO 생성

213c016

[#11] fix: 공통코드 수정 Member Update 추가

ece9783

[#11] feat: 마이페이지 수정 구현

f81361a

[#13] feat: Annual 클래스 생성

91afde6

[#13] feat: annual 조회 구현

6184ef3

[#13] feat: duty 조회 구현

bcac4d1

[#13] fix: typo 정리

057d900

[#13] feat: 마이페이지 구현 todo 포함, 공통 코드 수정

71c87d4

Revert "[#13] feat: 마이페이지 구현 todo 포함, 공통 코드 수정"

19366e0

This reverts commit 43c3ff3.

Revert "[#13] fix: typo 정리"

ac98c0d

This reverts commit 3e9a5aa.

Revert "[#13] feat: duty 조회 구현"

161e4ea

This reverts commit 82be823.

Revert "[#13] feat: annual 조회 구현"

3055c21

This reverts commit 913966c.

Revert "[#13] feat: Annual 클래스 생성"

27af9bb

This reverts commit 170eed4.

Revert "[#11] feat: 마이페이지 수정 구현"

4849a7b

This reverts commit d04a58f.

Revert "[#11] fix: 공통코드 수정 Member Update 추가"

0fe527e

This reverts commit 781bb82.

Revert "[#11] feat : MemberUpdateDTO 생성"

c5b30a9

This reverts commit 4839594.

Revert "[#11] feat : 마이페이지 조회 구현"

23c3f65

This reverts commit df5cada.

Revert "[#11] fix: 공통Member, main 수정"

050e20f

This reverts commit 14516a9.

brotherstone97 and others added 27 commits August 7, 2023 14:05

[#20] feat: 이메일 중복검사 기능 구현

508e7c6

[#21] feat: annual , duty 날짜 유효성 및 중복 검사 추가

61da641

[#21] fix: typo 정리

b636226

[#26] fix: 로그인된 유저정보 반영

f13e2e8

[#26] fix: 로그인된 유저정보 반영

ea4e5a9

[#26] fix: 로그인된 유저정보 반영

7581409

[#26] fix: 로그인된 유저정보 반영

bb2e0d6

[#26] fix: spentDays 주말 제외 로직 추가

cc9cb23

[#26] fix: typo 정리

6044795

Merge branch 'develop' of https://github.com/Across826/I5E1-Server in…

6bbe297

…to develop

[#26] fix: spentDays 주말 제외 로직 추가

5b7fc23

[#26] fix: typo 정리

0b359fc

[#23] 충돌 해결

9872368

[#23] feat: 요청으로 받은 암호를 인코딩한 결과가 db의 패스워드와 같은지 비교

bb31e43

같으면 login하여 토큰 반환, 틀리면 status code 400과 함께 에러메시지 반환

[#23] fix: AuthController의 return값으로 공통응답 사용하도록 변경

af96f5b

Merge branch 'develop' of https://github.com/Across826/I5E1-Server in…

d1726f0

…to develop

[#28] feat: 중복코드 serviceutil 대체, 연차신청 예외처리

3292e66

[#28] feat: 연차 수정, 당직 신청 수정 예외처리

d67cd19

[#28] fix: typo 정리

596e425

[#34] feat: Member엔티티 생성 시 created_at컬럼에 LocalDateTime.now()값이 들어가도록 함.

cf28543

MemberService.signUp() 메서드 내에서 builder를 이용해 createAt컬럼을 채우도록 했습니다.

Merge pull request #32 from Across826/feature#28

f71906f

당직, 연차 예외처리

Merge branch 'develop' into feature/#34

bc58371

Merge pull request #35 from brotherstone97/feature/#34

3c10479

Member엔티티 생성 및 수정 시 created_at, updated_at 갱신되도록 수정

[#36] feat: tel, password 중 하나만들어오거나, 공란이 들어와도 기존값으로 두고 수정되게 변경

8c39a5f

[#38] fix: Member엔티티의 annualCount를 Wrapper 클래스로 수정

08fe849

설정한 default value가 스키마에는 적용되나 실제 값으로는 반영이 안되던 이슈가 있어 해당 필드를 Wrapper클래스로 수정했습니다.

YangSooHyun0 reviewed Aug 21, 2023

View reviewed changes

leaveds5415 reviewed Aug 22, 2023

View reviewed changes

		@@ -18,8 +16,25 @@ public AnnualController(AnnualService annualService) {
		this.annualService = annualService;
		}

Conversation

cxxxtxxyxx commented Aug 17, 2023

Uh oh!

YangSooHyun0 commented Aug 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leaveds5415 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

YangSooHyun0 commented Aug 21, 2023 •

edited

Loading