member-api many commit #43

loustler · 2016-10-30T14:11:04Z

Commit history

refactor : member api comment and lombok, etc..

lombok 변경 ( chaining 추가 )
@Setter, @Getter 대신 @Data로 변경(DTO)
Social DTO Contructor 추가
주석 추가

feature : member-api service and controller , refactor : member-api 코드 수정

service layer
rest controller layer
테스트는 미실시
Entity <-> DTO를 위한 converter 추가
DTO에 builder pattern 적용(effective java 참조)
domain의 NoArgsConstructor의 access level 수정(public -> protected)

fix: member-api social repository initialize fail exception

method name 수정
method name을entity에 맞게 찾지 못해 exception

refactor : member-api

null check 변경 (Objects.isNull)
test 추가 (domain util Dto Converter)
주석 추가

fix: member api Relation , feature: member-api

Member - Social Relation이 잘못되어 있었던 것 수정 (mappedBy seq에서 member 로 수정)
refactor : Transactional 이동 / Member에서 SocialList empty List로 처리 / Social에서 ManyToOne의 fetch를 EAGER(default)로 수정 / Social의 FK인 member_seq를 FK update 막음 등
test : test 코드 작성
feature : MemberDto의 SocialList를 추가와 add할 수 있게 처리 / List 처리할 수 있게 EntityDtoConverter 처리 등

코드에 대한 전반적인 리뷰가 필요
특히, EntityDtoConverter 같은 경우가 특히 요망

또한 @Transactional 안에서 사용되어야 Exception이 발생하지 않는
fetch = FetchType.EAGER로 default가 되어 있는데
Social과 Member의 각 relation에서 fetch=FetchType.LAZY로 한다면
처리를 어떻게 해야 할지 등의 리뷰가 요망됨.

- lombok 변경 -> chaining 추가 -> `@Setter`, `@Getter` 대신 `@Data`로 변경(DTO) - Social DTO Contructor 추가 - 주석 추가

- service layer - rest controller layer - 테스트는 미실시

- Entity <-> DTO를 위한 converter 추가 - DTO에 builder pattern 적용(effective java 참조) - domain의 NoArgsConstructor의 access level 수정(public -> protected)

- method name 수정 - method name을entity에 맞게 찾지 못해 exception

- null check 변경 -> Objects.isNull로 - test 추가 -> domain util Dto Converter - 주석 추가

- Member - Social Relation이 잘못되어 있었던 것 수정 -> mappedBy seq에서 member 로 수정

- refactor : Transactional 이동 / Member에서 SocialList empty List로 처리 / Social에서 ManyToOne의 fetch를 EAGER(default)로 수정 / Social의 FK인 member_seq를 FK update 막음 등 - test : test 코드 작성 - feature : MemberDto의 SocialList를 추가와 add할 수 있게 처리 / List 처리할 수 있게 EntityDtoConverter 처리 등

loustler · 2016-10-30T14:22:18Z

리뷰 부탁드립니다

수정해야 될 부분이 꽤 많을 것으로 보여집니다

특히

등이 중점이 될 것 같습니다.

1은 Converter가

2,3은 relation의 fetch type이 ..

아무쪼록 리뷰 부탁드립니다 :)

changhwa · 2016-10-30T15:32:58Z

좋네요 낼부터 차근히 리뷰해보죠 👍
추가적으로 원래 커밋은 적절하게 합쳐서 풀리퀘 보내는게 좋긴합니다 :)

loustler · 2016-10-30T23:58:09Z

@changhwa 커밋하기까지 기간도 있고 해서 좀 지저분해 보이긴 하네요 :)
다음부터는 test, feature, refactor 등 각 커밋룰에 맞춰서 할 수 있게 합쳐서 하겠습니다
아, merge로 commit 합치면 되겠죠? :)

changhwa · 2016-10-31T02:06:00Z

@loustler

원래 원칙은 1풀리퀘는 1커밋으로 하는게 좋습니다.
이유는 1풀리퀘 = 하나의 기능 이기 때문에 커밋도 하나로 나오는게 좋겠죠.
상황에 따라 (환경설정이라던가) 기능과 별개만 별도의 커밋으로 분리하면 더 좋고요
다만 이러면 너무 빡빡하니 편하게 주면 될듯하네요.

