Skip to content

7월 BE 포스팅 #290

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Oct 31, 2023
Merged

7월 BE 포스팅 #290

merged 7 commits into from
Oct 31, 2023

Conversation

@pasly0920
Copy link
Member

@pasly0920 pasly0920 commented Jul 19, 2023

No description provided.

@hoqn hoqn added the 🙋‍♂️🙋‍♀️ 리뷰 요청 PR 리뷰를 요청합니다~ label Jul 21, 2023
@hoqn hoqn requested review from iju1633 and seunghb320 July 21, 2023 08:59
iju1633
iju1633 reviewed Jul 21, 2023
View reviewed changes
_posts/2023-07-19-oop-principle.md Outdated
Comment on lines 27 to 35
> 규칙 1: 한 메서드에 오직 한 단계의 들여쓰기(indent)만 한다.
규칙 2: else 예약어를 쓰지 않는다.
규칙 3: 모든 원시값과 문자열을 포장한다.
규칙 4: 한 줄에 점을 하나만 찍는다.
규칙 5: 줄여쓰지 않는다(축약 금지).
규칙 6: 모든 엔티티를 작게 유지한다.
규칙 7: 3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.
규칙 8: 일급 콜렉션을 쓴다.
규칙 9: 게터/세터/프로퍼티를 쓰지 않는다
Copy link

@iju1633 iju1633 Jul 21, 2023

Choose a reason for hiding this comment

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

스크린샷 2023-07-21 오후 9 41 03 위 사진과 같이 줄바꿈이 반영되어 있지 않습니다. 또한, 규칙 9에 여타 다른 규칙과 동일하게 `.`으로 마무리하는 건 어떨까요?

iju1633
iju1633 reviewed Jul 21, 2023
View reviewed changes
_posts/2023-07-19-oop-principle.md Outdated
Comment on lines 218 to 219
예를 들어보자면 위의 코드가 아래로 바뀐 것을 보여주는 것입니다.
아래의 코드는 Map을 wrapping하고 Private 변수로 선언함으로서 Method로만 접근 가능하고 다양한 비즈니스 로직들을 같이 넣을 수 있도록 변경하였습니다.
Copy link

@iju1633 iju1633 Jul 21, 2023

Choose a reason for hiding this comment

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

어미의 통일이 필요합니다.
~니다. -> ~다.

iju1633
iju1633 reviewed Jul 21, 2023
View reviewed changes
_posts/2023-07-19-oop-principle.md Outdated
}
```

위의 코드 또한 어느 정도 정돈된 것으로 보이지만, isUserHaverOnlyOneTeam이나 isTeamNameLeast2Character 같은 validation 부분이 해당 Class 의 최하단에 private function으로 추가되어 기하급수적으로 Class의 길이가 늘어났다.
Copy link

@iju1633 iju1633 Jul 21, 2023

Choose a reason for hiding this comment

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

[Nit] 메서드명이나 일부 영어 표현의 경우 "'"로 감싸 가독성을 높일 수 있습니다.
e.g. isUserHaverOnlyOneTeam -> isUserHaverOnlyOneTeam

iju1633
iju1633 reviewed Jul 21, 2023
View reviewed changes
_posts/2023-07-19-oop-principle.md Outdated
Comment on lines 284 to 285
이 부분은 크게 설명을 드릴 것이 없을 정도로 간단하다.
.을 통해서 자주 내부의 property나 method를 불러와 사용하는 경우가 있다. 이 때 .을 통해 연결을 단 한 번만 하라는 것이다.
Copy link

@iju1633 iju1633 Jul 21, 2023

Choose a reason for hiding this comment

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

여기서 줄바꿈을 하는 건 어떨까요? 추가적으로 .말고 .을 통해 온점이라는 것을 더욱 명시할 수 있을 것 같습니다 !

iju1633
iju1633 requested changes Jul 21, 2023
View reviewed changes
Copy link

@iju1633 iju1633 left a comment

Choose a reason for hiding this comment

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

구체적인 예를 들어 객체지향 생활 체조 9가지 원칙에 대해 설명해주셔서 감사합니다 ㅎㅎ 이번 글을 통해 일급 컬렉션에 대해 더 자세히 알아가네요 !

@pasly0920
Copy link
Member Author

pasly0920 commented Jul 22, 2023

@iju1633 긴 글 읽어주셔서 감사합니다 수정 완료했습니다.

iju1633
iju1633 reviewed Jul 22, 2023
View reviewed changes
_posts/2023-07-19-oop-principle.md Outdated
```

