Skip to content
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 pecs #919

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/main/java/core/mate/academy/model/Bulldozer.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,13 @@
*/
public class Bulldozer extends Machine {

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 (lines 3-6). The class declaration should follow directly after the package statement.

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 empty line before the class declaration.

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 good practice to use private access modifiers and provide getters and setters for accessing these fields.

public Bulldozer() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't remove the no-args constructor, but also make sure it doesn't contain unnecessary logic as currently seen in line 9.

setName(getName());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The no-args constructor is setting the name of the Bulldozer to its current name which is not initialized yet. This logic doesn't seem to serve any purpose and could potentially be removed unless there's a specific reason for this implementation.

}


@Override
public void doWork() {
System.out.println("Bulldozer started to work");
}

}
12 changes: 12 additions & 0 deletions src/main/java/core/mate/academy/model/BulldozerProducer.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package core.mate.academy.model;

import core.mate.academy.service.MachineProducer;
import java.util.List;

public class BulldozerProducer implements MachineProducer<Bulldozer> {

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. Remove the empty line at the start of the class definition.

@Override
public List<? extends Machine> get() {

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 parametrized with Bulldozer, not the raw Machine type to avoid using raw types and to ensure type safety.

Machine machine = new Bulldozer();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to declare a Machine type variable; you can directly create a list of Bulldozer objects. Consider creating a local variable of type List<Bulldozer> to store the bulldozers and return it.

return List.of(machine);
}
}
12 changes: 12 additions & 0 deletions src/main/java/core/mate/academy/model/ExcavatorProducer.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package core.mate.academy.model;

import core.mate.academy.service.MachineProducer;
import java.util.List;

public class ExcavatorProducer implements MachineProducer<Excavator> {

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. Remove the empty line before the class declaration.

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. Remove the empty line at the beginning of the class definition.

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.

@Override
public List<? extends Machine> get() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method get should return List<Excavator> instead of using a wildcard type List<? extends Machine>. This ensures that the list is specifically typed to Excavator, which is more precise.

Machine machine = new Excavator();

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 list of machines. This can help make the code more readable and maintainable. For example: List<Excavator> machines = List.of(new Excavator()); return machines;

return List.of(machine);
}
}
2 changes: 2 additions & 0 deletions src/main/java/core/mate/academy/model/Truck.java
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@

package core.mate.academy.model;

/**
* Add some custom fields that could be only in Truck
* Do not remove no-field constructor
*/

