-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.

tail
  • 101
  • 1
  • 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 – Ben Cottrell Jan 09 '23 at 13:13
  • Worth reading Uncle Bob's blog which explains this in more words: https://blog.cleancoder.com/uncle-bob/2014/05/08/SingleReponsibilityPrinciple.html – Ben Cottrell Jan 09 '23 at 13:16

1 Answers1

0

Yes, it obviously has two responsibilities.

  1. Updating the quality
  2. Determining the Rate

Make getRate private.

Ewan
  • 70,664
  • 5
  • 76
  • 161