물론 이 코드에서도 불만스러운 부분이 많이 보이긴 한다.
예를 들면 `saveMeetingTeam이라는` naming보다는 `createEmptyMeetingTeam`이라는 naming이 조금 더 직관적이지 않을까 하는 생각이 들고,
Copy link

@iju1633 iju1633 Jul 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

범위 수정 부탁드립니다 ! : saveMeetingTeam이라는 -> saveMeetingTeam이라는

iju1633
iju1633 reviewed Jul 22, 2023
View reviewed changes
_posts/2023-07-19-oop-principle.md Outdated

기존의 코드의 경우 userList Entity Collection을 그대로 드러내고 있었고, 이를 통해 원하는 대로 수정이 가능했다.

이를 `MeetingTeamUsers를` 통해 private collection 일급 콜렉션을 사용하여 직접 접근을 막고, 해당 콜렉션으로부터 의도를 가진 Method를 뽑아내어 사용할 수 있도록 했다.
Copy link

@iju1633 iju1633 Jul 22, 2023

Choose a reason for hiding this comment

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

동일하게 범위 수정 부탁드립니다 ! : MeetingTeamUsers를 -> MeetingTeamUsers를

@iju1633 iju1633 added ✅ 리뷰 완료 한 명 이상이 리뷰를 완료했을 때 🔨 수정 요청 리뷰 과정 중 수정 사항이 생겼을 때 and removed 🙋‍♂️🙋‍♀️ 리뷰 요청 PR 리뷰를 요청합니다~ labels Jul 22, 2023
@pasly0920
Copy link
Member Author

pasly0920 commented Jul 23, 2023

@iju1633 수정 완료했습니다!

@iju1633 iju1633 added 🛠 수정 완료 수정 요청 사항이 모두 해결됐을 때 and removed 🔨 수정 요청 리뷰 과정 중 수정 사항이 생겼을 때 labels Jul 23, 2023
iju1633
iju1633 previously approved these changes Jul 23, 2023
View reviewed changes
Copy link

@iju1633 iju1633 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

seunghb320
seunghb320 suggested changes Jul 24, 2023
View reviewed changes
Copy link

@seunghb320 seunghb320 left a comment

Choose a reason for hiding this comment

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

글을 시작하실 때 높임의 통일을 위해 '제가'보다는 '내가'가 나을것 같습니다. 그 외에는 너무 좋습니다!

@pasly0920
Copy link
Member Author

pasly0920 commented Jul 24, 2023

@seunghb320 수정 완료했습니다 감사합니다!

@SSUHYUNKIM SSUHYUNKIM removed the ✅ 리뷰 완료 한 명 이상이 리뷰를 완료했을 때 label Oct 31, 2023
@SSUHYUNKIM SSUHYUNKIM added 💯 최종 완료 모든 수정과 리뷰가 끝난 후 merge를 기다릴 때 and removed 🛠 수정 완료 수정 요청 사항이 모두 해결됐을 때 labels Oct 31, 2023
@SSUHYUNKIM SSUHYUNKIM merged commit 5901814 into GDSC-University-of-Seoul:master Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iju1633 iju1633 iju1633 left review comments

+1 more reviewer

@seunghb320 seunghb320 seunghb320 requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@pasly0920 pasly0920

Labels

💯 최종 완료 모든 수정과 리뷰가 끝난 후 merge를 기다릴 때

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pasly0920 @iju1633 @seunghb320 @hoqn @SSUHYUNKIM