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

[좌표 계산기] 베디 미션 제출합니다 #5

Merged
merged 45 commits into from
May 26, 2019

Conversation

dpudpu
Copy link

@dpudpu dpudpu commented May 22, 2019

리뷰 해주셔서 감사합니다.
많은 지적 해주시면 감사하겠습니다.

pkch93 and others added 30 commits May 21, 2019 12:27
- 거리 계산 구현
- 좌표 입력 기능
- input 유효성 검사
- input 값을 통해 List<Point> 생성
- 점이 4개인지 예외처리
- 직사각형인지 여부 예외처리
- 중복된 점 예외처리
같은 기능을 하는 X,Y를 Coordinate로 합침
Figure.class 는 Triangle, Square의 상위 클래스다.
Copy link

@phs1116 phs1116 left a comment

Choose a reason for hiding this comment

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

안녕하세요! 리뷰어 화투입니다.
객체지향적으로 짜시려고 노력 많이 하신점이 보여요 👍
조금 더 추상화 할 수 있을것 같아서 피드백 몇가지 추가했어요.
참고 부탁드리고 궁금하신 점 있으시면 연락주세요. :)

@@ -0,0 +1,18 @@
package car.domain;

public abstract class Car {
Copy link

Choose a reason for hiding this comment

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

프로그래밍 요구사항 - 2단계 : 인터페이스를 적용해 구현한다.

적용이 안되어 있는 것 같습니다. :)

public class CoordinateApp {
public static void main(String[] args) {
Points points = generatePoints();
if (points.size() == Line.NUMBER_OF_POINTS) {
Copy link

Choose a reason for hiding this comment

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

Line또한 다른 클래스와 마찬가지로 하나의 추상화로 표현 할 수있을 것 같아요. :)
if를 통한 분기 없이 한번에 표현해보면 어떨까요?


import coordinate.domain.generator.LinesGenerator;

public abstract class Figure implements AreaCalculator {
Copy link

Choose a reason for hiding this comment

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

Line또한 Point들의 조합이고
나머지 도형 또한 Point들의 조합이에요.
이것들을 포함한 하나의 추상화로 표현해보면 어떨까요? 이 인터페이스는 값을 돌려주는 것을 가지고 있을거구요.
그 하위로 영역을 구하는 추상화와 길이를 구하는 추상이 있을거고,
최상위 추상 인터페이스가 그걸 호출하도록 할 수 있을 것 같네요. :)

import java.util.Objects;

public class Point {
private final Coordinate x;
Copy link

Choose a reason for hiding this comment

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

Coordinate 값 포장 👍

import java.util.Objects;

public class Points {
private final List<Point> points;
Copy link

Choose a reason for hiding this comment

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

일급 컬렉션 👍

import coordinate.domain.Triangle;

public enum FigureFactory {
TRIANGLE(Triangle.NUMBER_OF_POINTS, Triangle::new),
Copy link

Choose a reason for hiding this comment

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

LINE도 같이 관리할 수 있도록 해보면 좋을 것 같아요. :)


public static Coordinate of(final int coordinate) {
Coordinate instance = map.get(coordinate);
if(map.get(coordinate)==null) {
Copy link

Choose a reason for hiding this comment

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

== 좌우 공백 맞춰주세요 :)


public UI() {
this.coordinates = new boolean[25][25];
}
Copy link

Choose a reason for hiding this comment

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

하드코딩이 아닌 상수로 빼는게 더 좋을 것 같아요!

@dpudpu
Copy link
Author

dpudpu commented May 25, 2019

화투님 안녕하세요. 좋은 피드백 해주셔서 감사합니다.

지적해주신 부분들 다 반영했습니다. Line도 추상화할 수 있지 않겠냐고 말씀을 해주셨는데요.
저 같은 경우는 Figure가 Lines(List<Line>)를 가지는 구조로 해서 Line이 상속받을 수 없는 구조였습니다.
하지만 피드백 받고 Line도 상속받는 방식으로 구현했는데요.
구조는
Figure 인터페이스와 이를 구현한 AbstractFigure 추상 골격 구현 클래스를 만들어서
Line과 Lines를 가진RealFigure이 상속받고 RealFigure를 Square와 Triangle이 상속 받도록 했습니다.

이렇게 구현하면 너무 복잡한 건 아닐까? 걱정도 되는데 어떻게 생각하시나요?
귀중한 주말에 피드백을 위해서 시간 내주셔서 감사합니다.

Copy link

@phs1116 phs1116 left a comment

Choose a reason for hiding this comment

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

안녕하세요. 리뷰어 화투입니다. :)
피드백 반영 너무 잘 해주신 것 같아요.
인터페이스, 추상 클래스 활용 👍
이번 단계 머지할게요!
메서드 네이밍 관련해서 피드백 하나만 딱 추가했는데 나중에 개발할 때 참고해주세요. :)


@Override
public double area() {
return super.getLines().lengths().get(0) * super.getLines().lengths().get(2);
Copy link

Choose a reason for hiding this comment

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

lines.getLength(0)으로도 표현 할 수 있지 않을까요?
일반적으로 자바의 메서드는 동사형으로 표현하니까요. :)

@phs1116 phs1116 merged commit 5c713aa into woowacourse:dpudpu May 26, 2019
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.

3 participants