6
\$\begingroup\$

I need some help in working "Video Rental Store" application design optimization. The application describes the following tasks:

  • Inventory stores videos. Inventory can add/delete/change and receive all videos. Inventory has 3 types of films: Old, New, Regular.
  • Videos have a title, type (described above) and availability which will be changed to 'not available'/'available' if this video is rented by someone.
  • Customer can rent only available videos(not rented by someone) with additional option to rent film for bonus points which gathers every time when a customer rents a video (1 point for rent)
  • Before the customer rents a video he/she set renting days. The price for rent calculated automatically based on rent days, video type. After the price is calculated for this rent transaction than customer is presented terms where he/she has a choice to accept or reject them. If a customer accepts terms than rent transaction is executed successfully.

Data is not persisted anywhere, data exists in runtime. Inventory and RentalStore class generates some fake records for presentation.

Could you please review my app and offer some tips or optimization tricks?

In general, I'm not sure in RentalStore class which implements connections to the Inventory and RentalTransactions classes. I don't know how to implement elegant class which will work with rental transactions and will know what items have inventory. I read some principles of design patterns how to organize application that it will be flexible as much as possible but it seems I need more experience in design area. That is why I contact with you. Someone suggested me that it is possible to do it better than my way.

RentalStore class:

public interface RentalDAO {
void rentItem(Person person, Item item, int days, Boolean temsAcceptance);

List<RentalTransaction> getAllRentals();

List<RentalTransaction> getAllRentalsForCustomer(Person person);
}

public class RentalStore implements RentalDAO {

    private Inventory inventory;
    private List<RentalTransaction> rentalTransactions;

    public RentalStore() {
        this.inventory = new Inventory();
        this.rentalTransactions = new LinkedList<RentalTransaction>();
        generateFakeRentalTransactions();
    }

    public Inventory getInventory() {
        return inventory;
    }

    private void generateFakeRentalTransactions() {
        rentItem(new Customer("Kauri"), inventory.getAllAvailableItems().get(new Random().nextInt(inventory.getAllAvailableItems().size() - 1) + 1), new Random().nextInt(30) + 1, true);
        rentItem(new Customer("Lauri"), inventory.getAllAvailableItems().get(new Random().nextInt(inventory.getAllAvailableItems().size() - 1) + 1), new Random().nextInt(30) + 1, true);
    }

    @Override
    public void rentItem(Person person, Item item, int days, Boolean temsAcceptance) {
        if (item.isAvailable()) {
            RentalTransaction newRentalTransaction = new RentalTransaction(person, item, days, temsAcceptance);

            if (!rentalTransactions.contains(newRentalTransaction)) {
                rentalTransactions.add(newRentalTransaction);
            }
        }
    }

    @Override
    public List<RentalTransaction> getAllRentals() {
        return rentalTransactions;
    }

    @Override
    public List<RentalTransaction> getAllRentalsForCustomer(Person person) {
        List<RentalTransaction> rentalTransactionsForCustomer = new LinkedList<RentalTransaction>();
        for (RentalTransaction rentalTransaction : rentalTransactions) {
            if (rentalTransaction.getPerson().equals(person)) {
                rentalTransactionsForCustomer.add(rentalTransaction);
            }
        }
        return rentalTransactionsForCustomer;
    }

}

In RentalStore class I don't like also that rentItem() method has hardcoded Person argument. It doesn't take me, for instance, define this object is really Customer or, may be, employee. Also it doesn't take me to invoke person.chargeBonus() due to it concern only to Customer objects. I can do so (CUstomer) person.chargeBonus() but it can cause Exception in some cases and looks somehow bad, how to optimize it too?

RentalTransaction class:

public class RentalTransaction {

private static final double PREMIUM_FEE = 40;
private static final double REGULAR_FEE = 30;

private Person person;
private Item item;
private int days;
private double price;
private boolean termsAcceptance;

public RentalTransaction(Person person, Item item, int days, boolean termsAcceptance) {
    this.person = person;
    this.item = item;
    this.days = days;
    this.price = calculate();
    this.termsAcceptance = termsAcceptance;
}

public Person getPerson() {
    return person;
}

public Item getItem() {
    return item;
}

public int getDays() {
    return days;
}

public void setDays(int days) {
    this.days = days;
    this.price = calculate();
}

public double getPrice() {
    return price;
}

private double calculate() {
    switch (item.getType()) {
        case NEW_RELEASES:
            this.price = PREMIUM_FEE * days;
            break;
        case REGULAR:
            if (days > 3) {
                this.price = REGULAR_FEE + REGULAR_FEE * (days - 3);
            } else {
                this.price = REGULAR_FEE * days;
            }
            break;
        case OLD:
            if (days > 5) {
                this.price = REGULAR_FEE + REGULAR_FEE * (days - 5);
            } else {
                this.price = REGULAR_FEE * days;
            }
            break;
    }
    return this.price;
}

public boolean isTermsAccepted() {
    return termsAcceptance;
}

public void setTermsAcceptance(boolean termsAcceptance) {
    this.termsAcceptance = termsAcceptance;
}
}

Inventory class:

public class Inventory implements InventoryDAO {

private static String[] itemNames = {"Shrek", "Spider Man III", "Game of Thrones", "Mortal Combat"};

private List<Item> items;

public Inventory() {
    items = new LinkedList<Item>();
    for (String itemName : itemNames) {
        items.add(new Item(itemName, true, Type.getRandom()));
    }
}

@Override
public void addItem(Item item) {
    if (!items.contains(item))
        items.add(item);
}

@Override
public void removeItem(Item item) {
    if (items.contains(item))
        items.remove(item);
}

@Override
public void changeItemType(String title, Type type) {
    for (Item item : items) {
        if (item.getTitle().equals(title) && !item.getType().equals(type)) {
            item.setType(type);
        }
    }
}

@Override
public List<Item> getAllItems() {
    return items;
}

@Override
public List<Item> getAllAvailableItems() {
    List<Item> availableItems = new LinkedList<Item>();
    for (Item item : items) {
        if (item.isAvailable())
            availableItems.add(item);
    }

    return availableItems;
}
}
public interface InventoryDAO {
void addItem(Item item);

void removeItem(Item item);

void changeItemType(String title, Type type);

List<Item> getAllItems();

List<Item> getAllAvailableItems();
}

Another thing is Type class. Is it possible to do base enum Type class for Item and redefine it to concrete enum type in Video class, as example?

Type class:

public enum Type {
    NEW_RELEASES("New Releases"),
    REGULAR("Regular"),
    OLD("Old");

    private final String type;

    private Type(String type) {
        this.type = type;
    }

    public static Type getRandom() {
        return values()[(int) (Math.random() * values().length)];
    }


    @Override
    public String toString() {
        return type;
    }
}

Where I use type(is it possible to make base enum Type class for redefining in Video class?):

public class Item {
    private String title;
    private boolean avability;
    private Type type;

    public Item(String title, Boolean avability, Type type) {
        this.title = title;
        this.avability = avability;
        this.type = type;
    }

    public String getTitle() {
        return title;
    }

    public void setTitle(String title) {
        this.title = title;
    }

    public Boolean isAvailable() {
        return avability;
    }

    public void setAvability(boolean avability) {
        this.avability = avability;
    }

    public Type getType() {
        return type;
    }

    public void setType(Type type) {
        this.type = type;
    }
}

Any suggestions?

