Skip to content
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.

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

[com8599-issue6] user 회원가입 기능 #22

Open
wants to merge 13 commits into
base: com8599
Choose a base branch
from
Open

Conversation

com8599
Copy link

@com8599 com8599 commented Aug 9, 2021

Issue: #6

작업 내용

  • 유저 회원가입 기능 추가

생성/변경 로직

  • UserBean추가
  • 스웨거추가
  • UserController 추가
  • 에러 코드 추가(패킷결과)
  • result json 처리부분 추가
  • java secure key 관련 추가

개인 코멘트

  • ptmt 이슈 해결을 못했습니다. 이후 해결시 추가 푸쉬 하겠습니다. map>>User객체 변경 후 해결완료되었습니다

jinyoungchoi95 and others added 4 commits August 5, 2021 12:00
- line coverage : 0.7 > 0.5
- method coverage : 1.0 > delete
feat: UserBean, UserController, Swagger, 등 기본 에셋 추가
@com8599 com8599 added the com8599 답이 없다잉의 이슈 label Aug 9, 2021
@com8599 com8599 changed the title [#6] user 회원가입 기능 [com8599-issue6] user 회원가입 기능 Aug 9, 2021
Copy link
Member

@jinyoungchoi95 jinyoungchoi95 left a comment

Choose a reason for hiding this comment

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

바쁘셨을텐데 api 구현 진행하느라 고생하셨습니다 :)

첫 개발 세팅이라 많은 부분이 힘드셨을 텐데 몇가지 코멘트 남겼으니 확인부탁드려요~

import java.util.Map;

@Component
public class UserBean {
Copy link
Member

Choose a reason for hiding this comment

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

UserBean이 하는 역할이 정확하게 어떤 역할일까요?
db에 쿼리를 날리고 결과를 받아오는, UserDAO 혹은 UserRepository라는 네이밍에 더 걸맞은 클래스일 수 있지 않을까요?
@Component 대신에 @Repository 어노테이션에 대해서 알아보시는 것도 좋을 것 같아요 :)

Copy link
Author

Choose a reason for hiding this comment

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

jdbctemplate 객체를 선언, 초기화 하여, jdbctemplate의 객체로부터 쿼리를 날리는 dao 역할을 하고있습니다.
클래스 네임을 dao로 변경하는것이 좋을거같네요 피드백 감사합니다.

return jdbcTemplate.update(query, new PSSetDao.PSSForTripleString(username, email, password));
}

public Map<String, Object> getUsers(String email) { //getUsers
Copy link
Member

Choose a reason for hiding this comment

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

Map을 그대로 반환하는 것보다는 User라는 도메인 객체에 담아서 보내는 것이 좋지 않을까요?

모든 상황에서 객체를 반환하는 것이 좋다라는것은 단정할 수 없지만 Map을 반환하는 경우에는 항상 주석을 통해서 해당 Map이 어떤 값을 보내는지, 어떤 정보를 나타내는지를 표시하는 것이 중요해요. (다른 사람과의 협업을 위해서요)
그래서 간단하게 보내는 정보에 대해서는 Map을 반환하면서 주석을 통해서 정보를 나타내는 경우도 있지만 User는 자주 사용할 도메인이지 않을까요? 항상 주석을 통해서 남기는 것보다는 도메인 객체를 추가하는 방향에 대해서 고민해보는 것도 좋을 것 같아요 :)

Copy link
Author

Choose a reason for hiding this comment

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

User객체 추가해서 새로 커밋했습니다 피드백감사합니다!

@@ -0,0 +1,31 @@
package com.study.realworld.common;

Copy link
Member

Choose a reason for hiding this comment

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

코드가 어떻게 커밋되었는지는 모르겠지만 항상 코드를 작성하고 커밋하기 전에는 컨벤션 확인 부탁드려요 :)

  • mac: ⌥⌘L
  • windows: Ctrl+Alt+L

Copy link
Author

Choose a reason for hiding this comment

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

컨벤션에 대해 무지했었는데 알게 되어서 너무 신기합니다 감사합니다!!

Comment on lines 30 to 51
public static String getZipJson( String bin ) {
JsonObject json = new JsonObject();

json.addProperty("Z", bin);

return json.toString();
}

public static class RseBinJson {
JsonObject R;
}

public static String getBinJson(JsonObject result, Key key) {
RseBinJson rse = new RseBinJson();
rse.R = result;

JsonObject json = new JsonObject();

json.addProperty("B", SecurityFunc.encryptData(SecurityFunc.AES_ALGORITHM, key, rse));

return json.toString();
}
Copy link
Member

Choose a reason for hiding this comment

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

어디에 사용되는 메소드일까요?

Copy link
Author

Choose a reason for hiding this comment

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

음.. 미래에 필요할거같아서 남겨뒀었는데 관련 파일들 삭제하겠습니다.
감사합니다

Comment on lines 14 to 18
if(objects != null && objects.length > 0) {
for(int i = 0; i < objects.length; i = i + 2) {
json.addProperty(String.valueOf(objects[i]), String.valueOf(objects[i + 1]));
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(objects != null && objects.length > 0) {
for(int i = 0; i < objects.length; i = i + 2) {
json.addProperty(String.valueOf(objects[i]), String.valueOf(objects[i + 1]));
}
}
if(objects == null && objects.length == 0) {
return json.toString();
}
for(int i = 0; i < objects.length; i = i + 2) {
json.addProperty(String.valueOf(objects[i]), String.valueOf(objects[i + 1]));
}

Copy link
Member

Choose a reason for hiding this comment

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

그냥 이렇게 바로 반환하는게 더 맞지 않을까요?

depth는 최대한 2 이하로 맞추는 걸 고려해보아요 :)

Copy link
Author

Choose a reason for hiding this comment

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

헙 감사합니다. 바로 바꾸겠습니다

public class UserController implements ControllerBase{
public UserBean userBean;

@Autowired
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Autowired

생성자 주입의 경우 @Autowired를 제거해도 괜찮아요.

Copy link
Author

Choose a reason for hiding this comment

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

@Autowired 를 사용하지 않을 경우 dao가 null로 보여 객체를 쓸수없게 되어서 넣었는데 틀린 문법인가요?

Copy link
Author

Choose a reason for hiding this comment

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

jdbctemplate외에 다른
@Autowired를 모두 삭제하고 @repository를 붙였습니다.

Copy link
Member

Choose a reason for hiding this comment

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

null로 붙이는게 어떤말씀인지는 잘모르겠어요 :)
생성자 주입으로써

public final UserBean userBean;

public UserController(UserBean userBean) {
    this.userBean = userBean;
}

을 말씀드린 거였습니다
해결하셨다니 좋습니당 :)

@RestController
@RequestMapping("/api")
public class UserController implements ControllerBase{
public UserBean userBean;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public UserBean userBean;
public final UserBean userBean;

Copy link
Author

Choose a reason for hiding this comment

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

static대신 final인자로 변경했습니다 감사합니다.

Comment on lines 30 to 50
JsonObject user = new Gson().toJsonTree(param).getAsJsonObject();
if (userBean.checkSameName(getString((JsonObject) user.get("user"), "username"))) { //닉네임 체크
return Func.getErrorJson(Errors.SAME_NICKNAME);
} else if (userBean.checkSameEmail(getString((JsonObject) user.get("user"), "email"))) { //email 체크
return Func.getErrorJson(Errors.SAME_EMAIL);
}

if ( userBean.registUser(getString((JsonObject) user.get("user"), "username"), getString((JsonObject) user.get("user"), "email"), getString((JsonObject) user.get("user"), "password")) < 0 ) {
//등록하다가 에러났을경우
return Func.getErrorJson(Errors.DB);
}

Map<String, Object> users = userBean.getUsers(getString((JsonObject) user.get("user"), "email"));
if ( users == null ) {
return Func.getErrorJson(Errors.INVALID_REQUEST);
}
JsonObject jsonObject = new Gson().toJsonTree(users).getAsJsonObject();
JsonObject result = new JsonObject();
result.add("user", jsonObject);

return Func.getResultJson(result);
Copy link
Member

Choose a reason for hiding this comment

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

컨트롤러 하나가 가지고 있는 역할이 너무 방대해요.

컨트롤러는 단순히 View와의 통신을 위한 다리라고 생각하고 분리한다면 어떨까요? db에 등록하는 역할이 view와 통신해야하는 컨트롤러가 해야하는 역할이 될 수 있을까요?
Service를 들어보셨을거라고 생각하는데 Service가 Controller의 역할을 조금 더 덜어갈 수 있지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

서비스로 분리 하여 새로 커밋하였습니다 리뷰 감사합니다!

Comment on lines 4 to 5
static final String _upper = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
static final String _upperHex = "0123456789ABCDEF";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static final String _upper = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
static final String _upperHex = "0123456789ABCDEF";
static final String UPPER = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
static final String UPPER_HEX = "0123456789ABCDEF";

상수에 대해서 자바 컨벤션은 항상 대문자 입니다 :)

Copy link
Author

Choose a reason for hiding this comment

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

바뀐 상수들에 대해서 런 이슈있었던 부분들 새로 수정하여 커밋했습니다!

Comment on lines 7 to 8
protected final int _length;
protected final Random _random;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
protected final int _length;
protected final Random _random;
protected final int length;
protected final Random random;

변수명에 언더바를 쓰지 않는 것이 컨벤션입니다 :)

Copy link
Author

Choose a reason for hiding this comment

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

바뀐 상수들에 대해서 런 이슈있었던 부분들 새로 수정하여 커밋했습니다!

com8599 and others added 5 commits August 9, 2021 16:49
Co-authored-by: 최진영 <jinyoungchoi95@gmail.com>
상수에 대한 자바 컨벤션은 항상 대문자 스네이크 이다.

Co-authored-by: 최진영 <jinyoungchoi95@gmail.com>
변수명에는 언더바를 쓰지않는것이 자바컨벤션입니다.

Co-authored-by: 최진영 <jinyoungchoi95@gmail.com>
@@ -0,0 +1,41 @@
package com.study.realworld.bean;

public class User {
Copy link
Member

Choose a reason for hiding this comment

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

User domain 👍
bean 큰 의미보다는 domain이라는 패키지명이 더 어울릴 것 같아요 :)

Copy link
Author

Choose a reason for hiding this comment

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

아하 패키지명 변경하겠습니다! 감사합니다

Comment on lines 15 to 16
@Component
@Repository
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Component
@Repository
@Repository

@Repository 어노테이션을 확인해보시면 @Comonent 어노테이션이 포함되어있어요 :)

Copy link
Author

Choose a reason for hiding this comment

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

헉.. 헐.. 다른 어노테이션인줄알았는데 감사합니다 적용하겠습니다!

public class UserDao {
private final JdbcTemplate jdbcTemplate;

@Autowired
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Autowired

제거하셔도 무방합니다~

Copy link
Author

Choose a reason for hiding this comment

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

헐 구글링했던 코드에는 그대로 되있어서 있어야 도는줄 알고있었네용... 없애고 한번 돌려보고 커밋하겠습니다 감사합니다!

Copy link

Choose a reason for hiding this comment

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

이유를 추가적으로 설명드리자면, 생성자가 1개 있는 컴포넌트의 경우 스프링에서 빈으로 올릴 때 자동으로 삽입하기 때문에 @Autowired라는 키워드가 필요없는 것입니다.

Comment on lines 13 to 14
@Service
@Repository
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Service
@Repository
@Service

UserDao가 @Repository의 역할을 하고 있고 이를 주입받고 있으니 따로 Service에서는 처리하지 않으셔도됩니다~

Copy link
Author

Choose a reason for hiding this comment

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

아하 감사합니다 수정하겠습니다!

Comment on lines 29 to 39
if ( userDao.registUser(getString(user, "username"),
getString(user, "email"),
getString(user, "password")) < 0 ) {
//등록하다가 에러났을경우
return Func.getErrorJson(Errors.DB);
}

User users = userDao.getUsers(getString(user, "email"));
if ( users == null ) {
return Func.getErrorJson(Errors.INVALID_REQUEST);
}
Copy link
Member

Choose a reason for hiding this comment

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

입력받은 user를 저장하고 user를 또 반환을 하는 메소드네요.

저장을 한 user를 db에서 반환하는 이유가 있을까요? 실제로 db에 들어간 것을 또 가져온다는 의미에서 명시적으로 처리한다라고 말할 수 있겠지만 불필요한 행위가될 수 있을 것 같아요.
내가 만든 객체가 db에 저장이 된다면 결국 db에서 가져오는 객체는 내가 만든 객체랑 동일한 값을 가질테니까요.
id가 필요한 상황이라면 다음과 같이 Dao를 변경하는 것도 고려할 수 있을것 같네요 :)

