-
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
first solution of the task #917
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's great to see that you've followed the checklist guidelines for most of the classes! However, we need to address the access modifier for the 'weight' field in the Excavator
class. Making sure that fields are properly encapsulated is crucial for maintaining the integrity of our objects. Let's make that field private and provide appropriate getters and setters. Once that's done, you'll be all set! Keep up the good work! 😊👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -5,9 +5,16 @@ | |||
* Do not remove no-args constructor | |||
*/ | |||
public class Bulldozer extends 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.
Don't begin class or method implementation with empty lines. Please remove the unnecessary empty lines before the class declaration.
@@ -5,9 +5,16 @@ | |||
* Do not remove no-args constructor | |||
*/ | |||
public class Bulldozer extends Machine { | |||
private int weight; |
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 the Bulldozer
class. Consider using private access modifiers for fields and providing public getters and setters to access these fields.
@@ -5,9 +5,16 @@ | |||
* Do not remove no-args constructor | |||
*/ | |||
public class Excavator extends Machine { | |||
private int weight; |
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
class. The field 'weight' should be protected or private to prevent direct access to the field. Consider using private and providing a getter and setter for it.
import java.util.List; | ||
|
||
public interface MachineProducer { | ||
public interface MachineProducer<T extends 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.
Don't begin class or method implementation with an empty line. Please remove the unnecessary empty lines at the beginning of the interface definition.
import java.util.List; | ||
|
||
/** | ||
* Parameterize this service and add its implementation in a separate class. | ||
*/ | ||
public interface MachineService { | ||
public interface MachineService<T extends 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.
Don't begin class or method implementation with empty line. There are multiple empty lines before the first method declaration which should be removed.
@@ -26,13 +27,13 @@ public interface MachineService { | |||
* @param machines - list of machines to be filled with value | |||
* @param value - any object of machine sub class | |||
*/ | |||
void fill(List<Object> machines, Object value); | |||
void fill(List<? super T> machines, T value); |
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 size of a list passed to fill()
method should remain the same. Ensure that the implementation of fill()
replaces the old values with the new ones without changing the list size.
/** | ||
* Your implementation of MachineService. | ||
*/ | ||
public class MachineServiceImpl { | ||
public class MachineServiceImpl implements MachineService<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.
Don't begin class or method implementation with empty line.
public List<Machine> getAll(Class<? extends Machine> type) { | ||
List<? extends Machine> machineList; | ||
switch (type.getSimpleName()) { | ||
case "Bulldozer": | ||
machineList = new BulldozerProducer().get(); | ||
break; | ||
|
||
case "Excavator": | ||
machineList = new ExcavatorProducer().get(); | ||
break; | ||
|
||
case "Truck": | ||
machineList = new TruckProducer().get(); | ||
break; | ||
|
||
default: | ||
machineList = new ArrayList<>(); | ||
} | ||
return new ArrayList<>(machineList); | ||
} |
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 to store the result of 'someProducer.get()' and then return a new list created from this variable.
default: | ||
machineList = new ArrayList<>(); |
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 return null or specific type List in getAll()
when no producer has been found. It's better to return an empty List to avoid potential NullPointerExceptions or unexpected behavior.
public void fill(List<? super Machine> machines, Machine value) { | ||
for (int i = 0; i < machines.size(); i++) { | ||
machines.set(i, value); | ||
} |
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 size of a list passed to fill()
method should remain the same. Just replace the old values with the new ones, which is correctly implemented here.
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.
While the overall implementation seems to meet the checklist requirements, there is a small but important issue regarding encapsulation best practices that needs to be addressed. The weight
field in the Truck
and Bulldozer
classes should be private, and accessed through getters and setters. This is crucial for maintaining the integrity of the objects' state and ensuring that future changes to the class internals won't affect other parts of the codebase. Keep up the good work, and with this small change, your code will be even better! 😊👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -4,10 +4,18 @@ | |||
* Add some custom fields that could be only in Bulldozer | |||
* Do not remove no-args constructor | |||
*/ | |||
|
|||
public class Bulldozer extends 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.
Don't begin class or method implementation with an empty line. Please remove the unnecessary empty lines (3-7).
public class Bulldozer extends Machine { | ||
private int weight; |
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 Bulldozer
class. The field weight
should be private and accessed through getters and setters to encapsulate the data.
@@ -5,9 +5,16 @@ | |||
* Do not remove no-args constructor | |||
*/ | |||
public class Excavator extends Machine { | |||
private int weight; |
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 field 'weight' should be accessed through getters and setters. It is a good practice to keep fields private and provide public methods to access them. This encapsulates the internal representation and allows for validation and control over how the fields are accessed or modified.
@@ -5,9 +5,16 @@ | |||
* Do not remove no-field constructor | |||
*/ | |||
public class Truck extends 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.
Don't begin class or method implementation with empty lines. Please remove the unnecessary empty lines before the class declaration.
@@ -5,9 +5,16 @@ | |||
* Do not remove no-field constructor | |||
*/ | |||
public class Truck extends Machine { | |||
private int weight; |
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 Truck
class. You should use getters and setters to access the weight
field instead of exposing it directly.
import java.util.List; | ||
|
||
/** | ||
* Parameterize this service and add its implementation in a separate class. | ||
*/ | ||
public interface MachineService { | ||
|
||
public interface MachineService<T extends 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.
Don't begin class or method implementation with an empty line.
/** | ||
* Your implementation of MachineService. | ||
*/ | ||
public class MachineServiceImpl { | ||
public class MachineServiceImpl implements MachineService<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.
Don't begin class or method implementation with empty lines. There should not be empty lines (7-9) before the class declaration.
default: | ||
machineList = new ArrayList<>(); | ||
} | ||
return new ArrayList<>(machineList); |
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. The list machineList
is already local, but you can directly return the result of the get()
method inside the switch-case blocks to avoid unnecessary variable assignment.
for (int i = 0; i < machines.size(); i++) { | ||
machines.set(i, value); | ||
} | ||
} |
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 size of a list passed to fill()
method should remain the same. Just replace the old values with the new ones, which you have done correctly. Good job!
for (Machine machine : machines) { | ||
machine.doWork(); | ||
} | ||
} |
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.
Keep if-else constructions simple. There are no if-else constructions in the startWorking
method, so this is implemented correctly.
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 work on your Pull Request! 🌟 While there are some minor issues like the need for private access modifiers and avoiding unnecessary empty lines, these are not critical and can be addressed in future updates. Keep up the good work and make sure to review the comments to further improve your code. Your progress is commendable, and it's important to learn from these suggestions to enhance your coding practices. Keep coding and learning! 💻📚
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
public class Bulldozer extends Machine { | ||
private int weight; |
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 Bulldozer
class. It's recommended to use private fields and provide public getter and setter methods for accessing them.
@@ -5,9 +5,24 @@ | |||
* Do not remove no-args constructor | |||
*/ | |||
public class Excavator extends 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.
Don't begin class or method implementation with empty lines.
@@ -5,9 +5,24 @@ | |||
* Do not remove no-args constructor | |||
*/ | |||
public class Excavator extends Machine { | |||
private int weight; |
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. It's good practice to make fields private and provide public getters and setters for accessing them.
*/ | ||
public interface MachineService { | ||
|
||
public interface MachineService<T extends 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.
Don't begin class or method implementation with empty line.
public List<Machine> getAll(Class<? extends Machine> type) { | ||
List<? extends Machine> machineList; | ||
switch (type.getSimpleName()) { | ||
case "Bulldozer": | ||
machineList = new BulldozerProducer().get(); | ||
break; | ||
|
||
case "Excavator": | ||
machineList = new ExcavatorProducer().get(); | ||
break; | ||
|
||
case "Truck": | ||
machineList = new TruckProducer().get(); | ||
break; | ||
|
||
default: | ||
machineList = new ArrayList<>(); | ||
} | ||
return new ArrayList<>(machineList); |
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 for the machineList
at the beginning of the getAll
method. This will make your code cleaner as you will not need to declare the variable type each time in the switch case blocks.
switch (type.getSimpleName()) { | ||
case "Bulldozer": | ||
machineList = new BulldozerProducer().get(); | ||
break; | ||
|
||
case "Excavator": | ||
machineList = new ExcavatorProducer().get(); | ||
break; | ||
|
||
case "Truck": | ||
machineList = new TruckProducer().get(); | ||
break; | ||
|
||
default: | ||
machineList = new ArrayList<>(); | ||
} |
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 switch statement could be simplified by removing the break
statements and directly returning the result of the get
method inside each case. This would also make the local variable machineList
unnecessary.
} | ||
|
||
@Override | ||
public void fill(List<? super Machine> machines, Machine value) { |
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 fill
method correctly replaces the old values with the new ones without changing the size of the list, adhering to the checklist.
} | ||
|
||
@Override | ||
public void startWorking(List<? extends Machine> machines) { |
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 startWorking
method is correctly implemented, it calls doWork
on each machine in the list.
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
public class TruckProducer implements MachineProducer<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.
Don't begin class or method implementation with an empty line.
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
public class TruckProducer implements MachineProducer<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.
The MachineProducer
interface should be parameterized to prevent implementations that do not use Machine
or its subtype. This is correctly done here with MachineProducer<Truck>
, ensuring that only Truck
or its subtypes can be used.
No description provided.