whatever-mentoring / AKKIN-BE

아낀거지 백엔드

0 stars 0 forks source link

1차 피드백 #12

Closed 70825 closed 1 year ago

70825 commented 1 year ago

1. build.gradle

[1] OpenFeign

implementation 'org.springframework.cloud:spring-cloud-starter-openfeign

이거는 HttpURLConnection, RestTemplate, OpenFeign, WebClient가 있는데 이중에 OpenFeign을 사용한 이유가 무엇인가요?

[2] JWT

implementation 'io.jsonwebtoken:jjwt-jackson:0.11.5'

이거는 jjwt의 gradle 항목을 보면 3개의 의존성이 추가하라는데, 이거 하나만 사용하는 이유가 있나요?

[3] Jackson

implementation 'com.fasterxml.jackson.core:jackson-databind:2.14.2'

이거도 추가한 이유가 있나요?

[4] MySQL, H2

implementation 'mysql:mysql-connector-java:8.0.33'
runtimeOnly 'com.h2database:h2'

이거는 implementation, runtimeOnly, testImplementation, testRuntimeOnly에 대해 검색해보고, 다시 적용해보면 좋을 것 같아요~

[5] 들여쓰기

    implementation 'mysql:mysql-connector-java:8.0.33'

    compileOnly 'org.projectlombok:lombok'
    annotationProcessor 'org.projectlombok:lombok'
    testCompileOnly 'org.projectlombok:lombok'
    testAnnotationProcessor 'org.projectlombok:lombok'

    runtimeOnly 'com.h2database:h2'

이거 확인점요

2. 코드

[1] Create, Read, Update, Delete 각각으로 나누어진 컨트롤러들

현재 상태에서 굳이 나눌 필요가 있을까요??

[2] 전체 컨트롤러

컨트롤러 코드들을 다 확인해보니까 더 좋은 방법이 있을텐데, HttpServletRequest를 파라미터로 받는 이유가 있나요?

[3] 어노테이션 컨벤션

따로 지정된게 있나요? 순서가 서로 달라서 안정했으면 정하는게 좋아보입니다.

GulbiUpdateController

@RequiredArgsConstructor
@RequestMapping("/api/gulbis")
@RestController

LoginController

@RequiredArgsConstructor
@RestController
@RequestMapping("/api")

MemberService

@RequiredArgsConstructor
@Transactional(readOnly = true)
@Service

GulbiUpdateService

@RequiredArgsConstructor
@Service

GulbiCreateService

@RequiredArgsConstructor
@Transactional
@Service

[4] API 응답

지금 모든 요청에 대한 성공 응답이 200 OK로 보내질 것 같은데 맞나요?

그리고 이것도 응답 부분 코드 컨벤션이 중구난방이라 확인점요

[5] `@Transactional`

GulbiCreateService에 클래스 위에 @Transactional가 붙여있고, 메서드에서도 @Transactional이 붙어있네요

[6] 접근 제어자

Service에 있는 접근 제어자들 제대로 된건지 확인 요청이요

한 곳에서 private가 되어야 하는데, public으로 되어 있어서 다른 곳도 전부 확인해보면 좋아보입니다

[7] `@Query`

이거 쿼리문 보기 힘든데, SELECT - FROM - WHERE 이렇게 줄바꿈으로 해서 나눠주세요. AND도 줄바꿈해서 구분하면 더 읽기 편할 것 같아요

[8] `@EntityListeners(AuditingEntityListener.class)`

제가 알기론 이걸 적용하면 Application.java에 어노테이션 하나 추가해야 동작하는 걸로 아는데 확인점요

brorica commented 1 year ago

피드백 감사합니다. 몇 가지 사항은 현재 발행한 이슈 해결하는대로 답글 달겠습니다.

build.gradle

[1] OpenFeign

현재 애플 로그인 부분에서 파싱 오류를 겪고 있어, 다른 분들의 코드를 기반으로 문제를 해결하려 하는 과정에서 도입했습니다. docs와 내부 코드를 분석한 결과 현재 목적에선 예상되는 이슈가 없다 생각했습니다.

로그인 이슈가 해결되면 해당 의존성은 제거하고 코드 레벨로 해결할 계획입니다. 해당 의존성을 추가하면 애플 로그인 관련 코드가 간결해진단 장점이 있지만, 20라인정도 더 줄이자고 외부 의존성을 추가하는건 좋은 방향은 아니라고 생각합니다.

[2] JWT

나머지 의존성 넣는걸 깜빡 했습니다. 해당 부분은 fix/apple-login 브랜치에 반영해놨습니다. 현재 해당 브랜치에서 JWS 파싱 문제가 발생하고 있기 때문에 버전의 문제인가 싶어서 잠깐 없앤 상태입니다.

[3] Jackson

소셜 로그인을 하는 과정에서 JSON Response를 객체에 바인딩하기 위해 추가했습니다. 그런데 애플 로그인이 기존에 구현해왔던 소셜 로그인하고 달라서 필요 없다고 판단되면 제거할 계획입니다.

[4] MySQL, H2

한번 알아보고 필요한 것만 적용하겠습니다. 감사합니다.

[5] 들여쓰기

이 부분은 다음 PR 떄 반영하겠습니다. 왜 저렇게 됐는지 저도 모르겠네요.. 언제 저렇게 된 거지

코드

[1] Create, Read, Update, Delete 각각으로 나누어진 컨트롤러들

오픈소스 프로젝트를 하는 과정에서 특정 메소드 위아래로 관련없는 기능들이 있으면 스크롤 조정하느라 가독성과 집중력이 떨어지는 문제를 겪었습니다. 그래서 이번 프로젝트를 진행할 때 기능별로 패키지를 나눈다면 관리 포인트가 감소할 것이라 생각했습니다. 물론 필요 이상으로 파일이나 패키지가 생기는 문제가 있지만, 찾고자 하는 코드를 바로 볼 수 있어서 저는 이렇게 나눈 것도 괜찮다고 생각합니다.

[2] 전체 컨트롤러

인증 토큰과 매칭되는 유저 정보를 가져오는 aop코드에서 처리하고 있어서 이걸 컨트롤러 메소드 단에서 가져올려면은 request attribute를 통해서 가져와야 한다 생각을 했습니다. 하지만, 피쳐가 추가될수록 모든 메소드에 HttpServletRequest 매개변수가 1개씩 더 추가되는 문제가 있을 것이어서 더 나은 방법이 있을 거라 생각은 하고 있습니다. 관련 키워드를 받을 수 있을지 궁금합니다.

[3] 어노테이션 컨벤션

이 부분 ctrl c + v로 통일하고 있었는데 빠진게 있었나 봅니다.. 다음 PR 떄 반영하겠습니다.

[4] API 응답

네 현재 모든 요청에 대해 검증을 통과하면 200OK를 반환하는게 맞습니다.

저도 이 부분에 대해서 어떻게 할지 고민했습니다만 현재로선 높은 우선순위를 가지지 않는다고 생각해 간단하게 처리했습니다.

현재 규모를 생각해보면 비즈니스 로직 상에서 발생할 수 있는 예외들은 adviceController 또는 exception 메시지로도 충분히 커버가 가능하기에 이 부분에 대한 자원을 많이 투자하지 않았습니다.

응답코드 컨벤션은같은 경우는 어떤게 더 괜찮은지 확인하기 위해서 여러 방향으로 작성했습니다. 이 부분도 방향이 결정되면 통일할 예정입니다.

[5] @Transactional

이 부분은 ctrl c + v 하면서 실수한것 같습니다. 다음 PR 떄 반영하겠습니다.

[6] 접근 제어자

이 부분도 ctrl c + v 하면서 실수한것 같습니다. 다음 PR 떄 반영하겠습니다.

[7] @Query

좋은 의견입니다. 다음 PR 때 반영하겠습니다.

[8] @EntityListeners(AuditingEntityListener.class)

2가지 방법으로 처리할 수 있는걸로 압니다. 하나는 말씀해주신 것처럼 Application.java에 쓰는 방법이 있고 다른 하나는 Configuration 파일을 만드는 것입니다. config 패키지에 JpaAuditingConfig 파일이 그 예입니다.

원래는 Application.java에 작성할 예정이었지만, 따로 커스텀할 상황이 생기지 않을까 싶어서 JpaAuditingConfig 로 분리했습니다.

70825 commented 1 year ago

[3] Jackson 소셜 로그인을 하는 과정에서 JSON Response를 객체에 바인딩하기 위해 추가했습니다. 그런데 애플 로그인이 기존에 구현해왔던 소셜 로그인하고 달라서 필요 없다고 판단되면 제거할 계획입니다.

해당 의존성을 제거해보고, 인텔리제이 External Libraries에서 jackson-databind가 존재하는지 확인해주세요

[1] Create, Read, Update, Delete 각각으로 나누어진 컨트롤러들 오픈소스 프로젝트를 하는 과정에서 특정 메소드 위아래로 관련없는 기능들이 있으면 스크롤 조정하느라 가독성과 집중력이 떨어지는 문제를 겪었습니다. 그래서 이번 프로젝트를 진행할 때 기능별로 패키지를 나눈다면 관리 포인트가 감소할 것이라 생각했습니다. 물론 필요 이상으로 파일이나 패키지가 생기는 문제가 있지만, 찾고자 하는 코드를 바로 볼 수 있어서 저는 이렇게 나눈 것도 괜찮다고 생각합니다.

코드에서 읽는 범위는 감소하지만, 패키지 관점에서 보면 아래처럼 관리포인트가 4배나 더 늘어나는 것도 생각해보면 좋을 것 같아요

클래스 파일: 3개 → 12개
패키지: 0개 추가 → CRUD 각각 1개씩해서 총 4개 추가

제가 알기론 이거는 규모가 큰 프로젝트에서만 더이상 유지보수하기 힘들어서 관리할 포인트가 4배로 늘어나더라도 그만큼 장점을 취할 수 있는 순간에 비로소야 전환을 하는 걸로 알고 있어요. 그런데 지금 프로젝트는 대규모 프로젝트가 아니라서 너무 과도한 분리가 아닐까 생각합니다

[2] 전체 컨트롤러 인증 토큰과 매칭되는 유저 정보를 가져오는 aop코드에서 처리하고 있어서 이걸 컨트롤러 메소드 단에서 가져올려면은 request attribute를 통해서 가져와야 한다 생각을 했습니다. 하지만, 피쳐가 추가될수록 모든 메소드에 HttpServletRequest 매개변수가 1개씩 더 추가되는 문제가 있을 것이어서 더 나은 방법이 있을 거라 생각은 하고 있습니다. 관련 키워드를 받을 수 있을지 궁금합니다.

그러면 AOP를 사용해 처리한 이유가 무엇인가요? 제가 AOP로 로그인 구현한 경우가 없어서 따로 더 방법이 있는지 모르겠지만, 방법을 바꿔 인터셉터를 사용하면 어노테이션으로 처리가 가능해서요

[8] @EntityListeners(AuditingEntityListener.class) 2가지 방법으로 처리할 수 있는걸로 압니다. 하나는 말씀해주신 것처럼 Application.java에 쓰는 방법이 있고 다른 하나는 Configuration 파일을 만드는 것입니다. config 패키지에 JpaAuditingConfig 파일이 그 예입니다.

아하 제가 config 패키지를 아예 안들어가서 몰랐네요 👍👍

원래는 Application.java에 작성할 예정이었지만, 따로 커스텀할 상황이 생기지 않을까 싶어서 JpaAuditingConfig 로 분리했습니다.

개인적으로 특별한 이유 없이 가능성만 가지고 따로 관리해야할 클래스를 늘리는건 오버엔지니어링이라고 생각해요 특히 이런 부분은 나중에 커스텀할 상황이 생기면 Application.java에 있던 어노테이션만 제거하면 되는거라 더더욱 여기에 부합한다고 느껴지네요

brorica commented 1 year ago

해당 의존성을 제거해보고, 인텔리제이 External Libraries에서 jackson-databind가 존재하는지 확인해주세요

이런게 있는지 몰랐네요. 그런데 이 방법을 쓰면 다른 컴퓨터에서 작업할 때마다 새로 설정해줘야 하는거 아닌가요? IDE 세팅하기 vs 외부 의존성 추가하기 생각하면 전자가 더 낫긴 하지만, 적용하기 전에 이 부분에 대해서 의견을 조금 더 듣고싶어요.

코드에서 읽는 범위는 감소하지만, 패키지 관점에서 보면 아래처럼 관리포인트가 4배나 더 늘어나는 것도 생각해보면 좋을 것 같아요 클래스 파일: 3개 → 12개 패키지: 0개 추가 → CRUD 각각 1개씩해서 총 4개 추가 제가 알기론 이거는 규모가 큰 프로젝트에서만 더이상 유지보수하기 힘들어서 관리할 포인트가 4배로 늘어나더라도 그만큼 장점을 취할 수 있는 순간에 비로소야 전환을 하는 걸로 알고 있어요. 그런데 지금 프로젝트는 대규모 프로젝트가 아니라서 너무 과도한 분리가 아닐까 생각합니다

저도 그 점에 대해선 동의합니다. 지금도 이건 좀 과하다란 생각이 들긴 해요. 어쩌면 나중에 회사에서 팀이 결정한 것을 따르는게 더 나을 겁니다.

그럼에도 이런 시도를 해보는 것은 이 서비스를 계속 발전시키고 싶고, 그 과정에서 제 수준에선 감당 못할 라인의 코드가 나올 것이라 생각해, 그 때를 대비해 미리미리 연습해보는 것도 좋다고 생각을 했습니다.

매번 CRUD 대로 나누진 않을 거 같아요. 막상 분리해보니 일부 케이스는 여러 기능을 합쳐놔도 그렇게 가독성이 떨어지진 않더라구요. 이 기준에 대한 저 나름의 기준을 잡는 것도 괜찮다고 생각이 듭니다. 다음 PR 때는 일부 코드들은 다시 병합해서 올릴 거에요. 생각만 하고 있다 이런 의견을 받고 나니 생각 정리가 좀 되네요

그러면 AOP를 사용해 처리한 이유가 무엇인가요? 제가 AOP로 로그인 구현한 경우가 없어서 따로 더 방법이 있는지 모르겠지만, 방법을 바꿔 인터셉터를 사용하면 어노테이션으로 처리가 가능해서요

이건 좀 단순한 이유인데... 다른 플젝에서 인터셉터를 사용해봤기 때문에 이번엔 aop를 사용해서 적용해보고 그 과정에서 어떤 문제들이 발생하는지를 좀 알고 싶었습니다. 그동안 aop 에 대해서 개념적으로만 알았지, 이걸 직접 구현해볼 일이 없었습니다. 그래서 이번 기회를 통해 aop를 사용해 기존 인터셉터와 어떤 차이를 가지는지 직접 경험해보고 싶었습니다.

개인적으로 특별한 이유 없이 가능성만 가지고 따로 관리해야할 클래스를 늘리는건 오버엔지니어링이라고 생각해요 특히 이런 부분은 나중에 커스텀할 상황이 생기면 Application.java에 있던 어노테이션만 제거하면 되는거라 더더욱 여기에 부합한다고 느껴지네요

의견을 듣고 보니 문제가 생길 때 해결해도 비용이 크게 달라지진 않을 거 같네요. 이 부분도 다음 PR 때 적용하겠습니다. 좋은 의견 주셔서 감사합니다.

70825 commented 1 year ago

이런게 있는지 몰랐네요. 그런데 이 방법을 쓰면 다른 컴퓨터에서 작업할 때마다 새로 설정해줘야 하는거 아닌가요? IDE 세팅하기 vs 외부 의존성 추가하기 생각하면 전자가 더 낫긴 하지만, 적용하기 전에 이 부분에 대해서 의견을 조금 더 듣고싶어요.

이거는 따로 추가할 수도 있겠지만, 의존성을 추가하면서 같이 딸려오는 것들도 모두 확인할 수 있는 곳이에요. 저도 살면서 여기서 따로 추가해본적은 없어요 build.gradle에 해당 의존성을 제거해보고 확인해보면 파악할 것 같습니다

저도 그 점에 대해선 동의합니다. 지금도 이건 좀 과하다란 생각이 들긴 해요. 어쩌면 나중에 회사에서 팀이 결정한 것을 따르는게 더 나을 겁니다. 그럼에도 이런 시도를 해보는 것은 이 서비스를 계속 발전시키고 싶고, 그 과정에서 제 수준에선 감당 못할 라인의 코드가 나올 것이라 생각해, 그 때를 대비해 미리미리 연습해보는 것도 좋다고 생각을 했습니다. 매번 CRUD 대로 나누진 않을 거 같아요. 막상 분리해보니 일부 케이스는 여러 기능을 합쳐놔도 그렇게 가독성이 떨어지진 않더라구요. 이 기준에 대한 저 나름의 기준을 잡는 것도 괜찮다고 생각이 듭니다. 다음 PR 때는 일부 코드들은 다시 병합해서 올릴 거에요. 생각만 하고 있다 이런 의견을 받고 나니 생각 정리가 좀 되네요

이건 좀 단순한 이유인데... 다른 플젝에서 인터셉터를 사용해봤기 때문에 이번엔 aop를 사용해서 적용해보고 그 과정에서 어떤 문제들이 발생하는지를 좀 알고 싶었습니다. 그동안 aop 에 대해서 개념적으로만 알았지, 이걸 직접 구현해볼 일이 없었습니다. 그래서 이번 기회를 통해 aop를 사용해 기존 인터셉터와 어떤 차이를 가지는지 직접 경험해보고 싶었습니다.

이 부분은 이 프로젝트를 어떻게 써먹을까에 따라 달라질 것 같네요 코드단까지 확인하는 회사에서 이거 프로젝트 이야기하면 탈탈 털릴지도