3
\$\begingroup\$

Exercise:

A car dealer wants a computer system to manage the data of their vehicles and classify them by type.

  • All cars have the following data

    1. Engine serial number
    2. Brand
    3. Year
    4. Price
  • The vehicles are classified into compact cars, luxury cars, trucks and wagons.

  • For cars and wagons, it is also important to store the number of passengers; While for trucks the load capacity should be controlled in kgs. And the number of axes and rounds.

  • Model this system and instantiate each of the classes, assigning data through their respective properties. Add a parameterized constructor to each class to initialize its data and invoke the base class constructor from the constructor of each derived class (do not use default constructors).

  • Implement the override of the toString() method to display the data for each type of auto. NOTE: Duplicate components are not allowed in classes or empty classes (no elements).

I'm just looking for some constructive (harsh) criticism of a homework I've done (the code works fine). Note that I didn't use getters and setters at all and I used private fields in the classes.

interface Passengers {
void setPassengers(int c);
int getPassengers();
}
abstract class Vehicle {
private int numSerieMotor;
private int year;
private String brand;
private double price;



Vehicle(int numSerieMotor, int year, String brand, double price) {

    this.numSerieMotor = numSerieMotor;
    this.year = year;
    this.brand = brand;
    this.price = price;
}

public String toString() {
    return " numSerieMotor=" + numSerieMotor + ", year=" + year + ", 
brand=" + brand + ", price="
            + price;
}

}

abstract class VehicPsj extends Vehicle implements Passengers {

private int psj;



VehicPsj(int numSerieMotor, int year, String brand, double price, int 
psj) {
    super(numSerieMotor, year, brand, price);
    this.psj = psj;

}

public void setPassengers(int c) {
    psj = c;
}

public int getPassengers() {
    return psj;
}

public String toString() {
    return super.toString() + "Passegers: " + psj;
}

}

class Acmpct extends VehicPsj {

Acmpct(int numSerieMotor, int year, String brand, double price, int 
psj) {
    super(numSerieMotor, year,brand, price, psj);

}
}

class Alux extends VehicPsj {

Alux(int numSerieMotor, int year, String brand, double price, int 
psj) {
    super(numSerieMotor, year, brand, price, psj);

}
}

class Vagon extends VehicPsj {



Vagon(int numSerieMotor, int year, String brand, double price, int 
psj) {
    super(numSerieMotor, year, brand, price, psj);

}
}

class Truck extends Vehicle {

private int capCharge;
private int axes;
private int wheel;

Truck(int numSerieMotor, int year, String brand, double price, int 
capCharge, int axes,
        int wheel) {
    super(numSerieMotor, year,brand, price);
    this.capCharge = capCharge;
    this.axes = axes;
    this.wheel = wheel;
}

public String toString() {
    return "Truck: capCharge=" + capCharge+ ", 
axes=" + axes
            + ",wheels=" + wheel + 
super.toString();
}
}

