search

nus-cs2103-AY2122S2 / forum

13 stars 1 forks source link

Arrowhead code #233

Closed hsiaojietng closed 2 years ago

hsiaojietng commented 2 years ago

Is this code considered arrowhead?

@Override
    public int getPersonsBasedOnRegion(String region) {
        int totalPersons = 0;
        //Resets to full list of Persons to prevent any logical error after `find` command
        model.updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS);
        if (Region.isValidRegion(region)) { //defensive code
            for (Person person : getFilteredPersonList()) {
                if (person.getUserType().isBuyer()) { //if is buyer, check region in preference
                    Preference preference = person.getPreference().isPresent() ? person.getPreference().get() : null;
                    if (preference != null && preference.getRegionInString().equals(region)) {
                        totalPersons++;
                    }
                } else { //if is seller, check region in Property
                    Set<Property> setOfPropertyValues = person.getProperties();
                    if (!setOfPropertyValues.isEmpty()) {
                        Iterator<Property> propertyIterator = setOfPropertyValues.iterator();
                        while (propertyIterator.hasNext()) { //Iterate through every property seller holds
                            Property property = propertyIterator.next();
                            if (property != null && property.getRegionInString().equals(region)) {
                                totalPersons++;
                            }
                        }
                    }
                }
            }
        }
        return totalPersons;
    }

If yes, may I know of any suggestions to change it? (Personally, I think it is an arrowhead code because of the additional defensive code checks, which should not count as arrowhead?)

Thank you for your help!

Yukun99 commented 2 years ago

I would say it is, perhaps try splitting into more methods?

damithc commented 2 years ago

@hsiaojietng as stated here, actual code is preferred over screenshots, to make it easy for others to copy-paste-modify the code in their answer.

flairekq commented 2 years ago

I think it is, perhaps you can try changing it to this:

if (!Region.isValidRegion(region)) { 
    return totalPersons;
}
for (Person person: getFilteredPersonList()) {
    Preference preference = person.getPreference.isPresent() ? person.getPreference().get() : null;
    if (person.getUserType().isBuyer() && preference != null && preference.getRegionInString().equals(region)) {
        totalPersons++;
        continue;
    }
    Set<Property> setOfPropertyValues = person.getProperties();
    for (Property p : setOfPropertyValues) {
        if (p != null && property.getRegionInString().equals(region)) {
            totalPersons++;
        }
    }
}
return totalPersons;
hsiaojietng commented 2 years ago

@hsiaojietng as stated here, actual code is preferred over screenshots, to make it easy for others to copy-paste-modify the code in their answer.

I apologize for that. I have rectified the issue.

hsiaojietng commented 2 years ago

@flairekq wow I did not think of that, really appreciate how simple it is now, thanks!

damithc commented 2 years ago

Good answer @flairekq In other words, use guard clauses to reduce nesting (and make the happy path prominent).