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

Question

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.

The flag itself is not bad if you know what you are doing. But if you use it for having huge function blocks. That is bad i think. — Novy, Nov 30 '17 at 16:50
In your example, why have the boolean when `if(childDataImporter != null)` would do the same thing? And there's no danger of a bug where the boolean and the truth go out of sync. — slim, Nov 30 '17 at 17:37
@slim But `childDataImporter` isn't available outside the loop. — linuxNoob, Nov 30 '17 at 17:40

DwB · Accepted Answer · 2017-11-30T17:31:08.550

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.

I think I get in general what you're conveying but in this case how will `doYourCategorStuff()` get rid of the boolean without introducing more private helper methods? — linuxNoob, Nov 30 '17 at 17:20
Ok, I reread your example code. The `boolean` your example code is not a parameter. My answer is related to boolean parameters. — DwB, Nov 30 '17 at 17:24

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

1 Answers1