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

solved task #909

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

solved task #909

wants to merge 6 commits into from

Conversation

LynAlinka
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.

Great job on implementing the classes and interfaces according to the checklist! 🌟 Remember to always return an empty list instead of null to avoid possible null pointer exceptions, and consider using local variables for better debugging and future code maintainability. Keep up the good work! 👍

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

Comment on lines 13 to 21
public List<T> getAll(Class<? extends T> type) {
if (type == Bulldozer.class) {
return (List<T>) new BulldozerProducer().get();
} else if (type == Excavator.class) {
return (List<T>) new ExcavatorProducer().get();
} else if (type == Truck.class) {
return (List<T>) new TruckProducer().get();
}
return new ArrayList<>();

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. Directly returning the result of new BulldozerProducer().get() is not recommended, as it does not allow for easy debugging or extension of the method's functionality. Instead, consider creating a local variable to hold the result before returning it.

Comment on lines 11 to 21
public class MachineServiceImpl<T extends Machine> implements MachineService<T> {
@Override
public List<T> getAll(Class<? extends T> type) {
if (type == Bulldozer.class) {
return (List<T>) new BulldozerProducer().get();
} else if (type == Excavator.class) {
return (List<T>) new ExcavatorProducer().get();
} else if (type == Truck.class) {
return (List<T>) new TruckProducer().get();
}
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. It's better to return an empty List to avoid potential null pointer exceptions and to provide a consistent API to the users of your method.

public class MachineServiceImpl<T extends Machine> implements MachineService<T> {
@Override
public List<T> getAll(Class<? extends T> type) {
if (type == Bulldozer.class) {

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 is no need for else after return in the if clause. The code can be made more concise by removing the else keyword when it follows a return statement.

Copy link

@Elena-Bruyako Elena-Bruyako left a comment

Choose a reason for hiding this comment

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

build failed, fix it

Copy link

@liliia-ponomarenko liliia-ponomarenko 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! Let's improve your solution ;)

public List<T> getAll(Class<? extends T> type) {
List<T> result = new ArrayList<>();

if (type == Bulldozer.class) {

Choose a reason for hiding this comment

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

better use method equals (the same for other blocks)

public class MachineServiceImpl<T extends Machine> implements MachineService<T> {
@Override
public List<T> getAll(Class<? extends T> type) {
List<T> result = new ArrayList<>();

Choose a reason for hiding this comment

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

Create List<? extends Machine> result = new ArrayList<>(); and avoid casting

Comment on lines 11 to 12
trucks.add(new Truck());
trucks.add(new Truck());

Choose a reason for hiding this comment

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

better use loop for creating a list of objects (the same for other classes)

}
if (type == Truck.class) {
List<Truck> trucks = new TruckProducer().get();
result.addAll((List<T>) trucks);

Choose a reason for hiding this comment

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

you can avoid calling the getAll method by assigning the result of the get method to a variable result

import java.util.ArrayList;
import java.util.List;

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.

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


public class MachineServiceImpl<T extends Machine> implements MachineService<T> {
@Override
public List<T> getAll(Class<? extends T> type) {

Choose a reason for hiding this comment

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

Suggested change
public List<T> getAll(Class<? extends T> type) {
public List<Machine> getAll(Class<? extends Machine> type)

}

@Override
public void fill(List<? super T> machines, T value) {

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<? super T> machines, T value) {
public void fill(List<? super Machine> machines, Machine value) {

}

@Override
public void startWorking(List<? extends T> machines) {

Choose a reason for hiding this comment

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

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

Copy link

@ahoienko ahoienko 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!

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