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

[이주영] 1차 과제 제출 #6

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

0-x-14
Copy link
Member

@0-x-14 0-x-14 commented Sep 30, 2024

No description provided.

@supsup-hae
Copy link
Member

잘 읽었습니다! 클래스 네이밍이 통일되어서 코드를 살펴보기 좋았어요! 👍

return new UpdateConjuredItemLogic();
}

switch (item.name) {
Copy link
Member

@supsup-hae supsup-hae Oct 7, 2024

Choose a reason for hiding this comment

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

팩토리를 이용해서 아이템별 처리 로직을 구분하신 거 좋네요!
다른 아이템들도 전체 이름이 아닌 위 Conjured처럼 특정 이름을 기반으로 분류되도록 수정하면 유지보수 측면에서 더 좋을 거 같아요!

if (item.quality < 0) {
// quality가 음수가 될수도 있으므로
// sellIn > 0, quality = 1인 경우, sellIn < 0, quality = 3인 경우 해당
item.quality = 0;
Copy link
Member

@supsup-hae supsup-hae Oct 7, 2024

Choose a reason for hiding this comment

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

quality 값을 조정하는 코드들을 별도의 클래스 혹은 메소드를 통해서 관리한다면 코드 중복이 많이 줄어들 거 같아요!

Copy link
Member

@shinyj0 shinyj0 left a comment

Choose a reason for hiding this comment

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

코드 잘 봤습니다!

Comment on lines +20 to +22
if (item.quality < 0) {
// quality가 음수가 될수도 있으므로
// sellIn > 0, quality = 1인 경우, sellIn < 0, quality = 3인 경우 해당
Copy link
Member

Choose a reason for hiding this comment

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

주석을 자세히 적어주셔서 코드를 볼 때 편했던 것 같아요!

Comment on lines +7 to +8
if (item.name.contains("Conjured")) {
// Conjured 아이템인 경우 switch문에서 판별 불가능하므로 별도 처리
Copy link
Member

Choose a reason for hiding this comment

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

궁금하게 있어 여쭈어봅니다! switch문에서 conjured아이템인 경우 판별 불가능한 이유 알 수 있을까요??

Copy link
Member Author

@0-x-14 0-x-14 Oct 9, 2024

Choose a reason for hiding this comment

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

switch문의 조건은 조건에서 제시한 값과 정확히 일치하는 경우에만 실행됩니다! Conjured 아이템이 테스트 케이스에서 제시된 “Conjured Mana Cake“만 있으면 switch문으로 처리해도 상관없겠지만, 테스트 케이스 외에도 다른 Conjured 아이템까지 처리하는 방향으로 구현하였습니다.

Comment on lines +7 to +18
@Override
public void update(Item item) {

if (item.quality < 50) {
item.quality++;
}
if (item.sellIn <= 10 && item.quality < 50) {
item.quality++;
}
if (item.sellIn <= 5 && item.quality < 50) {
item.quality++;
}
Copy link

Choose a reason for hiding this comment

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

저번 세션때 피드백을 반영해서 매직 넘버로 관리하면 좋을 것 같습니다!

return new UpdateConjuredItemLogic();
}

switch (item.name) {
Copy link

Choose a reason for hiding this comment

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

switch문을 사용해서 가독성이 좋아진 것 같습니다!👍

Copy link
Member

@yooooonshine yooooonshine left a comment

Choose a reason for hiding this comment

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

굿

item.quality++;
}

}
Copy link
Member

Choose a reason for hiding this comment

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

메서드 끝나기 전에 의미 없는 한 줄이 추가된 거 같습니다.

if (item.sellIn <= 5 && item.quality < 50) {
item.quality++;
}

Copy link
Member

Choose a reason for hiding this comment

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

2배, 3배가 된다는 것을 if문으로 처리하셨군요.
이 부분 약간 이해하기 힘든 거 같습니다! 직관적이지 않아보여요!
3배가 된다는 건 * 3로직으로 변경해보시면 어떨까 싶어요

Copy link
Member Author

Choose a reason for hiding this comment

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

작성하면서도 고민했던 부분인데 말씀해주신대로 *3으로 작성하는게 가독성이 더 좋을 것 같네요 수정해보겠습니다 피드백 감사합니다!

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.

5 participants