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

Develop #2155

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

Develop #2155

wants to merge 19 commits into from

Conversation

MrGizmen
Copy link

No description provided.

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.

It looks like you've put in a lot of effort here, and you're almost there! 👍 However, there are a few key areas we need to address to make sure the functionality is spot on. The messages in the startWork and stopWork methods across different machine types need to be more informative. Also, using polymorphism with the Machine class can make your code cleaner and more flexible. Let's make these adjustments to get your code in tip-top shape! 🛠️ Remember, these changes are important for maintaining clear communication in your program's output and for future-proofing your code against changes. Keep up the great work, and I'm confident you'll have this polished in no time! 🌟

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

src/main/java/core/basesyntax/Bulldozer.java Outdated Show resolved Hide resolved
src/main/java/core/basesyntax/Bulldozer.java Outdated Show resolved Hide resolved
src/main/java/core/basesyntax/Excavator.java Outdated Show resolved Hide resolved
src/main/java/core/basesyntax/Excavator.java Outdated Show resolved Hide resolved
src/main/java/core/basesyntax/Excavator.java Outdated Show resolved Hide resolved
src/main/java/core/basesyntax/Machine.java Outdated Show resolved Hide resolved
src/main/java/core/basesyntax/MainApp.java Outdated Show resolved Hide resolved
src/main/java/core/basesyntax/MainApp.java Outdated Show resolved Hide resolved
src/main/java/core/basesyntax/Truck.java Outdated Show resolved Hide resolved
src/main/java/core/basesyntax/Truck.java Outdated Show resolved Hide resolved
@MrGizmen
Copy link
Author

.

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 Machine class and adhering to the checklist! Remember that informative messages can greatly improve the readability and maintainability of your code, so keep that in mind for future projects. Keep up the good work! 😊👍

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


