[김영찬] 1차 과제 제출 #2
Conversation
| return updateLogicMap.get(filter); | ||
| } | ||
| } | ||
| return updateLogicMap.get(null); |
There was a problem hiding this comment.
Map을 통해 updateLogic을 관리하고, 가져와서 사용하는 것은 매우 좋은 거 같아요!
다만 filters의 값들과 updateLogics생성자의 key값들이 중복되고, 둘 다 String으로 갖고 있는 것은 별로 안 좋아보이네요.
차라리 Enum으로 만들고, .values메서드로 filters를 초기화하고, 생성자의 key값도 Enum을 통해 가져왔다면
변수 관리에 좀 더 좋지 않을까 싶네요!
| public class PassesUpdateLogic implements UpdateLogic { | ||
| @Override | ||
| public void update(Item item) { | ||
| item.sellIn -= 1; |
There was a problem hiding this comment.
모든 UpdateLogic 클래스의 메서드ㄷ들에서 item.sellIn -= 1;이라는 로직은 중복되는 거 같아요.
이 코드 로직에 변경이 생긴다면 모든 itemUpdate 메서드 수정이 필요할 수도 있겠네요.
이 부분 별도로 빼서 메서드화 하는 것에 대해 어떻게 생각하시나요?!
| item.sellIn -= 1; | ||
| item.quality = item.quality < 50 ? item.quality + 1 : 50; | ||
|
|
||
| if (item.sellIn <= 10) { |
There was a problem hiding this comment.
50, 10, 0, +1 등의 숫자들을 그대로 쓰고 계시군요.
만약에 Quality의 최대 값이 50에서 60으로 변경된다면 어떻게 될까요?
모든 코드를 다 확인하면서 50을 수정하면 매우 불편할 거 같아요.
이 부분에 대해서는 상수를 사용하면 좋을 거 같아서 레퍼런스 남겨둡니다!
https://prodo-developer.tistory.com/118
| item.quality = item.quality > 0 ? item.quality - 1 : 0; | ||
|
|
||
| if (item.sellIn < 0) { | ||
| item.quality = item.quality > 0 ? item.quality - 1 : 0; |
There was a problem hiding this comment.
2배로 떨어진다는 로직을 if문을 통해서 처리하셨군요!
그럼 만약에 2배가 아니라 3배로 떨어진다고 변경되면 어떨까요??
이 부분은 quality 변경량 * 판매하는 나머지 일수가 없어질 때 Quality 변경 가중치 등의 곱셈식으로 로직을 변경하는게 더 좋을 수 있을 거 같아요!
There was a problem hiding this comment.
피드백 감사합니다. 너무 제 위주로만 코드를 작성한 것 같네요. 수정해보겠습니다!
| } | ||
|
|
||
| public UpdateLogic findUpdateLogic(String name) { | ||
| if (name.contains("Aged Brie")) { |
There was a problem hiding this comment.
.contains가 계속 반복 되니 반복문을 사용해도 되지 않을까요?!
|
|
||
| import com.gildedrose.Item; | ||
|
|
||
| public abstract class UpdateLogic { |
There was a problem hiding this comment.
추상 클래스로 바꿔 중복되는 코드를 잘 처리하신 것 같습니다!
| import com.gildedrose.Item; | ||
|
|
||
| public abstract class UpdateLogic { | ||
| public static final int MAX_QUALITY = 50; |
| @@ -0,0 +1,5 @@ | |||
| package com.gildedrose.updateLogic; | |||
|
|
|||
| public enum ItemCategory { | |||
There was a problem hiding this comment.
혹시 왜 카테고리를 따로 만드신건지 알 수 있을까요?
There was a problem hiding this comment.
Item의 이름이 주어졌을 때, 이름 안에서 어떤 아이템인지 알아내는 문자열을 Map에 넣으려고 하는데, 그 문자열이 길이가 길다면, 사용하는 메모리가 늘어나서 좋지 않고, 가독성도 높이기 위해 카테고리를 만들었습니다!
…em의 종류를 매핑해주는 itemMap과 반복문을 사용하여 변경
| } | ||
| for (Item item : items) { | ||
| UpdateLogic updateLogic = updateLogics.findUpdateLogic(item.name); | ||
| updateLogic.update(item); |
There was a problem hiding this comment.
변수명과 클래스명을 잘지어서 어떤 로직을 수행하는지 직관적으로 보여요
| public abstract class UpdateLogic { | ||
| public static final int MAX_QUALITY = 50; | ||
| public static final int MIN_QUALITY = 0; | ||
| public static final int DOWN_MULTIPLE = 2; |
There was a problem hiding this comment.
혹시 enum과 private static final 중에 후자를 선택한 이유가 있으실까요?
enum이 상수화에 있어 이름의 중복, 선언의 긴 길이, 상수 값의 불명확성 등의 문제를 해결해주고, 상수 역할이 명확해진다고 생각해 사용했거든요.
영찬님의 의견이 궁금합니다.
There was a problem hiding this comment.
안녕하세요. 먼저, 시험이 오늘 끝난 터라 늦게 답변드린 점 죄송합니다.
치현님이 말씀해주신 enum 사용의 이유도 생각해보았지만, 개인적으로
- 클래스 내부에 변수를 선언하였을 때, 그 변수가 클래스에서 사용된다는 관계가 명확하게 보임
- 상수화를 하면, 중복을 피할 수는 있지만, 중복을 피하기 위해 길어지는 이름
- 사용할 때, 메소드를 사용하여 반환받아야 함
크게 위 3가지 이유로, 후자를 선택했었습니다.
|
|
||
| protected void decreaseSellIn(Item item) { | ||
| item.sellIn -= 1; | ||
| } |
There was a problem hiding this comment.
추상 클래스에 공통적인 기능을 수행하는 메서드를 따로 빼두셨네요.
배워갑니다 👍🏻👍🏻
| } | ||
|
|
||
| protected void decreaseQuality(Item item, int decreaseAmount) { | ||
| if (item.sellIn < 0) { |
There was a problem hiding this comment.
0 이라는 숫자가 문제에서는 "마감일까지 남은 일 수" 라고 정의되어 이것을 기준으로 분기를 했지만,
요구사항이 "마감일 하루 전까지" 로 바뀌게 되면 여러 군데를 바꿔야할 것 같아요.
의미를 갖는 매직 넘버는 상수화하는 것은 어떨까요?!
|
|
||
| public class UpdateLogicFactory { | ||
| private final Map<ItemCategory, UpdateLogic> updateLogicMap; | ||
| private final Map<String, ItemCategory> itemMap; |
There was a problem hiding this comment.
문자열로 받아온 아이템 이름에서 상수화된 수식어를 찾아 그에 맞는 상수를 반환하기 위해 정의한 것으로 보이는데요!
저는 이 역할은 수식어 상수 클래스에서 하는 것이 더 직관적이라고 생각해요. 업데이트 공장이라고 하면 업데이트와 관련된 책임을 갖고 있을 것 같고, 아이템 이름으로 특정 상수를 찾아줄 것이라고는 생각을 못했거든요.
영찬님 의견이 궁금해요 :)
There was a problem hiding this comment.
itemMap을 private로 선언하여 외부에서 직접 접근할 수 없고, itemMap을 사용하는 findUpdateLogic에서 ItemCategory를 찾는 용도로만 사용되기 때문에 괜찮다고 생각했는데, 치현님 말씀대로 분리하는 것이 더 좋은 것 같습니다. 감사합니다.
No description provided.