-2

Goal: I am learning about SOLID principles and trying to refactor Gilded Rose code, in order to make it fresh and clean.

What I have done: I have created an AbstractItem class as follows, which "wraps" Item class:

public abstract class AbstractItem {
    protected static final int MIN_QUALITY = 0;
    protected static int MAX_QUALITY = 50;
    public Item item;  // name, quality and sellIn

    public AbstractItem(Item item) {
        this.item = item;
    }

    public abstract void updateQuality();

    public boolean isValid(int quality) {
        return quality > MIN_QUALITY && quality < MAX_QUALITY;
    }

    public void updateSellIn() {
        item.sellIn--;
    }

    public boolean hasExpired() {
        return item.sellIn < 0;
    }

    public int getRate() {
        return hasExpired() ? 2 : 1;
    }
}

and it gets extended by AgedBrie, Sulfuras, BackstagePasses and any other item which is sold by the Gilded Rose. For example, if Gilded Rose will sell MagicApples in 2050, I will define the MagicApples class which extends AbstractItem.

My doubt: I was wondering whether this class violates the SRP (or any other SOLID principles) or you think this is ugly or not:

public class BackstagePasses extends AbstractItem {

    public BackstagePasses(Item item) {
        super(item);
    }

    @Override
    public void updateQuality() {
        if (isValid(item.quality)) {
            if (hasExpired())
                item.quality = 0;
            else
                item.quality += getRate();
        }
    }

    @Override
    public int getRate() {
        int daysToConcert = item.sellIn;
        // should I refactor the following statements?
        if (daysToConcert <= 5)
            return 3;
        else if (daysToConcert <= 10)
            return 2;
        else
            return 1;
    }
}

Thank you for your advice and patience.

2
  • 1
    Single Responsibility (along with the other SOLID principles) are all entirely context-dependent. A Responsibility is something which is up to you to decide on a case-by-case basis by understanding your functional/behavioural requirements as well as the different needs and expectations of the people who ultimately decide those requirements (for example, different users, product owners, stakeholders, people who use/support the system operationally, etc). The only people who can make decisions about where to draw lines of responsibility are those who understand the project at a deeper level Commented Jan 9, 2023 at 13:13
  • 1
    Worth reading Uncle Bob's blog which explains this in more words: blog.cleancoder.com/uncle-bob/2014/05/08/… Commented Jan 9, 2023 at 13:16

1 Answer 1

0

Yes, it obviously has two responsibilities.

  1. Updating the quality
  2. Determining the Rate

Make getRate private.

6
  • Cannot make it private since it overrides a public method from its base class. Commented Jan 10, 2023 at 18:01
  • base class has the same issue, rate doesnt need to be public Commented Jan 10, 2023 at 18:31
  • AbstractItem.getRate() cannot be private since it gets overridden Commented Jan 10, 2023 at 18:35
  • dont override it Commented Jan 10, 2023 at 20:23
  • I have to, since its behavior depends on many subclasses! Commented Jan 11, 2023 at 9:21

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.