\$\endgroup\$
  • \$\begingroup\$ I like your code's architecture \$\endgroup\$ – Christopher Francisco May 29 '14 at 18:55
  • \$\begingroup\$ Unfortunately, I don't. I can try to explain you what and why I don`t like it. Right now only, for example, RentalStore knows about inventory and rental transactions. When I need, for instance, remove item from inventory I need check we have any rental transactions. If no such a transaction then delete it otherwise do not delete this item. How can I see from inventory all transactions which are not available from this place directly. I can create observer pattern to notify RentalStore when I am trying to delete something but it looks too difficult in my opinion. \$\endgroup\$ – Viktor V. May 29 '14 at 19:15
  • \$\begingroup\$ Another moment, look at RentalStore -> rentItem -> customer.chargeBonus(item.getType()); There is hardcoded customer object because I need chargeBonus() method. What will happen if I pass Employee object here? It doesn`t flexible here. In general, we can design the application +100500 ways and it will work but not in all cases it will be optimized and flexible as much as possible. \$\endgroup\$ – Viktor V. May 29 '14 at 19:18
  • \$\begingroup\$ inventory doesn't need to knows about the transaction. RentalStore here is built as a Facade Pattern, ergo, RentalStore should check of ongoing rentals and then decide to remove or not the item \$\endgroup\$ – Christopher Francisco May 29 '14 at 19:19
  • \$\begingroup\$ That is to say I should write the same Inventory methods in RentalStore which will lead to already implemented methods in Inventory? You mean this way: RentalStore method -> removeItem(Item item) {inventory.removeItem(item);} But we will have transactions check inside RentalStore->removeItem(...) \$\endgroup\$ – Viktor V. May 29 '14 at 19:30
5
\$\begingroup\$

I think you are getting lost in a bunch of methods you are implementing before you have a real justification for them. So let's just ignore those for the moment, and concentrate on the bits that you have a handle on (in the expectation that things will become clearer).

private double calculate() {
    switch (item.getType()) {
        case NEW_RELEASES:
            this.price = PREMIUM_FEE * days;
            break;
        case REGULAR:
            if (days > 3) {
                this.price = REGULAR_FEE + REGULAR_FEE * (days - 3);
            } else {
                this.price = REGULAR_FEE * days;
            }
            break;
        case OLD:
            if (days > 5) {
                this.price = REGULAR_FEE + REGULAR_FEE * (days - 5);
            } else {
                this.price = REGULAR_FEE * days;
            }
            break;
    }
    return this.price;
}

First code smell - you are using a switch to change the behavior of your program. That usually means there's an abstraction that you are missing, often something that uses the StrategyPattern, and some sort of selector.

private double calculate() {
    PricingPolicy pricingPolicy = getPricingPolicy(item);
    return pricingPolicy.getPrice(days);
}

class PerDiemPricingPolicy implements PricingPolicy {
    private final double perDiem;

    double getPrice(int rentalDays) {
      return perDiem * rentalDays;
    }
}

static final PricingPolicy DEFAULT_PRICING_POLICY = new PerDiemPricingPolicy(REGULAR_FEE); 
static final PricingPolicy NEW_RELEASE_PRICING = new PerDiemPricingPolicy(PREMIUM_FEE);

class DiscountedPricingPolicy implements PricingPolicy {
    private final int discountPeriod;
    private final PricingPolicy pricingPolicy;

    double getPrice(int days) {
        if (days > discountPeriod) {
            days -= discountPeriod - 1;
        }
        return pricingPolicy(days);
    }
}

static final PricingPolicy REGULAR_PRICING = new DiscountedPricingPolicy(3,DEFAULT_PRICING_POLICY);
static final PricingPolicy OLD_PRICING = new DiscountedPricingPolicy(5, DEFAULT_PRICING_POLICY);

Now getPricingPolicy(Item) might be implemented in any of a number of ways; it could be the switch again, it could be a Map, it could be a Guava Function, it could be a state machine.

At the moment, your policies are all determined by Type (which should probably be an extensible enum), so you know that there needs to be some way to convert Item to Type -- but that doesn't necessarily mean that Type is a property of Item. Again, a guava Function is a good way to insulate your design against a change here.

I think your inventory class is a mess, because you haven't actually tried to use it. My suggestion would be to write out a test case that rents an item to a person. Forget about crud, forget about DAO, just write "Lauri rents Mortal Kombat", and see if Manager Bob can figure out what happened from the TPS report.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for help. Price calculator looks great now. It seems I need more practise to see all of this design patterns. Somewhere it`s a bit hard to see it. As for Inventory class, it seems you saw a bit outdated version of RentalStore and Inventory classes. Check it here Is it still too mess? Just take me know if it's incorrect from a design pattern. I'm a bit confuse how to create a test case where will be executed rentItem method which is not used my DAO and so on. I tried to do what you say, look at my testRentItem \$\endgroup\$ – Viktor V. May 30 '14 at 0:19
  • \$\begingroup\$ One more question to you, is it possible to do MethodExecutorByUserCommand on the same Strategy pattern but avoiding too much classes creation for each kind of methods? I want to get to a user a choice to execute any command which has StoreDAO at this moment. For instance, you type a number from 1-9 which concerns to a concrete method executor. \$\endgroup\$ – Viktor V. May 30 '14 at 15:20
  • \$\begingroup\$ I don't know - the contraint "avoiding too much classes creation" is poorly specified. Strategy Pattern is about having more than one way to do the same thing, so that sounds like a poor fit. 1-9 different things sounds like the Command pattern - which is superficially similar to Strategy. Both tend to use small classes that do one thing, which means many classes when there are many things to do. Hard to say. I'd suggest creating a new topic for this specific example, to see what the wisdom of the crowds has to offer. \$\endgroup\$ – VoiceOfUnreason May 30 '14 at 17:13
  • \$\begingroup\$ OK, I will create new topic for this question - command patter question. What you can say about my Inventory class. You said it's not used but it actually used by RentalStore to get and add items. Probably, it's not the best way to use it so. Especially that fact RentalStore has partially the same methods from Inventory class. It looks a bit strange in my opinion. But what you can recommend me in this case? Just remove this class and use only RentalStore class or how? \$\endgroup\$ – Viktor V. May 30 '14 at 18:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.