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

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 8 additions & 49 deletions Java/src/main/java/com/gildedrose/GildedRose.java
Original file line number Diff line number Diff line change
@@ -1,62 +1,21 @@
package com.gildedrose;

import com.gildedrose.updateLogic.UpdateLogic;
import com.gildedrose.updateLogic.UpdateLogicFactory;

class GildedRose {
Item[] items;
UpdateLogicFactory updateLogics;

public GildedRose(Item[] items) {
this.items = items;
this.updateLogics = new UpdateLogicFactory();
}

public void updateQuality() {
for (int i = 0; i < items.length; i++) {
if (!items[i].name.equals("Aged Brie")
&& !items[i].name.equals("Backstage passes to a TAFKAL80ETC concert")) {
if (items[i].quality > 0) {
if (!items[i].name.equals("Sulfuras, Hand of Ragnaros")) {
items[i].quality = items[i].quality - 1;
}
}
} else {
if (items[i].quality < 50) {
items[i].quality = items[i].quality + 1;

if (items[i].name.equals("Backstage passes to a TAFKAL80ETC concert")) {
if (items[i].sellIn < 11) {
if (items[i].quality < 50) {
items[i].quality = items[i].quality + 1;
}
}

if (items[i].sellIn < 6) {
if (items[i].quality < 50) {
items[i].quality = items[i].quality + 1;
}
}
}
}
}

if (!items[i].name.equals("Sulfuras, Hand of Ragnaros")) {
items[i].sellIn = items[i].sellIn - 1;
}

if (items[i].sellIn < 0) {
if (!items[i].name.equals("Aged Brie")) {
if (!items[i].name.equals("Backstage passes to a TAFKAL80ETC concert")) {
if (items[i].quality > 0) {
if (!items[i].name.equals("Sulfuras, Hand of Ragnaros")) {
items[i].quality = items[i].quality - 1;
}
}
} else {
items[i].quality = items[i].quality - items[i].quality;
}
} else {
if (items[i].quality < 50) {
items[i].quality = items[i].quality + 1;
}
}
}
for (Item item : items) {
UpdateLogic updateLogic = updateLogics.findUpdateLogic(item.name);
updateLogic.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.

변수명과 클래스명을 잘지어서 어떤 로직을 수행하는지 직관적으로 보여요

}
}
}
16 changes: 16 additions & 0 deletions Java/src/main/java/com/gildedrose/updateLogic/BrieUpdateLogic.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package com.gildedrose.updateLogic;

import com.gildedrose.Item;

public class BrieUpdateLogic extends UpdateLogic {
@Override
public void update(Item item) {
decreaseSellIn(item);

if (item.sellIn < 0) {
increaseQuality(item, 2);
} else {
increaseQuality(item, 1);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.gildedrose.updateLogic;

import com.gildedrose.Item;

public class ConjuredUpdateLogic extends UpdateLogic {
@Override
public void update(Item item) {
decreaseSellIn(item);

decreaseQuality(item, 2);
}
}
12 changes: 12 additions & 0 deletions Java/src/main/java/com/gildedrose/updateLogic/ElseUpdateLogic.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.gildedrose.updateLogic;

import com.gildedrose.Item;

public class ElseUpdateLogic extends UpdateLogic {
@Override
public void update(Item item) {
decreaseSellIn(item);

decreaseQuality(item, 1);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package com.gildedrose.updateLogic;

public enum ItemCategory {
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 Author

Choose a reason for hiding this comment

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

Item의 이름이 주어졌을 때, 이름 안에서 어떤 아이템인지 알아내는 문자열을 Map에 넣으려고 하는데, 그 문자열이 길이가 길다면, 사용하는 메모리가 늘어나서 좋지 않고, 가독성도 높이기 위해 카테고리를 만들었습니다!

BRIE, PASS, CONJURED, SULFURAS, ELSE
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package com.gildedrose.updateLogic;

import com.gildedrose.Item;

public class PassesUpdateLogic extends UpdateLogic {
@Override
public void update(Item item) {
decreaseSellIn(item);

if (item.sellIn < 0) {
resetQuality(item);
} else if (item.sellIn <= 5) {
increaseQuality(item, 3);
} else if (item.sellIn <= 10) {
increaseQuality(item, 2);
} else {
increaseQuality(item, 1);
}

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package com.gildedrose.updateLogic;

import com.gildedrose.Item;

public class SulfurasUpdateLogic extends UpdateLogic {
@Override
public void update(Item item) {

}
}
31 changes: 31 additions & 0 deletions Java/src/main/java/com/gildedrose/updateLogic/UpdateLogic.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package com.gildedrose.updateLogic;

import com.gildedrose.Item;

public abstract class UpdateLogic {
Copy link
Member

Choose a reason for hiding this comment

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

추상 클래스로 바꿔 중복되는 코드를 잘 처리하신 것 같습니다!

public static final int MAX_QUALITY = 50;
Copy link
Member

Choose a reason for hiding this comment

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

고정 변수도 좋은 거 같아요!

public static final int MIN_QUALITY = 0;
public static final int DOWN_MULTIPLE = 2;
Copy link
Member

Choose a reason for hiding this comment

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

혹시 enum과 private static final 중에 후자를 선택한 이유가 있으실까요?
enum이 상수화에 있어 이름의 중복, 선언의 긴 길이, 상수 값의 불명확성 등의 문제를 해결해주고, 상수 역할이 명확해진다고 생각해 사용했거든요.
영찬님의 의견이 궁금합니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

안녕하세요. 먼저, 시험이 오늘 끝난 터라 늦게 답변드린 점 죄송합니다.

치현님이 말씀해주신 enum 사용의 이유도 생각해보았지만, 개인적으로

  1. 클래스 내부에 변수를 선언하였을 때, 그 변수가 클래스에서 사용된다는 관계가 명확하게 보임
  2. 상수화를 하면, 중복을 피할 수는 있지만, 중복을 피하기 위해 길어지는 이름
  3. 사용할 때, 메소드를 사용하여 반환받아야 함

크게 위 3가지 이유로, 후자를 선택했었습니다.


protected void decreaseSellIn(Item item) {
item.sellIn -= 1;
}
Copy link
Member

Choose a reason for hiding this comment

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

추상 클래스에 공통적인 기능을 수행하는 메서드를 따로 빼두셨네요.
배워갑니다 👍🏻👍🏻


protected void decreaseQuality(Item item, int decreaseAmount) {
if (item.sellIn < 0) {
Copy link
Member

@ch1hyun ch1hyun Oct 10, 2024

Choose a reason for hiding this comment

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

0 이라는 숫자가 문제에서는 "마감일까지 남은 일 수" 라고 정의되어 이것을 기준으로 분기를 했지만,
요구사항이 "마감일 하루 전까지" 로 바뀌게 되면 여러 군데를 바꿔야할 것 같아요.
의미를 갖는 매직 넘버는 상수화하는 것은 어떨까요?!

item.quality = Math.max(item.quality - decreaseAmount * DOWN_MULTIPLE, MIN_QUALITY);
} else {
item.quality = Math.max(item.quality - decreaseAmount, MIN_QUALITY);
}
}

protected void increaseQuality(Item item, int increaseAmount) {
item.quality = Math.min(item.quality + increaseAmount, MAX_QUALITY);
}

protected void resetQuality(Item item) {
item.quality = 0;
}

public abstract void update(Item item);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package com.gildedrose.updateLogic;

import static com.gildedrose.updateLogic.ItemCategory.*;

import java.util.HashMap;
import java.util.Map;

public class UpdateLogicFactory {
private final Map<ItemCategory, UpdateLogic> updateLogicMap;
private final Map<String, ItemCategory> itemMap;
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 Author

Choose a reason for hiding this comment

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

itemMap을 private로 선언하여 외부에서 직접 접근할 수 없고, itemMap을 사용하는 findUpdateLogic에서 ItemCategory를 찾는 용도로만 사용되기 때문에 괜찮다고 생각했는데, 치현님 말씀대로 분리하는 것이 더 좋은 것 같습니다. 감사합니다.


public UpdateLogicFactory() {
updateLogicMap = new HashMap<>();
itemMap = Map.ofEntries(Map.entry("Aged Brie", BRIE), Map.entry("Backstage passes", PASS),
Map.entry("Conjured", CONJURED), Map.entry("Sulfuras", SULFURAS));

updateLogicMap.put(BRIE, new BrieUpdateLogic());
updateLogicMap.put(PASS, new PassesUpdateLogic());
updateLogicMap.put(CONJURED, new ConjuredUpdateLogic());
updateLogicMap.put(SULFURAS, new SulfurasUpdateLogic());
updateLogicMap.put(ELSE, new ElseUpdateLogic());
}

public UpdateLogic findUpdateLogic(String name) {
for (String key : itemMap.keySet()) {
if (name.contains(key)) {
return updateLogicMap.get(itemMap.get(key));
}
}
return updateLogicMap.get(ELSE);
}
}
17 changes: 0 additions & 17 deletions Java/src/test/java/com/gildedrose/GildedRoseTest.java

This file was deleted.

21 changes: 11 additions & 10 deletions Java/src/test/java/com/gildedrose/TexttestFixture.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,17 @@ public static void main(String[] args) {
System.out.println("OMGHAI!");

Item[] items = new Item[] {
new Item("+5 Dexterity Vest", 10, 20), //
new Item("Aged Brie", 2, 0), //
new Item("Elixir of the Mongoose", 5, 7), //
new Item("Sulfuras, Hand of Ragnaros", 0, 80), //
new Item("Sulfuras, Hand of Ragnaros", -1, 80),
new Item("Backstage passes to a TAFKAL80ETC concert", 15, 20),
new Item("Backstage passes to a TAFKAL80ETC concert", 10, 49),
new Item("Backstage passes to a TAFKAL80ETC concert", 5, 49),
// this conjured item does not work properly yet
new Item("Conjured Mana Cake", 3, 6) };
new Item("+5 Dexterity Vest", 10, 20), //
new Item("Aged Brie", 2, 0), //
new Item("Elixir of the Mongoose", 5, 7), //
new Item("Sulfuras, Hand of Ragnaros", 0, 80), //
new Item("Sulfuras, Hand of Ragnaros", -1, 80),
new Item("Backstage passes to a TAFKAL80ETC concert", 15, 20),
new Item("Backstage passes to a TAFKAL80ETC concert", 11, 40),
new Item("Backstage passes to a TAFKAL80ETC concert", 6, 40),
new Item("Backstage passes to a TAFKAL80ETC concert", 0, 40),
// this conjured item does not work properly yet
new Item("Conjured Mana Cake", 3, 6)};

GildedRose app = new GildedRose(items);

Expand Down