1커밋이 중요한 이유는 커밋이 여러개인 경우 위의커밋을 리뷰했는데 아래커밋에서 바껴있거나 이러면 혼란스러우니까요..^^

git rebase -i 로 합치면됩니다.

loustler · 2016-10-31T04:16:41Z

@changhwa
앞으로 PR의 취지에 맞게 1PR 1commit으로 해야겠네요
그게 보기도 더 좋을 거고 구분도 더 쉽겠네요 :)

reiphiel · 2016-11-02T14:55:49Z

SocialServiceIntegrationTest#delete_social 테스트에서 익셉션 관련 테스트는
catch를 이용하기 보다는 @test(expected = NullPointerException.class) 와 같이 expected를 이용하는게 깔끔할 듯 합니다.

loustler · 2016-11-02T23:25:29Z

@reiphiel 감사합니다 expected 사용해서 수정해서 다시 commit 하겠습니다

- try - catch 대신 Test의 expected 사용

loustler · 2016-11-03T03:51:33Z

@reiphiel 수정하였습니다.

changhwa · 2016-11-03T12:09:10Z

convert가 너무 많아서 헷갈리네요
굳이 컨버터를 만든 이유가 있으신가요? ModelMapper 같은건 어떠신가요

loustler · 2016-11-03T12:11:19Z

@changhwa
아 그거 안 써보고 한 번 해보려고 했는데
역시나 예상했던대로 좀 난잡하군요
ModelMapper 한 번 보고 써서 다시 커밋날리겠습니다

changhwa

4e30c63 요 커밋은 메세지가 어떤걸 의미하는지 잘 모르겠네요.

changhwa · 2016-11-03T12:12:36Z

src/main/java/io/codechobo/member/application/MemberService.java

@@ -31,7 +34,7 @@
        List<Member> member = memberRepository.findAll();

        return member.stream()
-                .map(m -> new MemberDto(m.getSeq(), m.getId(), m.getPassword(), m.getEmail(), m.getNickName(), m.getRegistrationDate(), m.getPoint()))
+                .map(m -> EntityDtoConverter.memberConvertToDto(m))


굳이 EntityDtoConvert 라는 걸 만드는것 보다는 modelmapper 또는 해당 도메인에 직접 구현하는게 맞지 않을까 싶습니다.

changhwa · 2016-11-03T12:14:52Z

src/test/java/io/codechobo/member/domain/support/SocialDtoIntegrationTest.java

+                            .build();
+
+
+        System.out.println(dto);


개인적으로는 테스트코드는 항상 assert 구문으로 끝나는게 좋다고 생각합니다.

한 번 확인해본다고 해놨는데
일관성 있게 assert로 처리하겠습니다

e197892 에서 assert로 다 수정하고 sysout 삭제했습니다

changhwa · 2016-11-03T12:15:39Z

src/test/java/io/codechobo/member/interfaces/MemberRestControllerIntegrationTest.java

