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차 과제 제출 #4

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

Conversation

seoyoung0121
Copy link
Member

No description provided.

Copy link
Member

@kych4n kych4n left a comment

Choose a reason for hiding this comment

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

코드 포맷팅을 하시면, 가독성이 더 좋아질 것 같습니다. 잘 읽었습니다.

import com.gildedrose.Item;

public class LogicFactory {
public static HashMap<String, Logic> logics=new HashMap<String, Logic>();
Copy link
Member

Choose a reason for hiding this comment

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

저도 HashMap을 이용해서 반갑게 느껴졌습니다.
다만, HashMap을 선언할 때, Map의 종류를 바꾸는 경우를 대비하여, public static Map<String, Logic> logics=new HashMap<String, Logic>();로 쓰는 것이 일반적이라고 합니다. 참고만 해주세요!

Copy link
Member

Choose a reason for hiding this comment

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

이런 부분에 대해서는 알지 못하였는데 좋은 정보 감사합니다

Copy link
Member

Choose a reason for hiding this comment

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

좋은 지적이네요! Map은 인터페이스이고 HashMap은 구현체라 hade3님 말대로 Map을 사용하는 것이 Solid 원칙에 부합한 거 같아요

if (item.quality < 50) {
item.quality++;
}
if (item.sellIn < 11) {
Copy link
Member

Choose a reason for hiding this comment

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

개인적으로, 10일 이내를 표현하기 위해서 11 미만 보다는 10 이하로 쓰는 것이 더 좋은 것 같습니다!

item.quality++;
}
}
if (item.sellIn < 6) {
Copy link
Member

Choose a reason for hiding this comment

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

마찬가지로, 5일 이내를 표현하기 위해서 6 미만 보다는 5 이하로 쓰는 것이 더 좋은 것 같습니다!

Copy link
Member

@goalSetter09 goalSetter09 left a comment

Choose a reason for hiding this comment

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

코드를 정말 깔끔하게 잘 짜신 것 같습니다! 많이 배우고 갑니다~~

public class AgedBrieLogic implements Logic{
@Override
public void update(Item item) {
item.sellIn--;
Copy link
Member

Choose a reason for hiding this comment

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

찬호님이 네트워킹 시간에 말씀하셨듯이, sellIn을 감소시키는 로직을 하나의 함수로 만들어 사용하는 것을 고려해보시면 좋을 것 같습니다!

}

public Logic getLogic(Item item){
if(logics.containsKey(item.name)){
Copy link
Member

Choose a reason for hiding this comment

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

테스트 코드의 아이템들이 정확히 키에 해당하는 아이템이 아니라면 또 다시 코드를 수정해야하는 번거로움이 생길 것 같아요!
테스트 코드에 종속되지 않는 코드로 고쳐보시는 건 어떨까요?
예를 들면 테스트 코드에 "Backstage passes to a TAFKAL80ETC concert"가 아닌 "Backstage passes to xxx concert" 라면 Common의 로직이 수행이 되는 것 처럼 문제가 있을 것 같아요~

Copy link
Member

Choose a reason for hiding this comment

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

이러한 부분에 대하여 생각해보지 못했는데 좋은 의견 감사합니다!

}
}
for (Item item : items) {
logicFactory.getLogic(item).update(item);
Copy link
Member

Choose a reason for hiding this comment

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

각각의 개별 아이템들마다 다르게 구현되어진 업데이트 함수들을 map 을 통해서 연결을 하고 호출하는 방식을 통해서 코드가 굉장히 간소화되어진 거 같아요

public class CommonLogic implements Logic{
@Override
public void update(Item item) {
item.sellIn--;
Copy link
Member

Choose a reason for hiding this comment

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

개별 업데이트 함수마다 퀄리티를 계산하는 부분이 중복되고 있는데, 이런 부분들을 처리해주는 상위 클래스가 있을면 좋을거 같습니다

Copy link
Member

@chanmin-00 chanmin-00 left a comment

Choose a reason for hiding this comment

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

map, 인터페이스, 코드 포맷팅을 사용하여 코드를 일관되고 깔끔하게 작성하셔서 인상깊게 보았습니다. 수고하셨습니다!

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