public class Main {

public static void main(String[] args) {
    Scanner x = new Scanner(System.in);

    Vehicle v[] = new Vehicle[4];

    System.out.println("Vagon data:" + "\n");

    System.out.println("motor code:");
    int numSerieMotor = x.nextInt();
    System.out.println("brand");
    x.nextLine();
    String brand = x.nextLine();
    System.out.println("year");
    int year = x.nextInt();
    System.out.println("price");
    double price = x.nextDouble();
    System.out.println("passengers");
    int passengers = x.nextInt();

    v[0] = new Vagon(numSerieMotor, year, brand, price, passengers);
    x.nextLine();
    System.out.println("luxury car data:" + "\n");

    System.out.println("motor code");
    int a = x.nextInt();
    System.out.println("brand");
    x.nextLine();
    String b = x.nextLine();
    System.out.println("year:");
    int c = x.nextInt();
    System.out.println("price:");
    double d = x.nextDouble();
    System.out.println("passengers:");
    int e = x.nextInt();

    v[1] = new Alux(a, c, b, d, e);
    x.nextLine();
    System.out.println("compact auto data:" + "\n");

    System.out.println("motor code");
    int f = x.nextInt();
    System.out.println("brand");
    x.nextLine();
    String g = x.nextLine();
    System.out.println("year");
    int h = x.nextInt();
    System.out.println("price:");
    double i = x.nextDouble();
    System.out.println("passengers:");
    int j = x.nextInt();

    v[2] = new Acmpct(f, h, g, i, j);
    x.nextLine();
    System.out.println(" truck data:" + "\n");

    System.out.println("motor code");
    int k = x.nextInt();
    System.out.println("brand");
    x.nextLine();
    String l = x.nextLine();
    System.out.println("year:");
    int m = x.nextInt();
    System.out.println("price:");
    double n = x.nextDouble();

    System.out.println("charge capacity:");
    int p = x.nextInt();
    System.out.println("axes:");
    int q = x.nextInt();
    System.out.println("wheels:");
    int r = x.nextInt();

    v[3] = new Truck(k, m, l, n, p, q, r);
    x.nextLine();
    for (int z = 0; z < 4; z++) {
        System.out.println(v[z].toString() + " ");
    }

}
}
\$\endgroup\$
  • \$\begingroup\$ Note: I've changed some name variables but it doesn't matter too much, it's due the translation, the code was originally in spanish. \$\endgroup\$ – user178403 Sep 3 '17 at 23:54
  • \$\begingroup\$ Try using an enum to hold your vehicle classifications. \$\endgroup\$ – d33j Sep 4 '17 at 4:05
  • \$\begingroup\$ what is an enum? @d33j \$\endgroup\$ – user178403 Sep 4 '17 at 4:12
  • 2
    \$\begingroup\$ don't use enum, it not gonna give you anything. \$\endgroup\$ – user902383 Sep 4 '17 at 10:59
7
\$\begingroup\$

(Since you're still a student, I might go over the top with some explanations.)

Code Style

  • Always format your code. Always. (CTRL+SHIFT+F in eclipse and CTRL+ALT+L in intellij). There's also the possibility to activate the formatting for save actions. Use the correct indentions and use empty lines (but not three empty lines).

Naming things

The naming of the classes and variables can be improved. Naming things is essential. It may sound stupid, but it's not uncommon that at least two people discuss the name of the class. And then, if you got the name, you ask another one: "If you read the class name WhateverThingerClass, what do you think it does"? The background is: You spend an awful lot of time reading and analyzing code. It can happen, that you need 4 days of reading code, and then a single day to implement the feature. And if the code is not self explanatory, you need more time to understand what's going on. It's not only about maintainability, it's also very important from an economic perspective.

  • Vehicle: If a class is abstract, you usually use abstract as prefix: AbstractVehicle
  • psj: Shouldn't that be passenger? Better: amountOfPassengerSeats.
  • Passengers: In 99% of the time, a class name should a singular substantive. Also, if you read a class name Passenger, what do you think it does?
  • setPassengers(int c): What's c? And shouln't it be setAmountOfPassengerSeats? Reads like you are putting actual passengers into the vehicle.
  • numSerieMotor: I'm sure, this should be something engineSerialNumber
  • year: Production year or model year?
  • Acmpct: That really looks like at least one abbreviation. A general rule is, not to use abbreviations at all. An exception can be for instance, if it's a widley known business term.
  • VehicPsj: Again, two abbreviations
  • v[3] = new Truck(k, m, l, n, p, q, r); Without looking at the code: What is this line doing? Of course, that was a rhetorical question. new Truck(loadCapacity, modelYear, amountOfAxes, etc) would really help.

Too many arguments

I don't think it's the scope of your assigment, but since we're here: When there are too many constructor arguments, you write for instance a VehicleBuilder, see "Builder Pattern". You then do stuff like

new VehicleBuilder()
    .withLoadCapacity(1)
    .withModelYear(2012)
    .build()

It's much easier to use and understand.

toString()

For instance in the Truck type, you append super.toString() to the return value. Is there any reason for that?

@Override

You're overwriting the toString method. Always use the @Override annotation for overwritten methods. It helps the reader to see, that it actually is overwritten, and, the compiler actually checks, if the method is actually from a super class (if you mess things up)

Liskov / inheritance

Now, I'm pretty sure, this isn't within the scope of your assignment either. In your main method, you got an array of Vehicles, at the end you iterate over it. Now if you were to know, how many passenger seats there are, you will be only able to do that, if do something like if(Vehicle instanceof Passenger). If you add another interface, for instance Three-Wheeler and want to get information out of that, you add another if clause. That's a violation of the Liskov Substitution Principle, or, in other words: bad inheritance. (You should look that up, it's a object oriented core principle).