@@ -68,7 +68,7 @@ public void setUp() {

    @Test
    public void 멤버_업데이트_update_member() throws Exception {
-        MemberDto memberDto = new MemberDto("id", "password", "[email protected]", "닉네임");
+        MemberDto memberDto = new MemberDto.Builder().id("id").password("password").email("[email protected]").nickName("닉네임").build();


빌더 패턴을 적용했으면 장점을 활용해야합니다.
이런경우는 @before 에 기본데이터를 필요해주고 필요한 경우에만 추가로 변경해서 build 해주면 됩니다.

changhwa · 2016-11-03T12:16:27Z

src/main/java/io/codechobo/member/domain/repository/MemberRepository.java

@@ -11,6 +11,6 @@
 */
 @Repository
 public interface MemberRepository extends JpaRepository<Member, Long> {
-    Member findByNickName(String nickname);
-    Member findByEmail(String email);
+    Member findByNickName(final String nickname);


final 을 넣으신 이유가 있나요?

굳이 필요없으려나요?
혹시 몰라서 final로 만들어서 변경못하게 하려고 했는데..

일반적으로 Method 파라미터에는 final 은 선언해서 사용하면 실수에 의한 재 대입을 막을 수 있어서 좋은 패턴인데 인터페이스의 메소드는 파라미터를 사용하는 부분이 없어서 의미가 없긴합니다.

굳이 필요없군요
삭제하겠습니다 :)

굳이 삭제를 따로 할 필요도 없어 보입니다^^;; 바이트 코드가 미세하게 증가할려나;; 그건 class파일을 봐봐야 할 것 같고 저는 습관적으로 붙이는 편이라 가끔 인터페이스에도 붙이기도 합니다.
메소드 파라미터에 final을 붙이는 것 자체는 굉장히 좋은 습관입니다.

통일성을 위해서 삭제를 하든지 추가를 하든지 해야될 것 같아서요 ..
final을 파라미터에 붙이는 건 습관을 들이도록 노력해야겠습니다 :)

두 method 모두 final로 처리되어 있어서 그대로 놔두겠습니다

Member findByNickName(final String nickname); Member findByEmail(final String email);

loustler · 2016-11-03T12:22:43Z

@changhwa
4e30c63 커밋메시지가 이상하네요 ;;

- MemberRepository findByMemberSeq 수정 
- NoArgsConstructor 접근제어자 수정

이렇게 했었던 것 같은데;

changhwa

일단 깃허브로 커밋을 대충 훑어본것만 적었구요 주말에 시간될때 한커밋씩 자세히 봐볼게요.

changhwa · 2016-11-03T12:18:49Z

src/main/java/io/codechobo/member/application/SocialService.java

@@ -61,12 +61,9 @@ public SocialDto getSocial(@NotNull final Long socialSequence) {
     */
    @Transactional
    public SocialDto createSocial(@NotNull final SocialDto socialDto) {
-        if(socialDto.getMemberSequence() == null)
-            throw new NullPointerException();
+        Member foundMember = memberRepository.findOne(Objects.requireNonNull(socialDto.getMemberSequence()));


만약 Objects.requireNonNull이 널로 넘어오면 어떤식으로 동작하게 되나요?

changhwa · 2016-11-03T12:20:26Z

src/main/java/io/codechobo/member/application/SocialService.java

@@ -85,8 +82,7 @@ public SocialDto createSocial(@NotNull final SocialDto socialDto) {
     */
    @Transactional
    public SocialDto updateSocial(@NotNull final SocialDto socialDto) {


요거 제대로 동작하나요?
null 넘기는 테스트가 있을까요?

changhwa · 2016-11-03T12:21:37Z

src/test/java/io/codechobo/member/domain/util/EntityDtoConverterIntegrationTest.java

+@RunWith(SpringRunner.class)
+@SpringBootTest
+@ActiveProfiles(value = "test")
+public class EntityDtoConverterIntegrationTest {


요건 의도하신바는 알겠는데 만약 요기능을 제대로 만들고 싶었더라면 종속되지 않도록 만들면 될듯한데
이미 modelmapper가 있으니 ㅎㅎ

이 테스트는 삭제했어요 e197892 에서 비워뒀어요
나중에 Converter를 없앨 때 같이 삭제할 예정이에요

changhwa · 2016-11-03T12:22:14Z

src/main/java/io/codechobo/member/domain/Member.java

@@ -67,7 +66,7 @@
    /*
     * social은 member에 종속적이므로 cascade
     */
-    @OneToMany(fetch = FetchType.EAGER, mappedBy = "seq")
+    @OneToMany


요렇게 하는것도 나쁘진 않지만 가능하면 이게 어떤어떤 동작을 하게 되는지는 적어주는 편이 좋습니다.

changhwa · 2016-11-03T12:23:06Z

src/main/java/io/codechobo/member/domain/Social.java

-    @ManyToOne(optional = false, fetch = FetchType.LAZY)
-    @JoinColumn(name = "MEMBER_SEQ")
+    /**
+     * Transactional 범위밖에서 벗어나서 LAZY에서 EAGER(default)로 수정


Lazy를 써야하는 순간이 어느순간인지 고민해보면 좋을거같네요.

- 테스트코드에서 sysout 삭제 - assert로 통일 - EntityDtoConvert Test empty 처리

devsejong · 2016-11-11T01:11:07Z

src/main/java/io/codechobo/member/application/SocialService.java

+     * @param socialSequence
+     * @return SocialDto
+     */
+    public SocialDto getSocial(@NotNull final Long socialSequence) {


@NotNull이라고 붙여놓았는데, 이게 실제로 동작하는건가요??
제가 기억하기로는 별도의 preProcessor가 있어야지만 동작하는걸로 알고 있어서요./

한 번 확인해보겠습니다
저도 저건 처음 써봤는데 막연하게 동작할 거다
라고 생각하고 말았네요

validator lib같은 게 별도로 있어야지 동작하나보네요.

혼자선 아무것도 못하나봅니다.

제외하든지 @Vaild 등을 사용하든지 하겠습니다.

@NotNull 을 삭제하고
Objects.requireNonNull로 대체했습니다.
dc15595

- \'@NotNull\' 삭제 - Objects.requireNonNull로 대체

reiphiel · 2016-11-20T01:05:12Z

머지 해도 되지 않을까요?

loustler added 7 commits October 22, 2016 17:53

refactor: member api comment and lombok, etc ..

a028c06

- lombok 변경 -> chaining 추가 -> `@Setter`, `@Getter` 대신 `@Data`로 변경(DTO) - Social DTO Contructor 추가 - 주석 추가

feature: member-api sevice and controller

9bc8f2e

- service layer - rest controller layer - 테스트는 미실시

refactor: member-api 코드 수정

9e0b4ba

- Entity <-> DTO를 위한 converter 추가 - DTO에 builder pattern 적용(effective java 참조) - domain의 NoArgsConstructor의 access level 수정(public -> protected)

fix: member-api social repository initialize fail exception

4e30c63

- method name 수정 - method name을entity에 맞게 찾지 못해 exception

refactor : member-api

6de5c78

- null check 변경 -> Objects.isNull로 - test 추가 -> domain util Dto Converter - 주석 추가

fix: member api Relation

2caa9ea

- Member - Social Relation이 잘못되어 있었던 것 수정 -> mappedBy seq에서 member 로 수정

loustler added bug feature refactor test labels Oct 30, 2016

loustler added this to the Sprint 4 milestone Oct 30, 2016

loustler assigned changhwa, devsejong, reiphiel and iamkyu Oct 30, 2016

loustler mentioned this pull request Oct 30, 2016

Member API #29

Open

loustler changed the title ~~Feature/member api~~ member-api many commit Oct 30, 2016

refactor: member-api test code refactor

afd37ba

- try - catch 대신 Test의 expected 사용

changhwa reviewed Nov 3, 2016

View reviewed changes

refactor : member-api remove sysout in test code

e197892

- 테스트코드에서 sysout 삭제 - assert로 통일 - EntityDtoConvert Test empty 처리

devsejong reviewed Nov 11, 2016

View reviewed changes

refactor : @NotNull remove

dc15595

- \'@NotNull\' 삭제 - Objects.requireNonNull로 대체

changhwa merged commit b124022 into develop Nov 20, 2016

member-api many commit #43

member-api many commit #43

Conversation

loustler commented Oct 30, 2016 • edited Loading

loustler commented Oct 30, 2016

changhwa commented Oct 30, 2016

loustler commented Oct 30, 2016

changhwa commented Oct 31, 2016

loustler commented Oct 31, 2016

reiphiel commented Nov 2, 2016

loustler commented Nov 2, 2016 • edited Loading

loustler commented Nov 3, 2016

changhwa commented Nov 3, 2016

loustler commented Nov 3, 2016

changhwa 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

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

loustler commented Nov 3, 2016

changhwa 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

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

reiphiel commented Nov 20, 2016

loustler commented Oct 30, 2016 •

edited

Loading

loustler commented Nov 2, 2016 •

edited

Loading