[사다리 1-4단계] 천희정 미션 제출합니다. #53
Conversation
- 오류 수정 - MVC 적용
|
<리팩토링 1>
<장순님 PR>
<공통>
|
There was a problem hiding this comment.
우선 1차적으로 리뷰 드려봅니다 희정님!
MVC
MVC 구조는 일단 전체적인 구조 면에서는 컨트롤러의 조율을 통한 구조로 잘 만들어주신 것 같아요. 다만, MVC 구조가 근본적으로 하고자 하는 것이 무엇인지, 얻을 수 있는 장점이 무엇인지 좀 더 고민을 해보면 좋을 것 같아요. 지금은 약간 MVC 구조를 흉내낸 것에 불과하다는 느낌도 있어서, 깊은 고민을 해보면 아키텍처 설계에 많은 도움이 될거에요!
enum
제 생각에 이번 미션에는 enum을 적용할만한 곳은 크게 없어보여요!
테스트
안그래도 이 부분은 코드에 File Changes에 리뷰를 남기려다가 본문으로 가져오려고 한 부분인데요. 전체적으로 테스트 하나하나의 크기가 너무 크고 검증이 장황합니다. 우리가 프로덕션 코드도 클래스 하나, 메서드 하나가 하는 일이 너무 많으면 분리하잖아요? 테스트 코드도 코드인 만큼 같은 논리로 볼 수 있을 것 같아요. 테스트 코드 역시 유지보수를 해야 하고, 그렇기 때문에 가독성을 유지해야 합니다. 또한 테스트 코드는 문서화의 역할을 겸할 수도 있기 때문에 가독성이 더더욱 중요하죠.
특히나 이번 PR에서는 도메인 객체들이 print 메서드들을 모두 가지고 있는데, 이 부분과도 연결해서 생각해보면 좋을 것 같습니다. 힌트는, 저는 도메인 객체에 print가 있는 것은 무슨 일이 있어도 반대입니다.
저는 개인적으로 각 클래스 단위, 메서드, 분기 단위로 테스트를 나누는 것을 추천드립니다. (따로따로가 아니라 셋 모두를 적용해야 해요)
- 클래스 단위: Ladder, Line이 있으면 LadderTest, LineTest
- 메서드 단위: 존재하는 메서드들에 대한 테스트. 단, getter 테스트 등 무의미한 테스트는 불필요.
- 분기 단위: 메서드 내부에서 분기가 존재한다면 해당 분기에 대한 경곗값을 모두 테스트
또한 한 테스트에서 여러 검증문이 있을 수는 있으나, 그것은 테스트하려는 기능을 호출했을 때 여러 상태가 변경되기 때문에 그것을 검증해야 하는 경우에 사용하는 것이지 한 테스트에 여러 기능을 테스트하기 위해 사용되는 것은 아닙니다. 테스트 하나는 하나의 기능만을 테스트해야 합니다. 만약 도메인에 대한 단위 테스트라면 도메인의 메서드 하나를 테스트 해야 하는 것이고, 전체 프로그램에 대한 통합 테스트를 한다면 전체 프로그램이 가진 기능 중 하나를 테스트 해야 하는 것이죠.
전체적으로 코드는 가독성 있게 잘 짜주셨기 때문에, 프로그램의 구조, 그리고 탄탄하고 가독성 좋은 테스트 라는 두 가지 주제를 바탕으로 리팩토링을 진행해보시면 좋을 것 같습니다!
|
|
||
| public static Line generate(int width) { | ||
| List<Boolean> steps = new ArrayList<>(); | ||
| Random random = new Random(); |
| public void print() { | ||
| System.out.print(" "); | ||
| for (Boolean step : steps) { | ||
| System.out.print(step ? "|-----" : "| "); | ||
| } | ||
| System.out.println("|"); | ||
| } |
There was a problem hiding this comment.
출력의 책임은 Line일까요? 만약 게임의 규칙은 유지한 채, 출력 수단이 콘솔에서 웹으로 바뀐다면 도메인도 변경되어야 할까요?
| public class Height { | ||
| private final int value; | ||
|
|
||
| public Height(int value) { | ||
| this.value = value; | ||
| } | ||
|
|
||
| public int getValue() { | ||
| return value; | ||
| } | ||
| } |
There was a problem hiding this comment.
height는 그냥 int 값으로 존재해도 괜찮았을 것 같군요(어차피 width가 있는 것도 아니어서)
의미 있는 값이 되려면 value > 0 검증 정도라도 했어야 할 것 같은데, 이 상황에서는 클래스 자체가 없어도 됐을 것 같긴 해요.
애초에 List의 사이즈가 곧 height일테니까요!
| public class LadderGenerator { | ||
| public static Ladder generate(int countOfPlayers, Height height) { | ||
| List<Line> lines = new ArrayList<>(); | ||
| for (int i = 0; i < height.getValue(); i++) { |
There was a problem hiding this comment.
여기를 보니까 결국 height가 굳이 클래스일 필요가 없는 것이 증명되긴 했군요
| throw new IllegalArgumentException("이름은 5글자 이하여야 합니다."); | ||
| } | ||
| if (value.equals("all")) { | ||
| throw new IllegalArgumentException("\"all\"은 참가자 이름으로 사용할 수 없습니다."); |
| public static class Result { | ||
| private final String value; | ||
|
|
||
| public Result(String value) { | ||
| this.value = value; | ||
| } | ||
|
|
||
| public String getValue() { | ||
| return value; | ||
| } | ||
| } |
There was a problem hiding this comment.
웬만하면 별도의 파일을 통해 클래스를 만드는 것을 추천드려요! 내부 클래스보다는 그게 더 가시성이 좋은 편입니다!
| import domain.Names; | ||
| import java.util.Map; | ||
| public class OutputView { | ||
| public void printLadder(Names names, Ladder ladder, Results results) { |
| assertDoesNotThrow(() -> new Name("A")); | ||
| assertDoesNotThrow(() -> new Name("12345")); | ||
| assertDoesNotThrow(() -> new Name("가나다라마")); | ||
| assertDoesNotThrow(() -> new Name(" A ")); |
There was a problem hiding this comment.
과거 미션 요구사항에 테스트는 assertj의 메서드를 사용하는 것이 있었던 것 같은데요! 혹시 assertj보다 jupiter의 assertion이 더 편하다고 느끼시나요?
| } | ||
|
|
||
| public Results.Result play(Name name) { | ||
| int index = names.getValues().indexOf(name); |
안녕하세요 ! 오찌 :-)
이번 리뷰도 잘 부탁드립니다
1. 기존 프로그래밍 요구사항
이번 미션에서는 기존 프로그래밍 요구사항인 indent, else등을 잘 지키기 위해 노력해서 코드를 작성했습니다 ! 값 검증에서 else가 들어가야하는 경우나, if가 계속해서 들어가야하는 경우 메소드로 따로 작성했습니다. 또한, 지난 리뷰에 작성해주신 와일드 카드 임포트 없이 필요한 클래스만 임포트 해서 사용했습니다.
-> 사실 와일드 카드가 굉장히 편리하다고 느꼈는데, 왜 사용하지 말라고 하시는지 궁금하여 찾아보았습니다.
명확하지 않은 의존성,클래스 이름 충돌 위험.불필요한 클래스 임포트,가독성 저하정도가 있는 것 같습니다. 지금 처럼 규모가 작은 프로젝트에서는 문제를 일으키지 않겠으나, 개발 규모가 커지면 안전하지 않을 수 있기에 와일드 카드를 지양하는 습관을 가지려고 하겠습니다 !2. MVC
지난 주 미션 리뷰에서 말씀해주신 모델-뷰-컨트롤러의 개념과, 도메인, 추가로 의존성에 대한 공부를 한 경험을 바탕으로 이번 미션에서는 MVC 방식을 지키기위해 노력했습니다. 그런데 이 구조를 제대로 사용하고 있는지, 역할을 제대로 분리하고 있는지 궁금합니다.
3. Java Enum
이번 미션에서 Java Enum을 어디에 적용하면 좋은지 궁금합니다 !
-> Enum에 관해 공부해보았을때
타입안정성 존재,코드 자동완성 제공(IDE에서),가독성 좋음,메모리 효율성 좋음,추가 기능 구현 가능정도의 장점을 갖고 있는 것으로 알고 있습니다. 현재 제가 작성한 코드에서는 사용자가 결과값을 입력할 수 있기도하고, 이름의 수와 결과의 개수가 매번 달라질 수 있기 때문에, enum을 사용하지 않았습니다. 미션의 요구사항에 Enum을 적용하라는 내용이 있는 만큼 어디에 적용하면 좋을지 궁금해서 여쭤봅니다 !4. 테스트 코드
이번 미션 같은 경우 테스트 코드를 Domain의 값 검증, 그리고 ladder game 두가지로 나누어 코드를 작성했습니다. 본래 domain, view, controller 내의 모든 클래스마다 각각 테스트 파일을 만들어 코드를 작성해야하나 고민했으나, 파일이 너무 많아질 것 같기도하고, 테스트의 내용이 Height나 Result, Name의 경우 너무 짧을 것 같아 두개의 파일로 작성했습니다. 모든 클래스별로 테스트 파일을 만드는것이 좋은지, 혹은 지금처럼 domain별로, view 별로 작성하는 것이 좋은지 궁금합니다 !
감사합니다 :-)