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

Created nessesery classes, changed Objects to PECS. #901

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Wersebu
Copy link

@Wersebu Wersebu commented Sep 1, 2024

Created nessesery classes, changed Objects to PECS. All tests and builds passed

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a 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 according to the checklist requirements! 🌟 Remember to keep an eye on the small details like unnecessary empty lines and direct field access, as they can affect the overall quality and maintainability of your code. Keep up the good work, and always strive for clean and readable code! 💻👍

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

@@ -5,7 +5,10 @@
* Do not remove no-args constructor
*/
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. Remove the unnecessary empty lines between the package declaration and the class declaration.

@@ -5,7 +5,10 @@
* Do not remove no-args constructor
*/
public class Bulldozer extends Machine {
private int fuelConsuptionPerHour;

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 fuelConsuptionPerHour should be private and accessed through getters and setters.

@@ -5,7 +5,10 @@
* Do not remove no-args constructor
*/
public class Excavator extends Machine {
private int shovelSize;

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. You should never want to expose the object fields directly. They should be accessed through special methods (getters and setters).

@@ -5,7 +5,10 @@
* 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.

According to the checklist, you should never want to expose the object fields directly. They should be accessed through special methods (getters and setters). Please add getter and setter methods for the maxCapacity field.

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.

public class MachineServiceImpl implements MachineService<Machine> {

@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. It's good practice to create a local variable for the result list, which will be returned at the end of the method. This makes the code more readable and maintainable.

} else if (type == Truck.class) {
producer = new TruckProducer();
} else {
return new ArrayList<>();

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. Instead of returning a new ArrayList<>(); directly, it's better to assign it to a local variable and return that variable. This ensures consistency and readability in the method's return points.

return new ArrayList<>();
}

List<? extends Machine> specifyMachines = producer.get();

Choose a reason for hiding this comment

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

There is no need to have a separate variable specifyMachines. You can directly assign the result of producer.get() to allMachines and return it, simplifying the code.


List<? extends Machine> specifyMachines = producer.get();
List<Machine> allMachines = new ArrayList<>(specifyMachines);
return allMachines;

Choose a reason for hiding this comment

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

The variable allMachines is unnecessary since you can return the new ArrayList<>(specifyMachines) directly. This will reduce the amount of code and improve readability.

/**
* Your implementation of MachineService.
*/
public class MachineServiceImpl {
public class MachineServiceImpl implements MachineService<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. There are unnecessary empty lines at the start of the class implementation.

…and it must be new empty array beacuse when I return null tests dont pass
Copy link

@MateuszRostek MateuszRostek left a comment

Choose a reason for hiding this comment

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

You should add constructors to Machine class:

    public Machine() {

    }

    public Machine(String name, String color) {
        this.name = name;
        this.color = color;
    }

Then in classes that extends Machine you should leave one empty constructor and add another one that will look like this (of course adjusted to the relevant fields). For example in the Bulldozer:

public Bulldozer() {
    }
    
    public Bulldozer(String name, String color, Integer fuelConsuptionPerHour) {
        super(name, color);
        this.bladeWidth = bladeWidth;
        this.cabCapacity = cabCapacity;
    }

Then in all producers you should create objects using the second constructor (this one with all fields), For example:

    @Override
    public List<Excavator> get() {
        List<Excavator> excavator = new ArrayList<>();

        excavatorList.add(new Excavator("Exc 3000", "Blue", 60.0, 5.5));
        excavatorList.add(new Excavator("Exc 5000", "Yellow", 100.0, 8.5));
        excavatorList.add(new Excavator("Exc 800", "Yellow", 20.5, 2.5));

        return excavator;
    }

Copy link

@MateuszRostek MateuszRostek left a comment

Choose a reason for hiding this comment

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

Good job! See one minor advice and implement it!

Comment on lines +15 to +30
@Override
public List<Machine> getAll(Class<? extends T> type) {
MachineProducer<? extends T> producer;

if (type == Bulldozer.class) {
producer = (MachineProducer<? extends T>) new BulldozerProducer();
} else if (type == Excavator.class) {
producer = (MachineProducer<? extends T>) new ExcavatorProducer();
} else if (type == Truck.class) {
producer = (MachineProducer<? extends T>) new TruckProducer();
} else {
return new ArrayList<>();
}

return (List<Machine>) producer.get();
}

Choose a reason for hiding this comment

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

Do not use over-complicated if-else constuction, bette to use switch here, for example:

    @Override
    public List<Machine> getAll(Class<? extends Machine> type) {
        List<? extends Machine> machines;
        switch (type.getSimpleName()) {
            case "Bulldozer" -> machines = new BulldozerProducerImpl().get();
            case "Excavator" -> machines = new ExcavatorProducerImpl().get();
            case "Truck" -> machines = new TruckProducerImpl().get();
            default -> {
                return Collections.emptyList();
            }
        }
        return new ArrayList<>(machines);
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants