-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: master
Are you sure you want to change the base?
Conversation
chimaek
commented
May 11, 2021
- post http메서드로 회원 data보내기 (정상처리 200)
- 중복 회원있을 시 예외처리 (badRequest 400)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
더 많은 테스트 코드가 필요합니다.
public Long save(@RequestBody AccountSaveDto accountSaveDto) { | ||
return accountService.save(accountSaveDto); |
There was a problem hiding this comment.
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 = "잘못된 접근입니다.") |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
하지말아야할 장애포인트입니다. findAll()은 시간, 공간 복잡도가 O(n)인 데다, 한번에 다량의 데이터를 애플리케이션 메모리에 적재하게 되므로, 애플리케이션 및 저장소 둘다 부담이 가게 됩니다.
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); |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요건 정확히 말하면 단위 테스트가 아니라, 모든 의존성이 맞물려 연결된 엔드 테스트입니다. 작성하신 각각의 코드들에 대해서 전부 테스트를 작성하셔야 합니다.
- AccountService 단위 테스트
- 정상 작동 케이스
- 예외 발생 케이스
- AccountController 단위 테스트
- 정상 작동 케이스
- 예외 발생 케이스
- AccountController 엔드 테스트 <- 형이 지금 작성하신거.
if (httpStatus == HttpStatus.BAD_REQUEST){ | ||
System.out.println(s); | ||
} |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
표현층에 도메인층 의존성이 노출되면 안됩니다. 변경에 취약한 구조입니다.
account.save(); | ||
return accountRepository.save(account); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
두 save에는 무슨 차이가 있나요? 🤔
public void save() { | ||
checkUser(username); | ||
checkPassword(password); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메소드명은 save인데 실상은 유저네임과 패스워드를 체크하는 구조네요? 🤔 메소드 명이 잘못 작성된것 같습니다.
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(); |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accountRepository.existsByUsername 메소드를 추가하는건 어떨까요?