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

1. 회원가입 요구사항 Update #1

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

1. 회원가입 요구사항 Update #1

wants to merge 7 commits into from

Conversation

chimaek
Copy link
Contributor

@chimaek chimaek commented May 11, 2021

  1. post http메서드로 회원 data보내기 (정상처리 200)
  2. 중복 회원있을 시 예외처리 (badRequest 400)

Copy link
Member

@aerain aerain left a comment

Choose a reason for hiding this comment

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

더 많은 테스트 코드가 필요합니다.

Comment on lines 18 to 19
public Long save(@RequestBody AccountSaveDto accountSaveDto) {
return accountService.save(accountSaveDto);
Copy link
Member

Choose a reason for hiding this comment

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

표현층의 dto를 응용층에 내려주고 있는데, 이번 경우에는 간단한 요구사항이라 괜찮지만, 분리를 해야할지 고민해야할 구조입니다.


private final AccountRepository accountRepository;

@ResponseStatus(value = HttpStatus.BAD_REQUEST, reason = "잘못된 접근입니다.")
Copy link
Member

Choose a reason for hiding this comment

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

응용층에서 표현층의 책임을 지고있습니다. 응용층에서는 예외를 내려주되, 표현층에서 예외를 핸들링하여 요구사항에 맞는 상태 코드를 부과해야 합니다.

@Transactional
public Long save(AccountSaveDto accountSaveDto) {
String username = accountSaveDto.getUsername();
boolean anyMatch = accountRepository.findAll()
Copy link
Member

Choose a reason for hiding this comment

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

하지말아야할 장애포인트입니다. findAll()은 시간, 공간 복잡도가 O(n)인 데다, 한번에 다량의 데이터를 애플리케이션 메모리에 적재하게 되므로, 애플리케이션 및 저장소 둘다 부담이 가게 됩니다.

Comment on lines 51 to 63
ResponseEntity<Long> responseEntity = testRestTemplate.postForEntity(url, accountSaveDto, Long.class);

assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.OK);

assertThat(responseEntity.getBody()).isGreaterThan(0L);

AccountSaveDto accountSaveDto2 = AccountSaveDto.builder()
.username(username)
.password(password)
.build();

ResponseEntity<Object> responseEntity2 = testRestTemplate.postForEntity(url, accountSaveDto2, Object.class);
assertThat(responseEntity2.getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST);
Copy link
Member

Choose a reason for hiding this comment

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

서로 다른 케이스에 대해서 테스트를 실시하고 있습니다. 한가지 테스트는 한가지 시나리오에만 집중해야 합니다.

import static org.junit.jupiter.api.Assertions.*;

@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
class AccountControllerTest {
Copy link
Member

Choose a reason for hiding this comment

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

요건 정확히 말하면 단위 테스트가 아니라, 모든 의존성이 맞물려 연결된 엔드 테스트입니다. 작성하신 각각의 코드들에 대해서 전부 테스트를 작성하셔야 합니다.

  1. AccountService 단위 테스트
  • 정상 작동 케이스
  • 예외 발생 케이스
  1. AccountController 단위 테스트
  • 정상 작동 케이스
  • 예외 발생 케이스
  1. AccountController 엔드 테스트 <- 형이 지금 작성하신거.

Comment on lines 8 to 10
if (httpStatus == HttpStatus.BAD_REQUEST){
System.out.println(s);
}
Copy link
Member

Choose a reason for hiding this comment

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

로깅할때는 표준 입출력에 직접 붙어서 사용하지마세요.

@GeneratedValue(strategy = GenerationType.AUTO)
private Long userId;

@Id
Copy link
Member

Choose a reason for hiding this comment

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

?? 여기에 있어야할 이유를 모르겠네요

@Table(name = "account")
public class Account {

private final AccountRepository accountRepository;
Copy link
Member

Choose a reason for hiding this comment

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

도메인 엔티티에는 자신의 속성을 표현하는것 이외에 필드를 담으면 안됩니다.

@@ -0,0 +1,23 @@
package com.maximus.spring_server.controller;

import com.maximus.spring_server.domain.Account;
Copy link
Member

Choose a reason for hiding this comment

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

표현층에 도메인층 의존성이 노출되면 안됩니다. 변경에 취약한 구조입니다.

Comment on lines 21 to 22
account.save();
return accountRepository.save(account);
Copy link
Member

Choose a reason for hiding this comment

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

두 save에는 무슨 차이가 있나요? 🤔

Comment on lines 28 to 31
public void save() {
checkUser(username);
checkPassword(password);
}
Copy link
Member

Choose a reason for hiding this comment

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

메소드명은 save인데 실상은 유저네임과 패스워드를 체크하는 구조네요? 🤔 메소드 명이 잘못 작성된것 같습니다.

Comment on lines 46 to 54
Account account2 = accountService.saveUser(account);
assertThat(account.getUsername()).isEqualTo(username);
assertThat(account.getPassword()).isEqualTo(password);

assertThat(account2.getUsername()).isEqualTo(account.getUsername());
assertThat(account2.getPassword()).isEqualTo(account.getPassword());

assertThat(account2.getUsername()).isNotEmpty();
assertThat(account2.getPassword()).isNotEmpty();
Copy link
Member

Choose a reason for hiding this comment

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

반환된 객체는 어차피 given(..).willReturn(...) 로 직접 작성하신 구조인데, 테스트할 의미가 있나요?

그냥 accountRepository.save(..) 가 잘 동작했는지만 판단하면 될것 같습니다.

e.printStackTrace();
}
return account;
public ResponseEntity<?> signUp(@RequestBody Account account) throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

throws 필요한가요?

@@ -25,17 +27,18 @@ public Account(String username, String password) {
@Column(name = "password", nullable = false)
private String password;

public void save() {
public void validationUser() {
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 void validationUser() {
public void validateUser() {

서술형으로 작성 잊지맙시다

account.save();
return accountRepository.save(account);
public Account saveUser(Account account) throws Exception {
if (accountRepository.findByUsername(account.getUsername()) != null) {
Copy link
Member

Choose a reason for hiding this comment

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

accountRepository.existsByUsername 메소드를 추가하는건 어떨까요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants