Code review my code?

IamtheWalrus

package cars;

import java.sql.Date;
import java.sql.SQLException;


/*
 * Main hire service class
 */
public class HireService {
	private DbService db;
	
	//called by rest servlet to make booking
	public long hire(String name, String licenseNumber, String cd, String reg, String start, int days, double rate) throws SQLException {
		boolean hired = false;
		Client client = getClient(name, licenseNumber, cd);
		Car car = getCarDetails(reg, cd);
		if(car == null || car.hired) {
			return -1;
		}
		
		long hireNumber = System.nanoTime(); //use as unique id
		HireRecord hire = new HireRecord();
		client.addHirerecrd(hire);
		hire.setCar(car); hire.setDays(days);
		hire.setClient(client.getName());		
		hire.setStartDate(Date.valueOf(start));
		hire.setRate(rate); hire.setState(1);
		hire.setHireno(hireNumber);
		hire.setDays(days);		
		db.saveToDatabase(client, cd);
		car.hire(db, cd, hire);
		return hireNumber;
	}
	
	public void markReturned(String cd, long hireno)  {
		try {
			Car car = (Car) db.loadFromDb(cd, "select * from crs where hrrnm = " + hireno, Car.class);
			car.release(db, cd);
		} catch (SQLException e) {
			//nothing we can do
		}
	}
	
	public Car getCarDetails(String rg, String cd)  {
		try {
			return (Car) db.loadFromDb(cd, "select * from crs where rg = " + rg,Car.class);
		} catch (SQLException e) {
			return new Car(rg);
		}
	}
	
	//dont use
	private DbService getDb() {
		return db;
	}

	public void setDb(DbService db) {
		this.db = db;
	}

	public Client getClient(String name, String licenseNumber, String cd)  {
		Client client;
		try {
			client = (Client) db.loadFromDb(cd, "select * from clients where clientId = " + name,Client.class);
		} catch (Exception e1) {
			throw new RuntimeException();
		}
		
		if(client == null) {
			client = new Client(name,licenseNumber,cd);
			try {
				db.saveToDatabase(client, cd);
			} catch (Exception e) {
				e.printStackTrace();
			}
		}
		
		return client;
	}
}

package cars;

import java.util.Date;

public class HireRecord {
	private Car car;
	private String client;
	private Date startDate;
	private int days;
	private double rate;
	private int state;
	private long hireno;

	public Car getCar() {
		return car;
	}

	public void setCar(Car car) {
		this.car = car;
	}

	public String getClient() {
		return client;
	}

	public void setClient(String client) {
		this.client = client;
	}

	public Date getStartDate() {
		return startDate;
	}

	public void setStartDate(Date startDate) {
		this.startDate = startDate;
	}

	public int getDays() {
		return days;
	}

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

	public int getState() {
		return state;
	}

	public void setState(int state) {
		this.state = state;
	}

	public long getHireno() {
		return hireno;
	}

	public void setHireno(long hireno) {
		this.hireno = hireno;
	}

	public double getRate() {
		if(car instanceof Bike) return rate;  //no discount for bikes
		return this.rate = car.getAge() > 3 ? rate * 0.9 : rate;  //discount for older cars
	}

	public void setRate(double rate) {
		this.rate = rate;
	}
	
	
}

package cars;

import java.util.Vector;

public class Client {
	private String name;
	String licenseNumber;
	private String cd;
	private Vector records;
	
	public Client(String name, String licenseNumber, String cd) {
		this.name = name;
		this.licenseNumber = licenseNumber;
		this.cd = cd;
	}

	public String getName() {
		return name;
	}

	public String getLicenseNumber() {
		return licenseNumber;
	}

	public String getCd() {
		return cd;
	}

	public Vector getRecords() {
		return records;
	}

	public void addHirerecrd(HireRecord r) {
		if(records == null) {
			records = new Vector();
		}
		records.add(r);
	}

	@Override
	public String toString() {
		return name + licenseNumber;
	}
	
}

package cars;

import java.sql.SQLException;
import java.util.Calendar;
import java.util.Date;

import org.joda.time.DateTime;

public class Car {
	public String make;
	public String reg;
	public int category;
	public boolean hired;
	public Date hireEnd;
	private int age;
	private long hireNumber;

	public Car(String reg) {
		this.reg = reg;
	}

	public Car(String reg, String make, int category) {
		this.make = make;
		this.reg = reg;
		this.category = category;
	}

