CODEWITHSUNDEEP
javaoopjava-7
linuxNoob
linuxNoob

Reputation: 720

Why is boolean flag within the method body a bad idea?

Suppose I have something as follows where DataImporter is a utility to retrieve data from the file system and has child data importers within it for retrieving data from the sub folders based on the category string:

        List<String> categories = getCategories();
        boolean doesChildImporterExist = false;
        for (String category : categories)
        {
            DataImporter childDataImporter=importer.getChild(category);
            if (childDataImporter != null)
            {
                doesChildImporterExist = true; 
                populateImportedData(childDataImporter.importData());
            }
        }
         if(!doesChildImporterExist)
            populateImportedData(importer.importData());

I know the other option is to construct a List of child data importers and check for its size, if it is 0 or not and based on that import the data using the desired importer. However, I'm trying to understand what is wrong with using the boolean flag here?

Assume that the code above is within a method and using Java 1.7.

Upvotes: 1

Views: 756

Answers (1)

DwB
DwB

Reputation: 38300

When you use a boolean flag in a method as a branch decider (not the best terminology), you are actually taking the functionality of two different methods and smashing them into one method.

Often, the better solution is to have a method for the shared functionality and a second method for the super set functionality.

For example:

public DataImporter doYourCategoryStuff()
{
    List<String> categories = getCategories();
    ... blah including the for loop.

    return theDataImporter;
}


public void doAllTheStuffs()
{
    final DataImporter theDataImporter;

    theDataImporter.doYourCategorStuff();

    populateImportedData(theDataImporter.importData());
}

Edit More to the point in your code.

In your code, the boolean flag indicates "I did something to a child importer and need to update parent importer". In this case you are smashing "identify things to update" and "do the update" together; split them.

Consider something like this:

Set<DataImporter> updateSet = new HashSet<>();

for (category for loop)
{
    final DataImporter child = importer.getChild(category);
    if (child != null)
    {
        updateSet.add(child);
        updateSet.add(importer);
    }
}

for (final DataImporter current : updateSet)
{
    current.importData();
}

Even though the add(importer) (the parent) may be called multiple times, the set will only ever contain one instance of each DataImporter. This should be reasonable even if you don't implement hashCode and equals on DataImporter, since the parent reference will always be the same.

Upvotes: 2

Related Questions