feat: implements coordinate calculator except rectangle by oliviarla · Pull Request #7 · object-oriented-thinking/java-coordinate-playground

oliviarla · 2022-08-01T13:00:27Z

No description provided.

this-is-spear

이번 미션도 수고 많으셨습니다. ☺️
프로젝트가 정말 잘 설계된게 눈에 보이네요! 저도 코드 리뷰를 하면서 많이 배웠습니다.

this-is-spear · 2022-08-02T06:10:41Z

src/main/java/CoordinateApplication.java

+    private static FigureFactory figureFactory = new FigureFactory();
+
+    public static void main(String[] args) {
+        inputView = new InputView();


CoordinateApplication의 main을 실행할 때, InputView 인스턴스를 생성자로 주입하거나 미리 주입하는 건 어떨까요?

this-is-spear · 2022-08-02T06:11:01Z

src/main/java/CoordinateApplication.java

+    private static InputView inputView;
+    private static FigureFactory figureFactory = new FigureFactory();


두 필드 전부 final 키워드를 붙여서 방어적인 코드로 설계하는 것이 좋아보입니다!

this-is-spear · 2022-08-02T06:11:32Z

src/main/java/coordinate/AbstractFigure.java

+//    protected Point getPoint(int index) {
+//        return points.getPoints().get(index);
+//    }


사용하지 않는 코드는 삭제하는 게 좋습니다 ☺️ 삭제하지 않고 방치하게 된다면 잊혀져서 관리가 잘 안되는 경우가 많더라구요 ㅜ

좋은 습관인 것 같습니다 ㅜ 앞으로 삭제하는 버릇을 들여야겠어요!

this-is-spear · 2022-08-02T06:19:38Z

src/main/java/coordinate/Triangle.java

+        distances.add(points.get(0).getDistance(points.get(1)));
+        distances.add(points.get(1).getDistance(points.get(2)));
+        distances.add(points.get(2).getDistance(points.get(0)));


points를 바로 호출해서 내부 데이터를 가져오게 된다면 내부 값이 변경될 수 있는 문제가 발생할 수 있을 것 같아요. 그리고 추후 설계가 변경된다면 가장 먼저 수정되어야 할 부분이 될 수도 있을 것 같습니다. 내부 데이터를 바로 호출하는 방식이 아닌 추상 클래스에서 ponits 내부 값을 전달하는 함수를 선언하는건 어떨까요?

this-is-spear · 2022-08-02T06:20:15Z

src/main/java/coordinate/Triangle.java

+        distances.add(points.get(0).getDistance(points.get(1)));
+        distances.add(points.get(1).getDistance(points.get(2)));
+        distances.add(points.get(2).getDistance(points.get(0)));
+        Double s = distances.stream().mapToDouble(i->i).sum() /2;


stream에서는 reduce() 함수도 제공하고 있습니다! 사용해보시는걸 추천드려요 ☺️

this-is-spear · 2022-08-02T06:27:15Z

src/main/java/coordinate/Point.java

+        this.x = x;
+        if (x < 0 || x > 24) {
+            throw new IllegalArgumentException();
+        }


유효성 검사를 실패하게 되면 인자를 주입할 필요가 없기 때문에 유효성 검사를 진행한 후 x에 값이 주입되어야 합니다.

Suggested change

this.x = x;

if (x < 0 || x > 24) {

throw new IllegalArgumentException();

}

if (x < 0 || x > 24) {

throw new IllegalArgumentException();

}

this.x = x;

this-is-spear · 2022-08-02T06:28:01Z

src/main/java/coordinate/Point.java

+        this.x = x;
+        if (x < 0 || x > 24) {
+            throw new IllegalArgumentException();
+        }
+
+        this.y = y;
+        if (y < 0 || y > 24) {
+            throw new IllegalArgumentException();
+        }


이렇게 선언하는 방식이 가독성이 좋을 것 같은데 어떻게 생각하시나요? ☺️

Suggested change

this.x = x;

if (x < 0 || x > 24) {

throw new IllegalArgumentException();

}

this.y = y;

if (y < 0 || y > 24) {

throw new IllegalArgumentException();

}

if (x < 0 || x > 24) {

throw new IllegalArgumentException();

}

if (y < 0 || y > 24) {

throw new IllegalArgumentException();

}

this.x = x;

this.y = y;

this-is-spear · 2022-08-02T06:33:27Z

src/main/java/coordinate/Points.java

+    public static List<Point> getPoints() {
+        return points;
+    }


방어적인 코드로 작성하지 않으면 내부 컬렉션의 값이 변경될 가능성이 있습니다. 만약 아무 조건없이 컬렉션의 참조값을 리턴하게 된다면 다른 인스턴스들이 내부 값이 변경된 상태로 로직을 수행할 수 있는 문제가 발생합니다.

리스트를 수정할 수 없도록 points 컬렉션은 읽기 전용 컬렉션으로 만들거나 방어적인 복사로 컬렉션을 새로 생성하는 방식으로 진행해야 합니다.

컬렉션에 담기는 객체들의 상태가 변경되지 않도록 불변 객체로 만들 필요가 있습니다. Point는 불변 객체이니 이 부분은 문제가 되진 않습니다.

불변객체의 중요성 꼭 기억하겠습니다!
그런데 getPoints가 사용되지 않는 메소드라 아예 제거해도 될거같습니다 ㅎㅎ

this-is-spear · 2022-08-02T06:35:30Z

src/main/java/view/InputView.java

+            List<String> nums;
+            nums = Arrays.stream(str.split("\\(|,|\\)")).filter(t -> !t.isEmpty()).collect(Collectors.toList());


이렇게 한 번에 선언할 수 있어보입니다.

Suggested change

List<String> nums;

nums = Arrays.stream(str.split("\\(|,|\\)")).filter(t -> !t.isEmpty()).collect(Collectors.toList());

List<String> nums = Arrays.stream(str.split("\\(|,|\\)")).filter(t -> !t.isEmpty()).collect(Collectors.toList());

this-is-spear · 2022-08-02T06:45:08Z

src/main/java/coordinate/Line.java

+
+    @Override
+    public void output(){
+        System.out.printf("두 점 사이의 거리는 %f", area());


output 메서드를 String을 반환해서 OutputView에게 System.out.printf하도록 위임하는 건 어떨까요? String을 반환하게 구현한다면 테스트가 쉬워지고 콘솔에 출력하는 기능(OutputView)을 모으게 된다면 확장과 변경에 용이합니다.

확실히 기능을 모으는게 유지보수에도 좋을 것 같네요! 감사합니다 :)

oliviarla

달아주신 코멘트 하나하나 정말 도움 많이 되었습니다 감사합니다 :)

