4
\$\begingroup\$

I have started programming a couple of months ago and have recently applied for an internship. One of the assignments was to create a console application, the assignment can be seen here - > HERE

My implementation is this:

I have created an abstract class called DiscountCard which holds all the info, getters and setters, and has one abstract method of calculateDiscountRate.

public abstract class DiscountCard {

    private String owner;
    private int turnover;
    private int purchaseValue;
    private double discountRate;


    public DiscountCard(int turnOver, int purchaseValue){

        setTurnover(turnOver);
        setPurchaseValue(purchaseValue);

    }

    abstract void calculateDiscountRate();

    public String getOwner() {
        return owner;
    }

    public void setOwner(String owner) {
        this.owner = owner;
    }

    public int getTurnover() {
        return turnover;
    }

    public void setTurnover(int turnOver) {

        if(turnOver>=0) {
            this.turnover = turnOver;
        }else{
            System.out.println("Turnover can't be less than 0.");
        }

    }

    public int getPurchaseValue() {
        return purchaseValue;
    }

    public void setPurchaseValue(int purchaseValue) {

        if(purchaseValue>=0) {
            this.purchaseValue = purchaseValue;
        }else{
            System.out.println("Purchase value can't be less than 0.");
        }
    }

    public void setDiscountRate(double discountRate) {
        this.discountRate = discountRate;
    }

    public double getDiscountRate() {
        return discountRate;
    }
}

Then, there are 3 Card classes (Bronze,Silver,Gold) which implement the above class and override its abstract method:

 package com.test.DiscountCards;

    public class GoldDiscountCard extends DiscountCard {


        public GoldDiscountCard(int turnOver, int purchaseValue){

            super(turnOver,purchaseValue);
            calculateDiscountRate();
        }

        @Override
        void calculateDiscountRate() {

            setDiscountRate(2);

            setDiscountRate(getTurnover()/100 + getDiscountRate());

            if(getDiscountRate()>10){
                setDiscountRate(10);
            }


        }


    }

Next is the StoreCalculator Class which takes an instance of DiscountCard class and calculates the discount and total.

package com.test;

import com.practice.DiscountCards.DiscountCard;

public class StoreCalculator {

    private DiscountCard discountCard;

    public StoreCalculator(DiscountCard discountCard){
        this.discountCard = discountCard;
    }

    public double calculateDiscount(){
        return discountCard.getPurchaseValue() * discountCard.getDiscountRate()/100;
    }

    public double calculateTotal(){
        return discountCard.getPurchaseValue() - calculateDiscount();
    }
}

And at last i have a class that takes the info from DiscountCard class and Calculator class and prints it to a console.

import com.test.DiscountCards.DiscountCard;

public class PayDesk {

    public static void print(DiscountCard discountCard){

        StoreCalculator calculator = new StoreCalculator(discountCard);

        System.out.println("Purchase value: $"+ discountCard.getPurchaseValue() );
        System.out.println("Discount rate: "+ discountCard.getDiscountRate()+"%" );
        System.out.println("Discount: $"+ calculator.calculateDiscount() );
        System.out.println("Total: $"+ calculator.calculateTotal());


    }
}

My question is if this code implements OOP and is there something i should improve. I have send this code for a review to a company, i just want to know if there can be an improvement so i can learn more.

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

The OO part seems to be OK when it comes to the type inheritance and the choice of an abstract class.

Code review.

For the abstract class

public DiscountCard(int turnOver, int purchaseValue){

This is a public constructor for a class that cannot be instantiated, consider protected instead. It also contains a turnOver, but that's specific to an owner it seems to me. So that doesn't make sense without the owner string. Similarly, I don't think the card is specific to one particular purchase. The card is applied on a particular purchase.

    setTurnover(turnOver);
    setPurchaseValue(purchaseValue);

Generally you'd set the fields directly, instead of calling the public methods. Calling methods can be dangerous, especially if they can be overridden.

abstract void calculateDiscountRate();

For what? That depends on purchaseValue but that should not depend on any class fields, as already established.

public String getOwner() {
    return owner;
}

Here you make a classic mistake: never leave your objects in an invalid state. As the owner may not be set, this method could return null.

public void setOwner(String owner) {
    this.owner = owner;
}

No. If the holder of the card can change then this would be in the model, and I don't see that. Generally cards are personalized and are therefore not transferable (at least from the payment desk's point of view).

public void setTurnover(int turnOver) {
    if(turnOver>=0) {
        this.turnover = turnOver;
    }else{
        System.out.println("Turnover can't be less than 0.");
    }
}

The turnover may actually be negative if the customer decided to return. In this kind of situations it is important to perform a reality check: what could happen. Furthermore, I guess that this should be called after each period of time (it's for the "previous month" according to the instructions. So call it, say, setTurnoverOfLastMonth() or something similar.

public void setDiscountRate(double discountRate) {
    this.discountRate = discountRate;
}

Is that really a public call, you override the discount rate? Seems to me that you'd have to switch cards for that.

For the gold card:

calculateDiscountRate();

By now you should have noticed that there are more rates possible, you are still trying to get along with one.

setDiscountRate(2);

setDiscountRate(getTurnover()/100 + getDiscountRate());

And you aren't just overwriting the single discount rate in the second call?

if(getDiscountRate()>10){
    setDiscountRate(10);
}

Leukoplast patching ain't gonna help, son.

For the store calculator

Where are the purchases? In the card?

I'd expect something like:

public Payment calculateResult(double purchaseValue);

Where the Payment contains all the details (card used, discount percentage, discount value and of course the final price).

Note too that the examples have a purchase value with .00 behind it. Somehow that indicates to me that every cent is appreciated (and you store as an int - which is actually fine if it means cents).

For the PayDesk

Just prints out stuff. Maybe correct, maybe it isn't, but just printing the bill is not what I was expecting. Probably can be merged with the StoreCalculator although I like the fact that you've created a separate calculator. That would certainly be a good idea for a more complicated PayDesk class.


Beware: it says in your requirements: "create instances with sample data as shown in Example outputs section;"

Now I don't personally think that the purchase should ever be part of the card data, but if it is stated this way I'd almost include it myself as a field.


Very non-nerdy of me, but I'd include a welcoming message for the card holder (Thank you for your purchase, ). Your GOLD CARD discount is: ... Etc... This would show your employer that you can act outside what is basic development.

And probably I'd also welcome non-card holders to make a purchase.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks a lot! Your answer is very helpful. One thing i am not sure i quite understand, about setting the DiscountRate. Should i set it directly in a constructor of a child card class and get rid of the setter? \$\endgroup\$ Feb 22, 2020 at 3:04
  • \$\begingroup\$ I don't think you should talk about one discount rate if you read the assignment carefully. Any information that is required to calculate the discount rate should be set when it becomes available. But it is known for the different cards even before they are constructed, right? This information could just be codified or placed - preferably - in a constant or two - three - four. \$\endgroup\$ Feb 22, 2020 at 16:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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