2
\$\begingroup\$

My code below is working but I am searching for performance and improvement. It consists of looping my PartList elements and check if my string parameter orginalDataStringList contains the elements on label string for each Part element.

private boolean excludeAllInformationVariation(List<String> orginalDataStringList, PartList parts) {
    for (Part part : parts) {
        List<String> listTemp = new ArrayList<String>();
        listTemp.addAll(orginalDataStringList);
        if (!part.getOptLogic().isLabelNullOrEmpty()) {
            String[] dataArrays = part.getOptLogic().getLabel().split("( )");
            for (String data : dataArrays) {
                listTemp.remove(data);
            }
        } else {
            return false;
        }
        if (!listTemp.isEmpty()) {
            return false;
        }
    }
    return true;
}

public class PartList extends ArrayList<Part> {
    public PartList() {
        //Empty constructor
    }
}

public class Part {
    private Logic optLogic;
    public Logic getOptLogic() { return optLogic; }
    public void setOptLogic(Logic optLogic) { this.optLogic = optLogic; }
}

public class Logic {
    private String label;
    public String getLabel() { return label; }
    public void setLabel(String label) { this.label = label; }
}
public boolean isLabelNullOrEmpty() {
    return (this.getLabel() == null || this.getLabel().isEmpty()) ? true : false;
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ return (condition) ? true : false; don't do that! Just return the condition! \$\endgroup\$ Apr 26, 2017 at 21:00

1 Answer 1

1
\$\begingroup\$

Thanks for sharing your code.

Naming

Finding good names is the most important but also hardest part in programming. So always take your time to find good names that express your intention of what the code does.

The name of your method excludeAllInformationVariation is a lie. It implies that there is something changed either in the object itself (its state) or the passed parameter object. But this method does not alters neither the objects stat nor the parameter. Therefore it should have a different name. Since it returns a boolean this name should start with is or has. eg.: isAllDataCovered(...)

Single Responsibility / Separation of Concerns

Your classes Part and Logic look like data transfer objects (DTOs), but the method isLabelNullOrEmpty() is some kind of business logic.

You should not mix DTOs with business logic. The method isLabelNullOrEmpty() should be in the same class as the method excludeAllInformationVariation and have a parameter of type String on which it should work.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.