Pull_request_1 #918
Pull_request_1 #918
Conversation
… subclasses and methods for managing machines.
There was a problem hiding this comment.
Great job on implementing the classes and producers according to the checklist! 🌟 Just a few minor suggestions for future improvements: remember to use private access modifiers for class fields to ensure encapsulation, and avoid unnecessary empty lines at the beginning of class or method implementations to keep your code clean and professional. Keep up the good work! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| @@ -5,11 +5,39 @@ | |||
| * Do not remove no-args constructor | |||
| */ | |||
| public class Bulldozer extends Machine { | |||
There was a problem hiding this comment.
Don't begin class or method implementation with an empty line. Please remove the unnecessary empty lines (lines 3-6) to comply with the checklist.
| @@ -5,11 +5,39 @@ | |||
| * Do not remove no-args constructor | |||
| */ | |||
| public class Bulldozer extends Machine { | |||
| private int bladeWidth; | |||
There was a problem hiding this comment.
Don't forget about access modifiers for your custom fields in Bulldozer class. It's a good practice to encapsulate fields using private access modifier and provide public getters and setters for accessing them.
| @@ -5,11 +5,39 @@ | |||
| * Do not remove no-args constructor | |||
| */ | |||
| public class Bulldozer extends Machine { | |||
| private int bladeWidth; | |||
| private int operatingWeight; | |||
There was a problem hiding this comment.
Don't forget about access modifiers for your custom fields in Bulldozer class. It's a good practice to encapsulate fields using private access modifier and provide public getters and setters for accessing them.
| @@ -5,11 +5,39 @@ | |||
| * Do not remove no-args constructor | |||
| */ | |||
| public class Bulldozer extends Machine { | |||
| private int bladeWidth; | |||
| private int operatingWeight; | |||
| private String engineType; | |||
There was a problem hiding this comment.
Don't forget about access modifiers for your custom fields in Bulldozer class. It's a good practice to encapsulate fields using private access modifier and provide public getters and setters for accessing them.
| @@ -5,11 +5,39 @@ | |||
| * Do not remove no-field constructor | |||
| */ | |||
| public class Truck extends Machine { | |||
There was a problem hiding this comment.
Don't forget about access modifiers for your custom fields in Truck class. It's a good practice to make fields private and provide public getters and setters to access them, which you've done, but the fields themselves should also be declared as private to prevent direct access.
| @@ -5,7 +5,7 @@ | |||
| /** | |||
| * Parameterize this service and add its implementation in a separate class. | |||
| */ | |||
| public interface MachineService { | |||
| public interface MachineService<T> { | |||
There was a problem hiding this comment.
Don't begin interface or method implementation with an empty line.
| public class MachineServiceImpl implements MachineService<Machine> { | ||
| @Override | ||
| public List<Machine> getAll(Class<? extends Machine> type) { | ||
| List<Machine> machineList = new ArrayList<>(); | ||
|
|
||
| if (type == Bulldozer.class) { | ||
| BulldozerProducer bulldozerProducer = new BulldozerProducer(); | ||
| machineList.addAll(bulldozerProducer.get()); | ||
| } | ||
|
|
||
| if (type == Excavator.class) { | ||
| ExcavatorProducer excavatorProducer = new ExcavatorProducer(); | ||
| machineList.addAll(excavatorProducer.get()); | ||
| } | ||
|
|
||
| if (type == Truck.class) { | ||
| TruckProducer truckProducer = new TruckProducer(); | ||
| machineList.addAll(truckProducer.get()); | ||
| } | ||
| return machineList; | ||
| } | ||
|
|
||
| @Override | ||
| public void fill(List<? super Machine> machines, Machine value) { | ||
| Collections.fill(machines, value); | ||
| } | ||
|
|
||
| @Override | ||
| public void startWorking(List<? extends Machine> machines) { | ||
| machines.forEach(Machine::doWork); | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
Don't begin class or method implementation with empty lines (lines 11-13). It would be better to remove these to comply with the checklist.
| public class MachineServiceImpl { | ||
| public class MachineServiceImpl implements MachineService<Machine> { | ||
| @Override | ||
| public List<Machine> getAll(Class<? extends Machine> type) { |
There was a problem hiding this comment.
Consider creating a local variable in your implementation of getAll(Class type) method. This can help to improve the readability of the method and ensure that the returned list is a new list rather than a direct reference to the list obtained from the producer.
| TruckProducer truckProducer = new TruckProducer(); | ||
| machineList.addAll(truckProducer.get()); | ||
| } | ||
| return machineList; |
There was a problem hiding this comment.
Keep if else constructions simple. Here, since each 'if' block contains a return statement, the subsequent 'if' statements can be changed to 'else if' to prevent unnecessary checks once a match is found.
| @Override | ||
| public void fill(List<? super Machine> machines, Machine value) { | ||
| Collections.fill(machines, value); | ||
| } |
There was a problem hiding this comment.
The size of a list passed to fill() method should remain the same. Just replace the old values with the new ones. The use of Collections.fill replaces all elements in the list with the specified value, which might not be the intended behavior if the list contains different types of Machines.
Implement MachineProducer and MachineService interfaces with specific subclasses and methods for managing machines.