Skip to main content
Continued review - added `CarsArray`
Source Link
AJNeufeld
  • 34k
  • 5
  • 39
  • 101

Car

Your Car class has what I would consider deficiencies.

The make, model, year_of_production and carId shouldn’t change after creating a car. They should be final. Attributes like the price (and colour and condition) of the car can change.

Inconsistent usage of this.. The function getCarID() doesn’t need it.

Bug: The method setYear_of_production doesn’t change the yearsOld member!!!

The constructor uses a hard-coded 2018.

The yearsOld member should not be a member at all, but a getter method which relies on the year of production and the current year. If after putting cars on the list, a year goes by, the cars current don’t (and can’t) age! If you haven’t learnt how to query the current year, at least use a parameter or a global static (volatile, not constant) value.

Your Car class is a data model class. It shouldn’t have “output” functions. The class could be used by a console application or a GUI application. “Debug” output is fine, but don’t print output if you can avoid it. And you can. Write a Car.toString() function, and the caller could System.out.println(car). Or better, add a printCarDetails() in CarPartsMenu. For the same reasons, I’d move printCars() out of CarsArray (a data model class) into CarPartsMenu.


CarsArray

The carsArray should be final.

.getCars(), .size(), and .addCar() all unnecessarily use this..

IndexOutOfBoundsException is not the right exception to use. NoSuchElementException is a better semantic match.


Your Car class has what I would consider deficiencies.

The make, model, year_of_production and carId shouldn’t change after creating a car. They should be final. Attributes like the price (and colour and condition) of the car can change.

Inconsistent usage of this.. The function getCarID() doesn’t need it.

Bug: The method setYear_of_production doesn’t change the yearsOld member!!!

The constructor uses a hard-coded 2018.

The yearsOld member should not be a member at all, but a getter method which relies on the year of production and the current year. If after putting cars on the list, a year goes by, the cars current don’t (and can’t) age! If you haven’t learnt how to query the current year, at least use a parameter or a global static (volatile, not constant) value.

Your Car class is a data model class. It shouldn’t have “output” functions. The class could be used by a console application or a GUI application. “Debug” output is fine, but don’t print output if you can avoid it. And you can. Write a Car.toString() function, and the caller could System.out.println(car). Or better, add a printCarDetails() in CarPartsMenu. For the same reasons, I’d move printCars() out of CarsArray (a data model class) into CarPartsMenu.

Car

Your Car class has what I would consider deficiencies.

The make, model, year_of_production and carId shouldn’t change after creating a car. They should be final. Attributes like the price (and colour and condition) of the car can change.

Inconsistent usage of this.. The function getCarID() doesn’t need it.

Bug: The method setYear_of_production doesn’t change the yearsOld member!!!

The constructor uses a hard-coded 2018.

The yearsOld member should not be a member at all, but a getter method which relies on the year of production and the current year. If after putting cars on the list, a year goes by, the cars current don’t (and can’t) age! If you haven’t learnt how to query the current year, at least use a parameter or a global static (volatile, not constant) value.

Your Car class is a data model class. It shouldn’t have “output” functions. The class could be used by a console application or a GUI application. “Debug” output is fine, but don’t print output if you can avoid it. And you can. Write a Car.toString() function, and the caller could System.out.println(car). Or better, add a printCarDetails() in CarPartsMenu. For the same reasons, I’d move printCars() out of CarsArray (a data model class) into CarPartsMenu.


CarsArray

The carsArray should be final.

.getCars(), .size(), and .addCar() all unnecessarily use this..

IndexOutOfBoundsException is not the right exception to use. NoSuchElementException is a better semantic match.


Don’t print in model classes.
Source Link
AJNeufeld
  • 34k
  • 5
  • 39
  • 101

Your Car class has what I would consider deficiencies.

The make, model, year_of_production and carId shouldn’t change after creating a car. They should be final. Attributes like the price (and colour and condition) of the car can change.

Inconsistent usage of this.. The function getCarID() doesn’t need it.

Bug: The method setYear_of_production doesn’t change the yearsOld member!!!

The constructor uses a hard-coded 2018.

The yearsOld member should not be a member at all, but a getter method which relies on the year of production and the current year. If after putting cars on the list, a year goes by, the cars current don’t (and can’t) age! If you haven’t learnt how to query the current year, at least use a parameter or a global static (volatile, not constant) value.

Your Car class is a data model class. It shouldn’t have “output” functions. The class could be used by a console application or a GUI application. “Debug” output is fine, but don’t print output if you can avoid it. And you can. Write a Car.toString() function, and the caller could System.out.println(car). Or better, add a printCarDetails() in CarPartsMenu. For the same reasons, I’d move printCars() out of CarsArray (a data model class) into CarPartsMenu.

Your Car class has what I would consider deficiencies.

The make, model, year_of_production and carId shouldn’t change after creating a car. They should be final. Attributes like the price (and colour and condition) of the car can change.

Inconsistent usage of this.. The function getCarID() doesn’t need it.

Bug: The method setYear_of_production doesn’t change the yearsOld member!!!

The constructor uses a hard-coded 2018.

The yearsOld member should not be a member at all, but a getter method which relies on the year of production and the current year. If after putting cars on the list, a year goes by, the cars current don’t (and can’t) age! If you haven’t learnt how to query the current year, at least use a parameter or a global static (volatile, not constant) value.

Your Car class has what I would consider deficiencies.

The make, model, year_of_production and carId shouldn’t change after creating a car. They should be final. Attributes like the price (and colour and condition) of the car can change.

Inconsistent usage of this.. The function getCarID() doesn’t need it.

Bug: The method setYear_of_production doesn’t change the yearsOld member!!!

The constructor uses a hard-coded 2018.

The yearsOld member should not be a member at all, but a getter method which relies on the year of production and the current year. If after putting cars on the list, a year goes by, the cars current don’t (and can’t) age! If you haven’t learnt how to query the current year, at least use a parameter or a global static (volatile, not constant) value.

Your Car class is a data model class. It shouldn’t have “output” functions. The class could be used by a console application or a GUI application. “Debug” output is fine, but don’t print output if you can avoid it. And you can. Write a Car.toString() function, and the caller could System.out.println(car). Or better, add a printCarDetails() in CarPartsMenu. For the same reasons, I’d move printCars() out of CarsArray (a data model class) into CarPartsMenu.

Source Link
AJNeufeld
  • 34k
  • 5
  • 39
  • 101

Your Car class has what I would consider deficiencies.

The make, model, year_of_production and carId shouldn’t change after creating a car. They should be final. Attributes like the price (and colour and condition) of the car can change.

Inconsistent usage of this.. The function getCarID() doesn’t need it.

Bug: The method setYear_of_production doesn’t change the yearsOld member!!!

The constructor uses a hard-coded 2018.

The yearsOld member should not be a member at all, but a getter method which relies on the year of production and the current year. If after putting cars on the list, a year goes by, the cars current don’t (and can’t) age! If you haven’t learnt how to query the current year, at least use a parameter or a global static (volatile, not constant) value.