[Review] 2022/06/09 5차 PR (Aribnb 서비스)

 

이번 리뷰는 Dion 께서 해주셨다! 리뷰해주셔서 감사합니다 !!

[Team - 06][BE] 에어비앤비 프로젝트 3주차 1회차 PR by leejohy-0223 · Pull Request #237 · codesquad-members-2022/a

 

1. 코드리뷰

1-1) 환경 변수에 대한 고민

내가 스크립트를 짜면서도 Dion과 같은 생각을 했다...

이걸 항상 다 설정해줘야 하나?? .... 이에 대한 고민은 너무 길어지는 것 같아 따로 글로 작성하였다.

https://blogshine.tistory.com/447

[Spring] Spring 환경변수 설정파일 선택하기

 

1-2) 시간은 ISO8601 표준은 준수하도록

해당 피드백은 진짜 혼자 할때는 전혀 인식하지 못하는 좋은 피드백이다. Dion 리뷰어님 진짜 너무 좋다!

 

변경된 코드는 다음과 같다. patter부분을 변경해 주었다!

@JsonDeserialize(using = LocalDateTimeDeserializer.class)
@JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd'T'HH:mm:ss")
private LocalDateTime startDateTime;

@JsonDeserialize(using = LocalDateTimeDeserializer.class)
@JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd'T'HH:mm:ss")
private LocalDateTime endDateTime;

좀 예전 글이기는 한데, 향로님의 다음 글 또한 읽어보면 좋다.

https://jojoldu.tistory.com/361

SpringBoot에서 날짜 타입 JSON 변환에 대한 오해 풀기

 

1-3) 가치있는 로그 인가?

어쩌면 단순 확인용으로 로그를 출력하고 있었다...

해당 메서드가 호출되는지는 그냥 실행창에서 호출 스택을 살펴봐도 된다...

 

또한 로그의 level 또한 문제이다. 이게 info 로 찍혀야 할 로그였을까?

수정한다면 debug가 더 적합한 log level인것 같다.

 

1-4) ResultDto 사용과 RESTapi

우선 나는 프로젝트에서 단순 결과 확인용으로 다음과 같은 ResultDto를 사용하고 있었다.

public class ResultDto {

    private final Boolean valid;
    private final String message;

    public ResultDto(Boolean valid, String message) {
        this.valid = valid;
        this.message = message;
    }

    public static ResultDto ok() {
        return new ResultDto(true, HttpStatus.OK.name());
    }

    public static ResultDto error(Exception e) {
        return new ResultDto(false, e.getMessage());
    }

    public Boolean getValid() {
        return valid;
    }

    public String getMessage() {
        return message;
    }
}

 

내 생각에는, 예를 들어 숙소를 찜하는 api를 호출한 후, 정상적으로 찜이 됬다면 다음과 같이 반환하면 좋을것 같다 생각했기 때문이다.

@PostMapping
public ResultDto createWish(@Validated @RequestBody WishCreateRequest request) {
    wishService.addWish(request.getUserId(), request.getHouseId());
    return ResultDto.ok();
}

 

하지만 리뷰어 께서 다음과 같은 리뷰를 남겨주셨다.

RESTful 하다면 응답 상태코드만 확인하면 된다는 점 이다!

이 부분에 대해서는 처음 듣는 이야기 였다.

 

https://sanghaklee.tistory.com/61

REST API 관점에서 바라보는 HTTP 상태 코드(HTTP status code)

위 글을 추가적으로 읽어보게 되었다. 다음부터는 상태코드를 더 적합하게 반환하도록 노력해야 겠다!

 

1-5) Domain 객체에서의 역참조 끊기

위 제목 부터가 잘못 코드를 작성했다는 의미이다...

애당초 Domain이 아닌 DTO를 잘 활용했다면, Entity 에서 역참조 문제가 발생했을리가 없다...

다음 부터는 DTO를 더욱 적극적으로 활용하다...

역참조를 끊어내면서도... Entity를 직접 사용하고 있음을 인식하지 못했다....

 

1-6) orElse 와 orElseGet 의 차이

이에 대해서는 고민해본적이 없었던 문제다.... 놀라운 내용을 알게 되었다!

https://zgundam.tistory.com/174

Optional 클래스의 orElse와 orElseGet에 대한 정리

난 여지껏 orElse는 null 의 상관 없이 실행되고, orElseGet은 null일때만 실행되는 줄 알았는데...

 

이게 잘못된 내요 이였다... 위 블로그 글을 다시 꼭 읽어보자!

 

1-7) Map의 동시성?

이에 대하여 다음과 같이 생각하여 Dion에게 DM을 보냈었다.

나처럼 생각해 보면 내말이 적합한것 같다. 다만 나는 map에 put이 될수 있는 상황이라 생각했던점이 문제였다.

우리의 oAuthServerMap은 처음에 Put을 2번만 하고, 이후 사용할때는 get()만 사용하게 된다.

 

Dion의 답변은 다음과 같다.

CoucurrentHashMap 코드에 가보니 다른곳에는 syncronized 키워드가 있지만, get()에는 동기화 키워드가 없었다.

즉, 애당초 CoucurrentHashMap도 get()의 동기성을 보장하지 않는다는 이야기 였다.

 

다음 코드 부분을 살펴보자.

양쪽이 심지어 거의 동일한 코드로 구현되어 있었다.

 

결론적으로 우리의 oAuthServerMap은 동시성이 지켜질수 있었다. 읽기 전용으로 사용하고 있었기 때문이다!

 

1-8) 예외에 대한 검증을 정확하게

예외가 발생하는지 검증하는 과정인데... 정작 예외를 잡는 부분을 코드로 작성하지 않았었다...

다음과 같이 예외를 잡는 코드 부분을 작성하였다.

resultActions.andExpect(
        (rslt) -> assertTrue(rslt.getResolvedException().getClass().isAssignableFrom(IllegalStateException.class))
).andReturn();

 

1-9) 행위 중심의 테스트를 작성하도록

1) 가입한 회원이 요청한다

2) 찜 목록에 사전에 찜한 목록이 있을경우 -> 위시 리스트를 반환

3) 찜 목록에 사전에 찜한 목록이 없을경우 -> 비어있는 리스트를 반환

 

Dion의 말 처럼 나는 직접 찜 목록 데이터를 밀어넣고 테스트 하고 있었다.

 

중간에 찜 목록을 직접 추가하는 부분과, 추가하지 않는 "행위"도 직접 작성해서 테스트하는것이 더 좋았을것 같다.

 

1-10) 행위 검증하기

Dion의 말 처럼 method에 대한 검증이 없었다.

상태 코드와, 반환하는 메시지만 검증하고 있었다.

 

추가적으로 DELETE 메서드를 사용했음을 검증했어야 더 좋은것 같다!