Skip to content
This repository was archived by the owner on Aug 13, 2022. It is now read-only.

User Domain 코드 Kotlin 변환#79

Open
Habist wants to merge 10 commits intodevelopfrom
feature/78
Open

User Domain 코드 Kotlin 변환#79
Habist wants to merge 10 commits intodevelopfrom
feature/78

Conversation

@Habist
Copy link
Copy Markdown
Collaborator

@Habist Habist commented Jul 15, 2021

작업 내용

User Domain 관련 Java 코드를 Kotlin 코드로 변환하였습니다.

주의 사항

코틀린 컴파일러로 컴파일된 class 파일들을 코틀린 런타임 라이브러리로 읽을 수 있도록
관련 의존성을 추가하였습니다.

참조이슈
#78

Habist added 4 commits July 14, 2021 23:29
Kotlin 사용에 필요한 Maven 의존성 설정 추가하였습니다.
Kotlin 기반 Spring 사용을 위해 코드를 변경하였습니다.
UserController, Service, Mapper, Model Classes 코틀린 코드로 변환하였습니다.
경고가 나지 않도록 코틀린 환경에 맞는 jackson 모듈을 추가했습니다.
@Habist Habist requested a review from f-lab-dev July 15, 2021 14:53
@Habist Habist self-assigned this Jul 15, 2021
Comment on lines +31 to +33
fun getUserInfo(@LoginUserId id: String): UserInfo? {
return userService.getUserInfoById(id)
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

중괄호와 리턴값을 생략하고 =를 써볼 수 있을 것 같네요~

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

리뷰 반영했습니다!

@Select("SELECT id, name, email, telephone FROM USER WHERE id = #{id}")
fun findById(id: String): UserInfo?

@Select("SELECT id, name, email, telephone FROM USER WHERE id = #{id} AND password = #{password}")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

이제 멀티라인 스트링을 쓸 수 있으니 여러 행으로 표현해주면 가독성이 더 높아질 것 같아요~

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

멀티라인으로 수정했습니다!

import javax.validation.constraints.NotBlank
import javax.validation.constraints.Size

abstract class LoginRequest(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

데이터클래스가 아니게 선언하신 이유가 있으실까요?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

LoginRequest는 사용자와 관리자의 로그인 요청에 나타나는 공통적인 부분을 추상화한 추상 클래스이고 클라이언트의 요청을 표현하는 클래스라 Request 관련 클래스는 전부 데이터 클래스로 선언하지 않았습니다.

데이터 클래스로 선언해야 할 이유가 있을까요?

Comment on lines +4 to +7
val id: String? = null,
val name: String? = null,
val email: String? = null,
val telephone: String? = null
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

모든 필드가 nullable인 이유가 있을까요?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

어떠한 상황에는 특정 필드만 필요할 것인데 그 경우에 nullable이 아니면 활용할 수 없을 거라 생각하여 nullable로 선언했습니다.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

특정 필드만 필요할 때에는 그 목적에 맞게 클래스를 따로 선언하는것도 괜찮지 않을까요?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

필요할 때마다 클래스를 따로 선언하면 클래스가 너무 많아져서 관리가 힘들어지지 않을까요?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

그건 트레이드오프 관계인 것 같네요~ 관리포인트를 좀 늘려가면서 입출력에 대한 명확성을 확보할것이냐, 혹은 하나로 통일할것이냐에서요

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

그럼 저는 명확성을 확보하는 쪽으로 가겠습니다 리뷰 반영할게요!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants