-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
implemented and parameterisedÑ methods #925
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems there are a few critical issues that need to be addressed before we can approve the Pull Request. Please ensure that the TruckProducer
is correctly parameterized with Truck
, and adjust the return type of the get()
method accordingly. Also, make sure to use access modifiers for your fields in Excavator
and consider using an else-if structure for better code readability. Keep up the good work, and don't hesitate to reach out if you need any help! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
private String bucket; | ||
private int wheels; | ||
private String platform; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget about access modifiers for your custom fields in Excavator. You should never want to expose the object fields directly. They should be accessed through special methods (getters and setters).
import java.util.List; | ||
|
||
public class BulldozerProducer implements MachineProducer<Bulldozer> { | ||
// можна підставити Machine і все буде працювати |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment here suggests that using Machine
would work, but according to the checklist, you should parameterize MachineProducer
in such a way that any implementation not using Machine
or its subtype could not be created. Ensure that the implementation strictly uses Bulldozer
as intended.
if (type == Bulldozer.class) { | ||
BulldozerProducer bulldozer = new BulldozerProducer(); | ||
finalList.addAll(bulldozer.get()); | ||
} | ||
if (type == Excavator.class) { | ||
ExcavatorProducer excavator = new ExcavatorProducer(); | ||
finalList.addAll(excavator.get()); | ||
} | ||
if (type == Truck.class) { | ||
TruckProducer truck = new TruckProducer(); | ||
finalList.addAll(truck.get()); | ||
} | ||
|
||
return finalList; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using an else-if structure instead of multiple if statements for checking the type. This will make the code more efficient and easier to read. According to the checklist: 'Keep if else constructions simple'.
import core.mate.academy.model.Truck; | ||
import java.util.List; | ||
|
||
public class TruckProducer implements MachineProducer<Machine> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TruckProducer
should be parameterized with Truck
instead of Machine
to ensure that it cannot be used with any type other than Truck
or its subtypes. This aligns with the checklist item: 'Parameterize MachineProducer
in such a way that any implementation NOT using Machine
or its subtype could NOT be created.'
|
||
public class TruckProducer implements MachineProducer<Machine> { | ||
@Override | ||
public List<Machine> get() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return type of the get()
method should be List<Truck>
instead of List<Machine>
to match the parameterization of TruckProducer
with Truck
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on implementing the classes and interfaces correctly! 🎉 Your code meets all the core requirements, and the suggestion about using a local variable is a minor improvement for future readability. Keep up the good work!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider creating a local variable in your implementation of getAll(Class type)
method as suggested in the checklist. This can improve readability and maintainability of your code.
} | ||
|
||
return finalList; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checklist advises against returning null or a specific type List when no producer has been found. Instead, it's better to return an empty List. Your current implementation already returns an empty List when no type matches, which is correct.
No description provided.