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

2주차 미션 / 서버 3조 - 임수빈 #20

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

limsubinn
Copy link

  1. Ladder → 높이(row) > 0, 참여자(numberOfPerson) > 0
  2. 직관적으로 구현하기 위해 [row+1][numberOfPerson+1] 크기의 배열을 생성
  3. drawLine → x행의 y와 y+1 사이에 다리를 생성 (x, y 좌표에서 오른쪽으로)
    따라서 0 < x ≤ row, 0 < y < numberOfPerson
  4. 이미 라인이 있는 곳에 또 라인을 그릴 수 없다.

저번 코드에서 로직 안 맞는 부분 수정하고, 강의 보면서 리팩토링 진행해서 강의와 비슷한 코드가 많습니다..!
요구사항 대로 잘 구현이 된 건지 모르겠네요 🥲

limsubinn and others added 26 commits March 22, 2023 14:48
drawLine, run, 유효성 검사
drawLine 파라미터
week1: 사다리 타기 구현
LadderRunner, LadderCreator, LadderGame
RowTest로 분리된 로직 제거
RandomLadderCreator, RandomLadderGame, RandomLadderTest
week2: 사다리 모양 출력 및 사다리 자동 생성
Copy link
Contributor

@jung-woo-kim jung-woo-kim left a comment

Choose a reason for hiding this comment

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

안녕하세요!
지난번엔 막바지에 pr이 올라왔던 것 같아 리뷰를 못드렸던거 같은데 이번에 드릴 수 있게되어 좋았습니다.

제 코드와 비슷한 면이 많았던 것 같아서 아마 보시고, 참고 하신 후에 진행하셨을 것 같은데
(착각이라면 정말..매우..죄송합니다.🥲🥲🥲)

어떤 부분들을 제가 생각하면서 코드를 작성했을지 (의존성 주입이라던가, 팩터리 패턴이라던가, wrapper 클래스들 이라던가) 잘 이해하신 후 넘어가셨으면 좋겠습니다!!

리뷰가 도움이 되었으면 좋겠습니다.
고생하셨어요! 👍

return position.getPosition();
}

private void printRows(LadderPosition ladderPosition) {
Copy link
Contributor

Choose a reason for hiding this comment

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

메서드 깔끔하네요! 다만, 저도 아직 코드에 적용해보진 못했지만 LadderViewr라든가 클래스를 만들어서 로직을 분리해보면 좋을 것 같아요!

return getRowValue() * getNumberOfPersonValue();
}

public static LadderSize createLadderSize(NaturalNumber row, NaturalNumber numberOfPerson) {
Copy link
Contributor

Choose a reason for hiding this comment

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

정적 팩토리 메서드 패턴의 장점은 다른 곳에서 객체를 생성하는 책임을 분리할 수 있게 하는 것이죠!
좋습니다!

public static NaturalNumber createNaturalNumber(int number) {
validationNaturalNumber(number);
return new NaturalNumber(number);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

?가 있어서 도움되는 글 첨부합니다.
이해 못하시는 스터디원이 또 있다면 공유해주세요!
정적 팩토리 메서드

Copy link
Contributor

Choose a reason for hiding this comment

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

LadderGame에서 RandomLadderGame을 생성할 수 있는데 이 코드는 없어도 되지 않나요??🧐🧐


if (nodes[position.getPosition()].isRight()) {
position.plus();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

node[position.getPosition()].next() 등의 메서드를 만들어 이 로직의 실질적 책임이 있는 node에 분리할 수 있지 않을까요?🧐

}

public boolean noLines(Position y) {
return (nodes[y.getPosition()].isCenter() && nodes[y.getNextPosition()].isCenter());
Copy link
Contributor

Choose a reason for hiding this comment

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

제 코드에서는 되게 복잡하게 했던 것 같은데, 이렇게 하니 깔끔하네요! 배워갑니다😮😮

public class RandomLadderCreator implements LadderCreatorInterface {

LadderSize ladderSize;
LadderCreator ladderCreator;
Copy link
Contributor

Choose a reason for hiding this comment

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

ladderCreator를 조합을 통해 기능을 활용하셨네요!
조합의 장점을 느끼셨다면 좋겠습니다👍👍

Copy link
Contributor

Choose a reason for hiding this comment

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

전체적으로 Wrapper 클래스들에 대한 테스트가 없는 것 같아요~~

@limsubinn
Copy link
Author

감사합니다 ☺️
말씀해주신 부분 참고해서 더 고민해보겠습니다!

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