Skip to content

Coordinate Step6 complete - #125

Merged
javajigi merged 4 commits into
code-squad:forevereverfrom
foreverever:step6
Oct 29, 2018
Merged

javajigi merged 4 commits into
code-squad:forevereverfrom
foreverever:step6

Conversation

@foreverever

Copy link
Copy Markdown

Coordinate Step6 complete.
please give me a feedback.
thank you.

@javajigi
javajigi changed the base branch from master to foreverever October 27, 2018 09:27

@javajigi javajigi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

인터페이스 적용 잘 했네요. 💯
상속에 대해 잘못 이해한 부분이 있는 것 같은데 다시 한번 생각하는 계기가 되면 좋겠네요.

Comment thread src/main/java/domain/Figure.java Outdated
import java.util.Set;

public abstract class Figure {
public abstract class Figure extends FigureFactory implements FigureInformation {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Figure가 FigureFactory를 상속하는 구조인데 괜찮은 설계인가?
Figure는 FigureFactory의 자식 특성을 그대로 가지고 있는 것이 맞나?
객체의 상속에 대해 다시 한번 생각해 보면 좋겠다.

static {
figures.put(FigureType.LINE, points -> new Line(points));
figures.put(FigureType.TRIANGLE, points -> new Triangle(points));
figures.put(FigureType.RECTANGLE, points -> new Rectangle(points));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

Comment thread src/main/java/domain/Line.java Outdated
public Line(List<Point> points) {
Line(List<Point> points) {
super(points);
checkSize(TWO);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

현재 각각의 Figure에서 checkSize(TWO), checkSize(THREE)와 같이 호출하고 있는데 이 부분의 중복도 없애 본다.
힌트는 FigureInformation 인터페이스에 size() 메소드를 추가하고 체크하는 로직을 Figure가 담당하도록 도전해 본다.

@javajigi
javajigi merged commit 075cb15 into code-squad:foreverever Oct 29, 2018
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.

2 participants