public class Truck extends Machine {

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 define fields with private access modifiers and provide public getters and setters for accessing them.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class Truck should contain custom fields that represent the state of a truck. For example, you might have fields such as loadCapacity, engineType, etc. Make sure to add these fields with appropriate access modifiers and methods (getters/setters).

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. It's good practice to start the class body immediately after the class declaration line to avoid unnecessary whitespace.

public Truck() {
}
Expand Down
12 changes: 12 additions & 0 deletions src/main/java/core/mate/academy/model/TruckProducer.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package core.mate.academy.model;

import core.mate.academy.service.MachineProducer;
import java.util.List;

public class TruckProducer implements MachineProducer<Truck> {

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. The empty line after the class declaration is unnecessary and should be removed.

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. Remove the empty line before the class declaration.

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. Remove the empty line at the start of the class definition.

@Override
public List<? extends Machine> get() {
Machine machine = new Truck();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are creating a Machine type object instead of a Truck type object. Since this is a TruckProducer, you should instantiate a Truck object directly.

return List.of(machine);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the return type of the get() method does not fully utilize generics to ensure type safety. According to the checklist, the MachineProducer interface should be parameterized in such a way that its implementations must use Machine or its subtypes. Consider changing the return type to List<Truck> to match the TruckProducer class's type parameter.

}
}
5 changes: 3 additions & 2 deletions src/main/java/core/mate/academy/service/MachineProducer.java
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
package core.mate.academy.service;

import core.mate.academy.model.Machine;
import java.util.List;

public interface MachineProducer {
public interface MachineProducer<T extends Machine> {

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. Here, the interface declaration should not be followed by an empty line.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameterize MachineProducer in such a way that any implementation NOT using Machine or its subtype could NOT be created. You've done this correctly by using T extends Machine.

/**

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't begin interface or method implementation with an empty line. It is unnecessary and does not follow Java code style conventions.

* In your implementations - create 2-3 models, add them to the list and return
* @return - the list of models
*/
List<Object> get();
List<? extends Machine> get();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MachineProducer interface's get() method should be parameterized with the generic type parameter T to ensure that any implementation is using Machine or its subtype. The current signature allows for raw usage which can lead to unchecked assignment warnings and should be avoided. Consider changing the return type to List<T>.

}
10 changes: 6 additions & 4 deletions src/main/java/core/mate/academy/service/MachineService.java
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package core.mate.academy.service;

import core.mate.academy.model.Machine;
import java.util.List;

/**
* Parameterize this service and add its implementation in a separate class.
*/
public interface MachineService {

public interface MachineService<T> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't begin interface or method implementation with empty lines.

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 excess empty lines to improve code readability.


/**
* Return the list of machines.
Expand All @@ -16,7 +18,7 @@ public interface MachineService {
* @param type - any class of Machine sub class. For example: Truck.class or Bulldozer.class
* @return the list of machines
*/
List<Object> getAll(Class type);
List<Machine> getAll(Class<? extends Machine> type);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not use raw types. Always parametrize interfaces and classes if it is possible. The return type of 'getAll' should be parameterized with the generic type 'T' instead of 'Machine'.


/**
* Fill the machines list with passed value
Expand All @@ -26,13 +28,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<Object> machines, Machine value);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'fill' method should use the generic type 'T' for the 'machines' list instead of 'Object'. This ensures type safety.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fill method should not use raw types. Consider parameterizing the machines list to be List<T> instead of List<Object>, ensuring type safety.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fill method should have a bounded wildcard for the machines list to ensure type safety. Use List<? super T> to allow lists of any supertype of T.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
void fill(List<Object> machines, Machine value);
void fill(List<? super T> machines, T value);


/**
* Call the method doWork() from each machine.
* This method should be able to accept a list of bulldozers as well as list of trucks.
*
* @param machines - the list of machines
*/
void startWorking(List<Object> machines);
void startWorking(List<? extends Machine> machines);

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 using a bounded wildcard, but consider if it could be better to use the generic type 'T' as the parameter to ensure that the method works with the specific machine type that the service is intended for.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
void startWorking(List<? extends Machine> machines);
void startWorking(List<? extends T> machines);

}
50 changes: 49 additions & 1 deletion src/main/java/core/mate/academy/service/MachineServiceImpl.java
Original file line number Diff line number Diff line change
@@ -1,7 +1,55 @@
package core.mate.academy.service;

import core.mate.academy.model.Bulldozer;
import core.mate.academy.model.BulldozerProducer;
import core.mate.academy.model.Excavator;
import core.mate.academy.model.ExcavatorProducer;
import core.mate.academy.model.Machine;
import core.mate.academy.model.Truck;
import core.mate.academy.model.TruckProducer;
import java.util.ArrayList;
import java.util.List;

/**
* Your implementation of MachineService.
*/
public class MachineServiceImpl {

public class MachineServiceImpl<T extends Machine> implements MachineService<T> {

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 (lines 13-16).

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 (lines 13-15).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class 'MachineServiceImpl' should not start with an empty line according to the common mistakes checklist.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public class MachineServiceImpl<T extends Machine> implements MachineService<T> {
public class MachineServiceImpl implements MachineService<Machine> {


private final Machine truck = new Truck();
private final Machine excavator = new Excavator();
private final Machine bulldozer = new Bulldozer();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using specific type parameters instead of the raw Machine type for the fields 'truck', 'excavator', and 'bulldozer'. This will ensure type safety.


@Override
public List<Machine> getAll(Class<? extends Machine> type) {

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 instead of creating multiple lists for each machine type.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'getAll' method should return a list of the specified type 'T', not the raw 'Machine' type. Adjust the return type and the logic to comply with the generic type 'T'.

List<Machine> machines = new ArrayList<>(); // Ініціалізуємо список Machine
List<? extends Machine> truckProducer = new TruckProducer().get();
List<? extends Machine> excavatorProducer = new ExcavatorProducer().get();
List<? extends Machine> bulldozerProducer = new BulldozerProducer().get();

if (Truck.class.isAssignableFrom(type)) {
machines.addAll(truckProducer);
} else if (Bulldozer.class.isAssignableFrom(type)) {
machines.addAll(bulldozerProducer);
} else if (Excavator.class.isAssignableFrom(type)) {
machines.addAll(excavatorProducer);
}
return machines;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'getAll' method should return an empty list instead of null when no producer has been found for the given type. Make sure to handle this case correctly.

}

@Override
public void fill(List<Object> machines, Machine value) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'fill' method should use List as a parameter instead of List to ensure type safety and avoid potential class cast issues at runtime.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'fill()' method should use a List of type T, not Object, to ensure type safety. Please correct the method signature to 'List machines' and adjust the implementation accordingly.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public void fill(List<Object> machines, Machine value) {
public void fill(List<? super Machine> machines, Machine value) {
for (int i = 0; i < machines.size(); i++) {
machines.set(i, value);
}
}

machines.replaceAll(ignored -> value);
Comment on lines +37 to +38

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fill() method should use generics for its machines parameter to ensure type safety. Please change the parameter type from List<Object> to List<T> where T is a bounded type parameter extending Machine.

Comment on lines +37 to +38

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. This implementation is correct, but please ensure the generic type is used for the method parameter.

}

@Override
public void startWorking(List<? extends Machine> machines) {
if (machines.contains(truck)) {
truck.doWork();
} else if (machines.contains(excavator)) {
excavator.doWork();
} else if (machines.contains(bulldozer)) {
bulldozer.doWork();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'startWorking' method logic is incorrect. It should iterate over the list of machines and call 'doWork()' on each machine. The current implementation checks if the list contains one of the specific machines created as fields, which is not the intended behavior.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fields 'truck', 'excavator', and 'bulldozer' are not used correctly. They are not needed as fields and should be removed. The 'startWorking' method should work with the passed list of machines.

}
}
Loading