oliviarla · 2022-08-13T13:37:14Z

src/main/java/coordinate/AbstractFigure.java

+//    protected Point getPoint(int index) {
+//        return points.getPoints().get(index);
+//    }


좋은 습관인 것 같습니다 ㅜ 앞으로 삭제하는 버릇을 들여야겠어요!

oliviarla · 2022-08-13T13:47:50Z

src/main/java/coordinate/Line.java

+
+    @Override
+    public void output(){
+        System.out.printf("두 점 사이의 거리는 %f", area());


확실히 기능을 모으는게 유지보수에도 좋을 것 같네요! 감사합니다 :)

oliviarla · 2022-08-13T14:00:36Z

src/main/java/coordinate/Points.java

+    public static List<Point> getPoints() {
+        return points;
+    }


불변객체의 중요성 꼭 기억하겠습니다!
그런데 getPoints가 사용되지 않는 메소드라 아예 제거해도 될거같습니다 ㅎㅎ

oliviarla · 2022-08-13T14:04:19Z

src/main/java/coordinate/Triangle.java

+        for(Double d: distances){
+            result *= (s-d);
+        }


stream 너무 어렵네요 ㅠㅠ 익숙해지기까지 많은 연습이 필요한 것 같습니다..

feat: implements coordinate calculator except rectangle

d195c95

this-is-spear reviewed Aug 2, 2022

View reviewed changes

fix: 코드리뷰 반영 및 개선

5cd8f30

oliviarla commented Aug 13, 2022

View reviewed changes

		private static InputView inputView;
		private static FigureFactory figureFactory = new FigureFactory();

		List<String> nums;
		nums = Arrays.stream(str.split("\\(\|,\|\\)")).filter(t -> !t.isEmpty()).collect(Collectors.toList());

	List<String> nums;
	nums = Arrays.stream(str.split("\\(\|,\|\\)")).filter(t -> !t.isEmpty()).collect(Collectors.toList());
	List<String> nums = Arrays.stream(str.split("\\(\|,\|\\)")).filter(t -> !t.isEmpty()).collect(Collectors.toList());

Comments

Conversation

oliviarla commented Aug 1, 2022

Uh oh!

this-is-spear left a comment

Choose a reason for hiding this comment

Uh oh!

this-is-spear Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

this-is-spear Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

this-is-spear Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

this-is-spear Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

this-is-spear Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviarla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

this-is-spear Aug 2, 2022 •

edited

Loading

this-is-spear Aug 2, 2022 •

edited

Loading

this-is-spear Aug 2, 2022 •

edited

Loading

this-is-spear Aug 2, 2022 •

edited

Loading

this-is-spear Aug 2, 2022 •

edited

Loading