Now, I really dislike inheritance, in general, it's often a pain in the long run. I would have hidden the sub classes behind a static factory or something and provide an API like

VehicleFactory.createCompactCar(...): Vehicle

I'd provided additional methods like hasLoadCapacity in the base class.

But since it's a home work assignment doesn't target real world problems, but wants to teach you something, I won't go into more detail.

Hope this helps,

slowy

\$\endgroup\$
2
\$\begingroup\$

I agree with the statements made by those above.

You have opted for the template pattern (predominantly at least):

There are costs and benefits with your approach, which as I understand is essentially this.

Abstract class: Vehicle, from which the following are (eventually) derived:

  1. Compact
  2. Luxury
  3. Truck
  4. Wagon

And the fields of the abstract parent class: 1. Engine Serial Number 2. Brand 3. Year 4. Price

The Template pattern vs Composition approach – pros and cons

  1. Template good when adding more sub-classes without much work
  2. Template bad if you want to change base class in future and this change does not apply to all sub-classes

The template pattern solution which you’ve gone for is great – especially if you need to add further vehicles – you can do so relatively easy – but only so long as they match exactly with the public interface of the Vehicle abstract class. In other words, all subclasses must: have an engine serial number, a brand, a year and a price. If not, then you cannot readily subclass from the Vehicle class. If you do, then you’re inviting problems for developers down the line, and for those maintaining your code base – you’ll be violating the Liskov substitution principle. This principle suggests to all future developers that they are entitled to believe that Wagon/Truck/LuxuryCar/CompactCar acts like a Vehicle if it is subclassed from the abstract vehicle class. In other words, the risk you take is that the parent class applies to all subclasses.

  1. Elaboration on the problems associated with changing the base-class

If later in the future, vehicles need an engine number which requires more sophistication (not an integer but a more complex type), but you still want the sub-classes to retain the integer type for their respective engine numbers then you’re kind of in a tight spot. You’ve already defined the super class. But you want the sub-class to be different. You want to violate the Liskov-substitution principle. With inheritance you can’t do this. With a composition based approach you can – because a Wagon/Truck/LuxuryCar/CompactCar is not a Vehicle. This means that the "abstract base class" can change it’s interface, but you don’t have to make subsequent changes where the "derived classes" are called – but you will have to make changes to the relevant "implementations" of the "derived classes". In other words, you'll have less code to change than if you opted for the inheritance approach. I hope this is making sense.

You could use the composition approach with a common interface between all types. Then in the future you could resort to an abstract parent class when you have more complete information. So do you wait, or do you bite the bullet and pour the concrete foundations of an abstract base class now, making it harder to change later?

Summary

  • If you go for the composition approach you’ll be repeating yourself may times over because you cannot simply automatically delegate/fwd to the abstract parent class.

  • But then again you’ll have freedom because you’re not locked into the “parent class” interface and any subsequent changes to the “parent class” need not affect the interface of the “sub-classes” (though it will affect their implementations).

  • There are pros and cons to both approaches – just be aware of them.

\$\endgroup\$
2
\$\begingroup\$
  • Learn Test Driven Development and apply it using JUnit or TestNG. Always write the test first, you will need a test case to prove each of your bullet points, each with multiple scenarios.
  • Do not use getters they are an anti-pattern instead apply the Tell Don't Ask idiom, e.g. car.display(); instead of x = car.getX() & system.out.println(x)
  • Replace setters with methods that express their intent based on the the language of the problem not programming programming language. c.f. Fluent Interface
  • Replace System.out with SLF4J or Log4j for logging.

Response to comment.

Test driven development is critically important to new programmers. It provides the continuous feedback they need to make good progress without being a drain on their coach. It instils good coding discipline from day one that doesn't need to be retrained to doing things correctly later on. It also guides the learner to producing decoupled code by default. TDD is essential to learning best practice in programming from day one. It is one of the first things I typically need to retrain with new graduate and young developers.

\$\endgroup\$
  • 2
    \$\begingroup\$ I think TDD and a logging framework would be too much of a good thing for this introducing assignment. Nonetheless, these are essential techniques for advanced programming. \$\endgroup\$ – Oliver530 Sep 4 '17 at 19:54

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.