	public int getAge() {
		return 2015 - Integer.parseInt("20" +reg.substring(3, 2));
	}
	
	public void hire(DbService dbService, String cd, HireRecord record) throws SQLException {
		hireEnd = new DateTime(record.getStartDate().getTime()).plusDays(record.getDays()).toDate();
		hireNumber = record.getHireno();
		Calendar.getInstance();
		dbService.saveToDatabase(this, cd);
	}
	
	public void release(DbService dbService, String cd) throws SQLException {
		hireNumber = 0;
		hireEnd = null;
		dbService.saveToDatabase(this, cd);
	}
	
	
		//disabled
		public Car(String make, String reg, int category, boolean hired,Date hireEnd) {
//			this.make = make;
//			this.reg = reg;
//			this.category = category;
//			this.hired = hired;
//			this.hireEnd = hireEnd;
			throw new RuntimeException("disabled full constructor");
		}
}

package cars;

/*
 * Andyr request want to hire bikes too 
 */
public class Bike extends Car {
	public int cc;

	public Bike(String reg) {
		super(reg);
	}
	
}

Ziycon

Would it not be better to have the parent class as vehicle and have bike and car extend it, bike extending car is a slight bit illogical.

Merrion

Other comments:-
1) Don't have multiple logical lines on one physical line - e.g. split up hire.setCar(car); hire.setDays(days);
2) Use full variable names - cd might mean "code" or "certification diplomatique"?
3) There doesn't seem to be any exception handling in most code paths
4) When you do swallow an exception you prevent anyone knowing about and fixing it ... I don't think an SQL Exception should result in the creation of an empty car record.

Creamy Goodness

What others have said but also review your comments

//don't use

Remove the comment and remove the code so someone can't use it!

Commented out code is a major pet peeve of mine.

Talisman

public int getAge() {
		return 2015 - Integer.parseInt("20" +reg.substring(3, 2));
	}

You have hard coded the year 2015 and determining the year from the reg is a hack. It's possible that reg.substring(3, 2) could return an IndexOutOfBoundsException especially in the case where the vehicle registration format is different.

A cleaner solution might be to record the year of the vehicle e.g. 2010 and use the java.time.Year class to determine the current year. Then getAge() would become a simple function.

PrzemoF

I don't know java, but:

1. Potential problem if clock goes back: long hireNumber = System.nanoTime(); //use as unique id
2. Mixed comment styles (in line, separate, you name it)
3. Inconsistent variable names (camel style, mashup). Capital letters all over the place with no good reason. getHireno is probably the worst one.
4. getRate() uses some hard coded values. Use clear definitions instead. Same for getAge and others
5. return -1 means "something died and smells"? Software dying without a proper, detailed message is _wrong_

chrislad

I'd not use public variables, unless it's a static class or something.

if(car == null || car.hired) should really be something like if (car == null || car.getHired()) where getHired returns the value of the boolean hired.

Aswerty

long hireNumber = System.nanoTime(); //use as unique id

Why not use a UUID value, I know the chances of your long being non-unique is astronomically small but why not use the correct tool for the job.

hireEnd = new DateTime(record.getStartDate().getTime()).plusDays(record.getDays()).toDate();

The goggles! They do nothing!

Try avoid doing so much in one line: 5 method calls while creating a new instance is a bit OTT.

Also as others have said, your exception handling shows you have no idea how to do exception handling. It's not a criticism, just an observation. One of the things to understand about exception handling is that often letting an exception bubble (e.g. rise through the call stack to a function further up the stack) can be the best solution instead of trying to eat it as soon as it happens. The top most function might inform the user their action could not be completed and log the error. Or it might be suitable to just let the process crash, though unless a system error such as a StackoverflowExcetion occurs, that might not be a great idea.

If I had an SQLException I'd typically let it bubble and let my Web Framework handle it by having it send a HTTP Status 500 response to the user with a custom error page/message. I'd also typically have a logging solution to log all unhandled errors in my code. Of course if your not using either of those - you have to make do with something homemade to do similar.

Some rules of exception handling:

• Don't do nothing.
• Don't use exceptions for validation.
• Don't catch an exception you don't know how to handle (unless you're just logging and re-throwing).

               try {
			client = (Client) db.loadFromDb(cd, "select * from clients where clientId = " + name,Client.class);
		} catch (Exception e1) {
			throw new RuntimeException();
		}

Just an example of the bad exception handling. You're converting every checked Exception into an unchecked RuntimeException here. This doesn't make any sense.

amen

Code:
long hireNumber = System.nanoTime(); //use as unique id
Why not use a UUID value, I know the chances of your long being non-unique is astronomically small but why not use the correct tool for the job.

The above. Use a UUID guaranteed uniqueness. Using time to generate uniqueness in a cluster environment is not a good idea.(yes small chance of collisions)

try {
			client = (Client) db.loadFromDb(cd, "select * from clients where clientId = " + name,Client.class);
		} catch (Exception e1) {
			throw new RuntimeException();
		}

Stop using select *. Only select the value you actually need.

If you dont want to use some form of prepared sql statements then move the sql to a file and load the sql from the file as required. This gives you the ability to change the sql without having to recompile the code.

public double getRate() {
		if(car instanceof Bike) return rate;  //no discount for bikes
		return this.rate = car.getAge() > 3 ? rate * 0.9 : rate;  //discount for older cars
	}

The use of 3 and 0.9 are Magic Number . You should have a CONST such as RATEYear for 3 and AgeDiscount for 0.9

What happens if you rate a 2104 vehicle on 31/12/2016, sell a policy and then rate it on again 01/01/2017 ? (You incorrectly get a mid-term discount. You now have a very unhappy insurance company)

LeinsterDub

Not sure what others think and I know in college the following was considered best practice

public long getHireno() {
		return hireno;
	}

public void setHireno(long hireno) {
		this.hireno = hireno;
	} 

But for me in the real world it's a lot of added code when you could simply make hireno public .

ishotjr2

I would be checking the variables.

What characters can name, licensenumber especially if it is going into a select (SQL injection), I imagine there is a sanitizer class before this.

But I am surprised at the lack of boundary, content and format checking.

Giblet

LeinsterDub wrote: »

Not sure what others think and I know in college the following was considered best practice

public long getHireno() {
		return hireno;
	}

public void setHireno(long hireno) {
		this.hireno = hireno;
	} 

But for me in the real world it's a lot of added code when you could simply make hireno public .

It's harder to refactor your usages out if you needed some additional checks around setting hireno.

LeinsterDub

Giblet wrote: »

It's harder to refactor your usages out if you needed some additional checks around setting hireno.

Is it really? Just change the get; set;

Aswerty

LeinsterDub wrote: »

Is it really? Just change the get; set;

Maybe I don't know my Java well enough but is get; set; not C# only (i.e. no Properties in Java).

Apologies if I've misinterpreted you!

Buford T Justice

LeinsterDub wrote: »

Not sure what others think and I know in college the following was considered best practice

public long getHireno() {
		return hireno;
	}

public void setHireno(long hireno) {
		this.hireno = hireno;
	} 

But for me in the real world it's a lot of added code when you could simply make hireno public .

Or use the IDE to auto-generate your getters and setters.

Giblet

Aswerty wrote: »

Maybe I don't know my Java well enough but is get; set; not C# only (i.e. no Properties in Java).

Apologies if I've misinterpreted you!

Well in C# it creates the methods for you secretly

This is more about not setting fields to be public, not Properties.

You should always encapsulate fields, such as lists, with ReadOnlyCollections or Iterators / IEnumerables.

Aswerty

Giblet wrote: »

Well in C# it creates the methods for you secretly

This is more about not setting fields to be public, not Properties.

My point was I don't think the syntax of "get; set;" exists in Java. So it sounded like that poster was angling toward a more C# related syntax - in so implying the use of Properties. That was why I said apologies if I'd misinterpreted him, and now you've gone and misinterpreted me!

In any case I should probably have just asked LeinsterDub to elaborate on what he meant rather than inferring things.

LeinsterDub

Aswerty wrote: »

My point was I don't think the syntax of "get; set;" exists in Java. So it sounded like that poster was angling toward a more C# related syntax - in so implying the use of Properties. That was why I said apologies if I'd misinterpreted him, and now you've gone and misinterpreted me!

In any case I should probably have just asked LeinsterDub to elaborate on what he meant rather than inferring things.

No sorry you're correct it's been a while since if done java was looking at it from what I thought was a general code perspective turns out it's a c# perspective

Aswerty

LeinsterDub wrote: »

No sorry you're correct it's been a while since if done java was looking at it from what I thought was a general code perspective turns out it's a c# perspective

No properties is one of the many reasons why I'd find it hard to move back to Java from C#. But yes, if Java had them the getter and setter methods would be overkill like you stated.