public User registerUser(String username, String email, String password) {
  KeyHolder keyHolder = new GeneratedKeyHolder();
  jdbcTemplate.update(conn -> {
    PreparedStatement ps = conn.prepareStatement("insert into USER(USERNAME, EMAIL, PASSWORD) values (?, ?, ?)", new String[]{"id"});
    ps.setString(1, username;
    ps.setString(2, email);
    ps.setString(3, password);
    return ps;
  }, keyHolder);
  return null;
}

Copy link
Author

Choose a reason for hiding this comment

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

헐 아 헐... update하면서 객체 리턴한다는 건 생각 못했네요!!....
일단은 update중에 db에러가 날 상황도 있을수있으니 해당 코드자체는 바로 머지는 힘들겠지만,
다시 db접근하여 이미 있는 데이터를 다시 불러오는방식은 지양하도록 하겠습니다 감사합니다!

Comment on lines 40 to 42
JsonObject result = new JsonObject();
ObjectMapper objectMapper = new ObjectMapper();
result.addProperty("user", objectMapper.writeValueAsString(users));
Copy link
Member

Choose a reason for hiding this comment

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

json으로 객체를 반환하는 로직이네요.
이전에 말씀드렸다시피 controller에서는 view와의 통신을 담당하는 역할로써 view에서 전달한 json을 객체로 매핑하는, 이를 service에 전달하는, service에서 받은 객체를 다시 view에게 전달하기 위해 객체를 json으로 전달하는 역할을 해요.

지금 로직을 controller에서 담당하도록 하고, Service는 로직이 수행된 객체만을 반환할 수 있는 방법을 생각해보는것이 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

에러 로직을 처리하는부분들까지 같이 수정해서 수행된 객체만을 반환하고싶었으나, 그렇게 되었을 경우 에러 로직에 이슈가 있어 수정하지 못했습니다. 이후에 방법이 새로 생각나면 고치겠습니다 감사합니다!

@repository안에 @component가 있습니다! 주의주의!

Co-authored-by: 최진영 <jinyoungchoi95@gmail.com>
Copy link
Contributor

@chance0523 chance0523 left a comment

Choose a reason for hiding this comment

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

일단 1차 리뷰 드립니당 👍

Comment on lines +3 to +9
import org.springframework.boot.CommandLineRunner;
import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.context.ApplicationContext;
import org.springframework.context.annotation.Bean;

import java.util.Arrays;
Copy link
Contributor

Choose a reason for hiding this comment

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

안 쓰는 import는 지우는 것이 좋습니다.
IntelliJ를 쓰신다면 커밋시에 optimize import에 체크하고 올리시면 됩니다.

스크린샷 2021-08-09 오후 6 19 15

Copy link
Author

Choose a reason for hiding this comment

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

피드백감사합니다 다음 커밋부터 유의하여 커밋하겠습니다!!

@@ -0,0 +1,40 @@
package com.study.realworld.common;

public enum Errors {
Copy link
Contributor

Choose a reason for hiding this comment

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

이건 단수인 Error가 더 좋지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

단수가 맞는거같습니다. 예리한 지적감사합니다!


NOT_FOUND_SESSION_USER(10000, "유저 정보를 찾을 수 없습니다."),

SQLEXCEPTION(-2, "서버와 통신중 에러가 발생했습니다."),
Copy link
Contributor

Choose a reason for hiding this comment

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

위와 동일하게 SQL_EXCEPTION으로 쓰는 것이 좋아보이네요

  • trailing good 👍

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다! 언더바 추가하겠습니다!


import java.security.Key;

public class Func {
Copy link
Contributor

Choose a reason for hiding this comment

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

이 네이밍은 공식 문서 같은 곳에 있는건가요?

Copy link
Author

Choose a reason for hiding this comment

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

따로 공식 문서에 있어서 쓰진 않았습니다. 제가 쓰던 함수들을 그대로 들고 와서 쓴것입니다.
Func라는 클래스 이름이 조금 어색한거같아서 JsonFunc으로 변경하겠습니다!

}

User users = userDao.getUsers(getString(user, "email"));
if ( users == null ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ( users == null ) {
if (users == null) {

로 하는게 좋겠네요. 29 라인두

Copy link
Author

Choose a reason for hiding this comment

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

넵 감사합니다!

return Func.getErrorJson(Errors.DB);
}

User users = userDao.getUsers(getString(user, "email"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
User users = userDao.getUsers(getString(user, "email"));
User user = userDao.getUser(getString(user, "email"));

단건만 가져오니 위에처럼 바꾸는 것이 좋아보이네요

Copy link
Author

Choose a reason for hiding this comment

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

해당 함수는 회원가입단계에서 필요하지 않다 판단하여 삭제하였습니다.
다음에 같은 기능을 넣게 된다면 고려하겠습니다 감사합니다

return (boolean) jdbcTemplate.query(query, new PSSetDao.PSSForString(email), new ResultDao.RSEForBoolean());
}

public int registUser(String username, String email, String password) { //신규 유저 삽입
Copy link
Contributor

Choose a reason for hiding this comment

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

registerUser 로 바꾸는 것이 좋겠네요
regist: 등록
register: 등록하다

Copy link
Author

Choose a reason for hiding this comment

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

다음부터 명사보다는 동사를 쓰도록 하겠습니다!
그리고 슬랙에서 피드백주신대로 register는 '회원가입하다'라는 뜻이 강해 db에 insert하는것이니 insert* 으로 이름짓기로 하였습니다! 감사합니다

this.jdbcTemplate = jdbcTemplate;
}

public boolean checkSameName(String username) { //같은 닉네임의 유저가 있는지 확인
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. checkName()
  2. findByName 같은 식으로 그냥 이름으로 찾아서
    boolean이 아니라 List 같은걸로 반환하는것은 어떨까요?
    반환 받는 곳에서 어차피 에러 던지고 있으니까 거기서 다 판단하는것이 좋아보여요
    아래 Email 도 마찬가지

Copy link
Author

Choose a reason for hiding this comment

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

서비스와 컨트롤러를 분리하여 커밋해보겠습니다. 감사합니다.

Copy link
Member

@DolphaGo DolphaGo left a comment

Choose a reason for hiding this comment

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

제가 모르겠는 코드들이 많네요 @_@
몇 가지만 리뷰 드렸습니다.

runtimeOnly 'com.h2database:h2'
testImplementation 'org.springframework.boot:spring-boot-starter-test'
implementation group: 'com.google.code.gson', name: 'gson', version: '2.7'
implementation 'jakarta.xml.bind:jakarta.xml.bind-api:2.3.3'
Copy link
Member

Choose a reason for hiding this comment

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

자바 객체와 xml을 직렬화/역직렬화 하는 부분이 어디인가요?

Comment on lines +4 to +14
EXCEPTION(-1, "통신중 에러가 발생했습니다."),
NONE(0, "이상없음"),
INVALID_REQUEST(1, "잘못된 요청입니다."),
DB(2, "데이터 베이스 오류입니다."),
SAME_NICKNAME(3, "동일 닉네임유저가 존재합니다."),
SAME_EMAIL(4, "동일 이메일유저가 존재합니다."),

NOT_FOUND_SESSION_USER(10000, "유저 정보를 찾을 수 없습니다."),

SQLEXCEPTION(-2, "서버와 통신중 에러가 발생했습니다."),
;
Copy link
Member

Choose a reason for hiding this comment

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

EXCEPTION 이라는 단어만 보고 통신 중 에러가 발생했는지
SQLEXCEPTION이라는 단어만 보고 서버와 통신 중 에러가 발생했는지 추측하기가 어려운데요.

ENUM 명을 NOT_FOUND_SESSION_USER 처럼 구체적으로 작성하는 것이 좋을 것 같습니다.

for (int i = 0; i < objects.length; i = i + 2) {
json.addProperty(String.valueOf(objects[i]), String.valueOf(objects[i + 1]));
}
System.out.println(errorCode.name() + " - " + errorCode.getCode() + " - " + errorCode.getDesc());
Copy link
Member

Choose a reason for hiding this comment

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

logging이 더 좋을 것 같네요.

this.userService = userService;
}

@ApiOperation(value = "사용자 등록", notes = "사용자 등록")
Copy link
Member

Choose a reason for hiding this comment

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

동일한 내용을 표현하고 있는데, value와 notes를 구분하신 이유는 무엇인가요?

Copy link

@povia povia left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다. 추가적으로 필요한 부분 + 아시면 좋을 내용들로 리뷰를 좀 꾸려봤어요. 힘드시더라도 적용하시면서 어떠한 부분을 고려해야 할지, 더 나은 방향으로 운영/유지보수할 수 있을 지를 고민해보시는 것도 좋아보입니다.
그리고 꼭 테스트 코드 넣어주세요. 개발할 때에는 귀찮아도 테스트 코드가 있고 없고 차이는 어마무시합니다. 내가 개발을 하고 QA -> 릴리즈의 과정에서 나오는 많은 이슈들을 미리 없앨 수 있기 때문에 꼭 필요할 거에요.
다시 한번 너무 고생 많으셨습니다. 해당 부분 + jwt가 사실 이 프로젝트에서 가장 중요하고 힘든 부분이기 때문에 이 부분만 넘기신다면 나머지 부분은 그리 어렵지 않을 겁니다. 조금만 더 힘내봅시다!

Comment on lines +23 to +26
if (userDao.insertUser(user.getUsername(), user.getEmail(), user.getPassword()) < 0) {
//등록하다가 에러났을경우
return ErrorCode.DB;
}
Copy link

Choose a reason for hiding this comment

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

삽입 시 에러가 날 때에는 throw exception을 하는게 좋지 않을까요?

this.userDao = userDao;
}

public Object users(User user) {
Copy link

Choose a reason for hiding this comment

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

아래의 ErrorCode를 반환하는 부분 때문에 Object type으로 반환토록 설정하신 것으로 보이는데 삽입 시 에러가 나면 exception을 던지도록 하고, 정상적인 경우에는 User를 반환하는 형식으로 구조를 잡으시는 게 좋아보입니다

Comment on lines +12 to +14
public UserService(UserDao userDao) {
this.userDao = userDao;
}
Copy link

Choose a reason for hiding this comment

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

불변으로 레포 선언 + 생성자 주입 좋습니다 👍

public class UserDao {
private final JdbcTemplate jdbcTemplate;

@Autowired
Copy link

Choose a reason for hiding this comment

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

이유를 추가적으로 설명드리자면, 생성자가 1개 있는 컴포넌트의 경우 스프링에서 빈으로 올릴 때 자동으로 삽입하기 때문에 @Autowired라는 키워드가 필요없는 것입니다.


public Optional<User> getUserByName(String username) throws EmptyResultDataAccessException { //같은 닉네임의 유저가 있는지 확인
final String query = "select id, email, username, password, bio, image from USER where USERNAME=?";
User user = (User) jdbcTemplate.query(query, new PSSetDao.PSSForStrings(username), new ResultDao.RSEForUser());
Copy link

Choose a reason for hiding this comment

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

파라메터에 Dao를 사용하기 보다는 username을 그대로 매핑하는 것이 좋아보입니다. 추가로 resultset에 대해서는 다음 페이지를 보시는 것도 좋아보여요.
Spring JdbcTemplate Querying Examples

@ApiOperation(value = "사용자 등록", notes = "사용자 등록")
@PostMapping("/users") //post 등록 api
public String users(@JsonProperty("user") User user) throws JsonProcessingException {
Object result = userService.users(user);
Copy link

Choose a reason for hiding this comment

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

변수의 타입에 Object를 사용하는 방식은 좋지 않아 보입니다 ㅜㅜ


@ApiOperation(value = "사용자 등록", notes = "사용자 등록")
@PostMapping("/users") //post 등록 api
public String users(@JsonProperty("user") User user) throws JsonProcessingException {
Copy link

Choose a reason for hiding this comment

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

Input Parameter로 User를 직접 사용하기 보다는, UserDto 등으로 사용하시는 것이 좋아보입니다. request와 response는 Controller 등의 표현 영역에서 받아 서비스 단에 넘기는 시점에 서비스에 필요한 타입으로 변환해서 넘기고, 받는 것이 좋아요.
나중에 DDD를 공부하시게 된다면 세세하게 아실 수 있을 듯 합니다!

Comment on lines +27 to +29
if (result instanceof ErrorCode) {
return JsonFunc.getErrorJson((ErrorCode) result);
}
Copy link

Choose a reason for hiding this comment

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

해당 방식으로 에러를 넘기는 것보다는 throw exception을 처리하도록 하시는게 좋아보여요.
Error Handling for REST with Spring
추천드리는 방식으로 개발을 하실 경우, exception을 따로 관리할 수 있기 때문에 운영, 유지보수 측면에서 이점이 상당히 높아집니다^^

if (result instanceof ErrorCode) {
return JsonFunc.getErrorJson((ErrorCode) result);
}
return JsonFunc.getResultJson(user);
Copy link

Choose a reason for hiding this comment

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

반환 타입으로 String을 사용하시기 보다는 ResponseEntity를 사용하시는 것이 좋아보입니다.


@Configuration
@EnableSwagger2
public class SwaggerConfig {
Copy link

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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
com8599 답이 없다잉의 이슈
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants