Tradeoff between clean code, duplicate code and code efficiency in java

Question

I have a question on writing clean code. I’m trying to refactor the following method:

private static Map<String, String> createMapOfAttributes(
        final String Id,
        final String attributes, 
        final Map<String, String> invalidLines
) {          
    final String[] arrayOfAttributes = attributes.split(";");
    final int numberOfAttributes = arrayOfAttributes.length;
    final Map<String, String> mapOfAttributes = 
        new HashMap<String, String>(numberOfAttributes);

    for (int i = 0; i < numberOfAttributes; i++) {
        final String attributeEntry = arrayOfAttributes[i];
        if (attributeEntry == null || attributeEntry.isEmpty()) {
            continue;
        }

        //extract family and attribute
        final int attributeEntryDelimitPosition =  
            attributeEntry.indexOf("=");

        final String family = 
            attributeEntry.substring(0, attributeEntryDelimitPosition).trim();

        final String attribute =
            attributeEntry.substring(attributeEntryDelimitPosition + 1).trim();

        final String familyAndAttribute = family + '=' + attribute;

        final String previousFamilyAndAttribute = 
            mapOfAttributes.put(family,familyAndAttribute);

        if (previousFamilyAndAttribute != null) {
            invalidLines.put(Id, family);
        }
    }
    return mapOfAttributes;
}

So the first two arguments are input arguments, the last argument is an output argument that is manipulated and then there is an output argument returned.

One guideline for writing clean code is that any method should do only one thing, which the method does not do.

When I try to separate the things the method does I run into a problem: in the four last lines of the for loop, the mapOfAttributes is filled and it is tested whether an entry already existed in the map; if so the entry is collected in the invalidLines map.

When I try to separate those two things I would come up with the following: one method returns the mapOfAttributes and a second method returns the invalidLines map. In the second method I would need to somehow test for each entry if it’s a dublicate entry, possibly by adding it again to a map and thereby doing the same as in the first method (leading to dublicate code and dublicate computational burden). Furthermore, I would need to have the code to extract family and attribute in both methods, also leading to dublicate code.

So my question is, what would be your take on this? How would you refactor the method? And also, in more general terms, are readable code, code efficiency and code dublicity contrary goals that sometimes cannot all be satisfied at the same time? (Which in this case might mean that there is no satisfying solution?)

Answer 1

I think you're taking the "do one thing" advice too literally. In my view, this method does do only one thing: it parses input in a text format into an internal data structure. That your data structure has two parts (the valid data and the list of invalid items) is, to my view, irrelevant. The parsing process is a single, atomic concern.

Things may be clearer if you collect the resulting map of valid items and list of invalid ones in a class so that you can return then both as an object. Output parameters are a design smell; the results of a function should be in it's return value, wherever possible.

Answer 2

For my taste you are doing too much in this method. You create an array of one parameter and then iterate over it. Each element is parsed in some way. Additionaly every element is collected into one of two containers (mapOfAttributes and invalidLines).

Imho its ok to have one method "createMapOfAttributes" that orchestrates these different operations, but I'ts a lot easier to understand if you would refactor the internals of the method into several smaller ones.

So I would have the for loop in the outer method (as is), and the parsing and collecting of items in seperate methods.

When I try to separate those two things I would come up with the following: one method returns the mapOfFeatures and a second method returns the invalidLines map.

yes

In the [this] second method I would need to somehow test for each entry if it’s a dublicate entry, possibly by adding it again to a map and thereby doing the same as in the first method (leading to dublicate code and dublicate computational burden). Furthermore, I would need to have the code to extract family and feature in both methods, also leading to dublicate code.

you can do the extraction in a seperate method, so that you dont have duplicate code for this. Also you dont necessarily need to put it again in a map for duplicate detection.

Also in general you should almost always favour a clean design and readability over a super optimized solution. Do this only if you really need to (is this code executed with really big maps and a lot of times in a timecritical operation?).

Some other issues:

• There is an undeclared variable: mapOfFeatures.
• Im not sure about the line with invalidLines.put(Id, family). This seems like a bug to me.
• invalidLines is an output parameter which I consider at least in this example as bad design. Try to search for a different solution.

Answer 3

Assuming Readability is the primary goal, you are definitely correct to refactor this method.

There are few things I would change off the bat.

The comment! - Whenever you have a comment like extract family and attribute, it is time to refactor. You had rightfully already discovered this, so now it is just a matter of making the implementation work.

You mention you already have a solution, but you don't like it due to code efficiency and code dublicity, and you are partly correct. code dublicity is considered as bad as unreadable code, it will introduce bugs, and decrease maintainability. code efficiency however is a lot more domain specific, and it is really hard to tell you what to do, but in my experience, anything like this isn't noteworthy.

So - to answer your question of how readable code, code efficiency and code dublicity all relates - Eliminate duplicity, and make it readable. Usually readability does not introduce duplicity since you refactor all logic out into smaller methods that are easier to reuse, so if you see this, it might be because you aren't done refactoring.

Your example

private static FamilyAttribute createMapOfAttributes(
        final String Id,
        final String attributes
)
{
    final String[] arrayOfAttributes = attributes.split(";");
    final int numberOfAttributes = arrayOfAttributes.length;
    final FamilyAttribute familyAttribute = new FamilyAttribute();

    for(int i = 0; i < numberOfAttributes; i++)
    {
        final String attributeEntry = arrayOfAttributes[i];
        if(isEmpty(attributeEntry)){ //Note
            continue;
        }
        //extract family and attribute - remove this comment!
        final String family = getFamily(familyAttribute, attributeEntry); //Note
        if(isFamilyInvalid()){ //Note
            familyAttribute.addInvalidLine(Id, family);
        }
    }
    return familyAttribute;
}

And the FamilyAttribute (might want some other name):

//Your class could look something like this.
public class FamilyAttribute{
    //your private maps here

    public void addAttribute(String name, string value){
        //add to map
    }

    public void addInvalidLine(String name, String value){
        //add to invalid lines map
    }

    public Map<String, String> getAttributes(){
        //return map of attributes
    }

    public Map<String, String> getInvalidLines(){
        //return map of invalid lines
    }
}

This is just a start, and more can be done, but I hope that the methods I have called is intuitive enough for you to know what to put in them. This allows you to write more English, and makes it more readable - clean code

---

Sidenote

I get the idea that you think that "a function must only do one thing" comes to mean, that a client of this class needs to call a method for each thing he wants done. This is not entirely true, a clean API requires public methods to do several things like get data -> error check the data -> manipulate the data -> return the data. This is totally fine, but when it comes to clean code is magic lies in method delegation. The client still only needs to call one method to get the data, but that method calls several other private methods that each do that one thing.