4
\$\begingroup\$

I was trying to achieve a simple problem :

  • If the user is an employee of the store, he gets a 30% discount
  • If the user is an affiliate of the store, he gets a 10% discount

I came up with the following :

The abstract super class :

abstract class Customer {   
String cid; 
String customer_name;
double fare;    

Customer(){
    this.cid="NA";
    this.customer_name="NA";
    this.fare=0.0d;
}
protected String getCid() {
    return cid;
}

protected String getCustomer_name() {
    return customer_name;
}

protected abstract double getFare();
protected abstract void printDetails();    
}

The child class :

class CustomerAffiliated extends Customer{

    CustomerAffiliated(String cid,String customer_name,double fare){

        super();
        this.cid=cid;
        this.customer_name=customer_name;
        this.fare=fare; 
    }

    @Override
    protected double getFare() {

        return this.fare-((10.0/100.0) * this.fare);        
    }

    @Override
    protected void printDetails() {
        System.out.println(this.getCid());
        System.out.println(this.getCustomer_name());
        System.out.println(this.getFare()); 
    }       
}

And like wise the other child class. Calling the methods something like :

Customer c;
    c=new CustomerAffiliated("1","a",100);
    c.printDetails();

Am I doing it in a right way? I did not preferred composition as it is (is a) relation. Moreover I am able to use all the fields in the parent class. Is is the correct approach ?

\$\endgroup\$
3
\$\begingroup\$

JavaDoc


Your whitespaces are all messed up, sometimes there are spaces, sometimes not, sometimes there's a new line after a function declaration, sometimes not. Be consistent, even better, use a code formatter.


abstract class Customer {
    String cid; 
    String customer_name;
    double fare;

Why is everything package-private? That class should most likely be public and the variables private or protected.


String cid; 
String customer_name;

Both names are not very good:

  1. The first one is abbreviated, the second isn't, be consistent.
  2. The customer prefix is superfluous, as it is a field in the class Customer.

double fare;

You're representing money with a double, this is dangerous! Use a BigDecimal or a similar class instead.


protected String getCid() {
protected String getCustomer_name() {
protected abstract double getFare();
protected abstract void printDetails();

Why are all these methods protected, shouldn't they be public?


CustomerAffiliated(String cid,String customer_name,double fare){

    super();
    this.cid=cid;
    this.customer_name=customer_name;
    this.fare=fare; 
}

There should be a consutructor in the Customer class which accepts these values. This makes sure that less knowledge is used when extending the class, additionally you could transform the values in the original constructor.


@Override
protected void printDetails() {
    System.out.println(this.getCid());
    System.out.println(this.getCustomer_name());
    System.out.println(this.getFare()); 
}

Printing directly to stdout is bad, you should return a String so that the system which invokes the method can decide where the output goes to. Ideally you could remove this function favor of overriding toString.


Customer c;
c=new CustomerAffiliated("1","a",100);
c.printDetails();

Why assigning it on the next line if you could do it on the same line? Why abbreviating the name of the class? It makes the code only unnecessary hard to read, imagine the following, exaggerated, example:

for (Customer c : l) {
    double f = c.getFare();

    if (f > 0) {
        a.add(c);
    }
}

Not exactly easy to read, is it? You can't take one line and take a guess what's going on. Let's try that again with long names:

for (Customer customer : customers) {
    double customerFare = customer.getFare();

    if (customerFare > 0) {
        customersWithFare.add(customer);
    }
}

Your structure makes somewhat sense, depending on what you're trying to achieve. It's hard to say if this is the best approach without knowing anything about the rest of the system. But as a more generic approach having a Customer class with a discount and type field would be more efficient in the real world (as most of the time the data for customers comes from a database which has now idea about some class structure).

public class Customer
    public String getId()
    public String getName()
    public BigDecimal getFare()
    public BigDecimal getDiscount()
    public CustomerType getType()

public enum CustomerType
    NORMAL
    AFFILIATED

This is not without drawbacks itself, but reduces the need to know about and create different classes. Technically you could also hardcode the discount value into the enum, always depending on what you're trying to achieve.

\$\endgroup\$
0

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.