@Override
public void doWork() {
System.out.println("Bulldozer" + " " + machineType + " " + "started its work.");

Choose a reason for hiding this comment

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

The message in the doWork method should be more informative and indicate that the Bulldozer is working. Consider including the specific action it performs, for example, 'Bulldozer is moving earth'.


@Override
public void stopWork() {
System.out.println("Bulldozer" + " " + machineType + " " + "stopped working.");

Choose a reason for hiding this comment

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

Similarly, the message in the stopWork method should be more informative. Rather than just saying 'stopped working', it could be more descriptive, like 'Bulldozer has finished moving earth'.


@Override
public void doWork() {
System.out.println("Exavator" + " " + machineType + " " + "started its work.");

Choose a reason for hiding this comment

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

There's a typo in the message printed to the console. It should be 'Excavator' instead of 'Exavator'. Make sure the messages are informative and correctly spelled.


@Override
public void stopWork() {
System.out.println("Exavator" + " " + machineType + " " + "stopped working.");

Choose a reason for hiding this comment

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

The same typo is present in the 'stopWork' method. It should be corrected to 'Excavator'.


Machine[] machines = {truck, bulldozer, excavator};
printTotal(machines);
System.out.println();

Choose a reason for hiding this comment

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

There's a redundant empty line after the call to printTotal(machines) method. According to the checklist, we should avoid adding unnecessary empty lines after method calls.

machine.doWork();
machine.stopWork();
}
System.out.println(Arrays.toString(machines));

Choose a reason for hiding this comment

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

The System.out.println(Arrays.toString(machines)); does not provide an informative message about the machines. It's better to have messages that indicate what type of machine is working. Consider overriding the toString method in each Machine subclass to provide more informative output.


@Override
public void doWork() {
System.out.println("Truck" + " " + machineType + " " + "started its work.");

Choose a reason for hiding this comment

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

The message should be more informative and in proper English. Instead of concatenating strings with spaces, you can use a formatted string. For example: System.out.println("Truck " + machineType + " started its work.");


@Override
public void stopWork() {
System.out.println("Truck" + " " + machineType + " " + "stopped working. ");

Choose a reason for hiding this comment

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

The stop message should also be informative and properly formatted. Consider changing it to: System.out.println("Truck " + machineType + " stopped working.");


@Override
public String toString() {
return machineType;

Choose a reason for hiding this comment

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

The toString method should return a string that represents the object. Returning only machineType is not very informative. Consider including the class name and other relevant information.

Copy link

@slade13 slade13 left a comment

Choose a reason for hiding this comment

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

Please remove machineType variable. Besides that everything seems to be correct, please check my comments.

package core.basesyntax;

public abstract class Machine {
protected String machineType;
Copy link

Choose a reason for hiding this comment

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

The 'machineType' variable seems unnecessary in this context as the type of machine can be determined by the instance of the class. Consider removing this and the associated constructor and toString method if not needed for other parts of your program.

public class MainApp {
public static void main(String[] args) {

Machine truck = new Truck("Mikki");
Copy link

Choose a reason for hiding this comment

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

The constructors for Truck, Bulldozer, and Excavator classes are not defined in the task. If these classes have constructors that take parameters, mention it in the task definition. If not, use default constructors instead.

machine.doWork();
machine.stopWork();
}
System.out.println(Arrays.toString(machines));
Copy link

Choose a reason for hiding this comment

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

This line is unnecessary. The printTotal method is supposed to invoke doWork() and stopWork() on each machine, there is no need to print an array of Machines.

Copy link
Author

Choose a reason for hiding this comment

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

So, should only output this?

Truck Mikki started its work.
Truck Mikki stopped working.
Bulldozer Eduardo started its work.
Bulldozer Eduardo stopped working.
Excavator Maks started its work.
Excavator Maks stopped working.
image


public class Excavator extends Machine {

public Excavator(String machineType) {
Copy link

Choose a reason for hiding this comment

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

The constructor for the Excavator class is unnecessary as it just calls the superclass constructor. Consider removing it if there's no additional logic inside.


@Override
public String toString() {
return machineType;
Copy link

Choose a reason for hiding this comment

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

It's not clear what the purpose of the overridden toString() method is, as it only returns 'machineType'. If it's not necessary, consider removing it to use the inherited method from Object class.


public class Truck extends Machine {

public Truck(String machineType) {
Copy link

Choose a reason for hiding this comment

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

The Machine class was not described as having a constructor that takes a machineType parameter. It's better to remove the constructor from the Truck class.

}

@Override
public String toString() {
Copy link

Choose a reason for hiding this comment

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

The 'machineType' field is not defined in this class. If 'machineType' is a field in the parent class, it should be accessed using 'super.machineType'. Or better yet, since this class already represents a specific machine type (Bulldozer), 'machineType' is not needed in the toString() method. You can return "Bulldozer" instead.


public class Bulldozer extends Machine {

public Bulldozer(String machineType) {
Copy link

Choose a reason for hiding this comment

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

The constructor is taking a parameter 'machineType' that seems unnecessary. The type of machine is already known (Bulldozer) and does not need to be passed as a parameter.


@Override
public void doWork() {
System.out.println("Bulldozer" + " " + machineType + " " + "started its work.");
Copy link

Choose a reason for hiding this comment

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

The 'machineType' field is not defined in this class. If 'machineType' is a field in the parent class, it should be accessed using 'super.machineType'. Or better yet, since this class already represents a specific machine type (Bulldozer), 'machineType' is not needed. Simply print "Bulldozer started its work."


@Override
public void stopWork() {
System.out.println("Bulldozer" + " " + machineType + " " + "stopped working.");
Copy link

Choose a reason for hiding this comment

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

The 'machineType' field is not defined in this class. If 'machineType' is a field in the parent class, it should be accessed using 'super.machineType'. Or better yet, since this class already represents a specific machine type (Bulldozer), 'machineType' is not needed. Simply print "Bulldozer stopped working."

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.

Please read again comments provided by mentor in previous review.

@@ -0,0 +1,13 @@
package core.basesyntax;

public abstract class Machine {

Choose a reason for hiding this comment

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

This is how this class should look like:

public abstract class Machine {
    public abstract void doWork();

    public abstract void stopWork();
}


public class Excavator extends Machine {

public Excavator(String machine) {

Choose a reason for hiding this comment

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

All constructors & toString methods should be removed.

Comment on lines 9 to 18
@Override
public void doWork() {
System.out.println("Excavator" + " " + machine + " " + "started its work.");
}

@Override
public void stopWork() {
System.out.println("Excavator" + " " + machine + " " + "stopped working.");

}

Choose a reason for hiding this comment

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

Please read previous comments about these methods.

public static void main(String[] args) {
Machine truck = new Truck("Katarzyna");
Machine bulldozer = new Bulldozer("Eduardo");
Machine excavator = new Excavator("Maks");

Choose a reason for hiding this comment

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

As Mateusz mentioned in the previous comment, you should remove all constructors from Machine child classes. If Java class doesn't have any explicitly defined constructor it will use the default one, so in this case you will be able to create Truck/Bulldozer/Excavator instances like this: Machine truck = new Truck(); Notice that in this case you don't have to provide "name" of the machine, because you are able to use default constructor, i.e. new Truck(); That's why you should remove "name" fields in all child classes and delete a constructors you created.

public class Excavator extends Machine {
private String name;

public Excavator(String name) {

Choose a reason for hiding this comment

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

Remove constructor and "name" field to be able to create instances without providing a name

public class Bulldozer extends Machine {
private String name;

public Bulldozer(String name) {

Choose a reason for hiding this comment

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

Remove constructor and "name" field to be able to create instances without providing a name


public Truck(String name) {
this.name = name;
}

Choose a reason for hiding this comment

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

Remove constructor and "name" field to be able to create instances without providing a name

Copy link

@George-Khomiak George-Khomiak left a comment

Choose a reason for hiding this comment

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

Well done, just to clarify one thing - the constructor you are creating i.e.
public Bulldozer() { } is actually redundant, because Java Classes have something called "default constructor" and if you didn't create any constructor it will have default one, even though you don't see it. And it looks exactly the same as yours
public Bulldozer() { }

So basically you created a default constructor, which is already there.
Anyways, good job :)

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.

5 participants