3

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?)

  • is the variable mapOfFeatures correctly named? Or should this be mapOfAttributes ( mapOfFeatures is not passed to the method...I am wondering if it is a static variable) – Ivan Apr 5 '17 at 13:06
  • Ivan, you're right. Sorry about that. The variable mapOfFeatures actually is the same as mapOfAttributes. I changed the question accordingly. – Andreas Braun Apr 5 '17 at 14:20
  • At a minimum, the very generic code that splits the "key=value" into trimmed key and value should be put into a separate function. – user949300 Apr 5 '17 at 14:24
  • Is it correct that the map named "invalidLines" will always return a single entry, with ID as the key? Just returning the last invalid? – Darius X. Apr 8 '17 at 3:42
5

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.

  • I am surprised to hear you say you think this method does one thing. The method has two separate outputs -- the returned output -- and the invalidLines output parameter. As you not that is a significant design smell – Ivan Apr 5 '17 at 13:11
  • 1
    @Ivan Yes, but the outputs are related. They both describe the contents of the source text; neither alone entirely describes it. Therefore they should be combined into a single object ... There is a code smell, but it indicates that the is a missing abstraction, not a function that had multiple responsibilities. – Jules Apr 5 '17 at 13:21
  • 2
    I have a rule of thumb, no logic in loops, it is one method's responsibility to iterate, and an other method's to execute the logic. This method does way more than one thing. – Chris Wohlert Apr 5 '17 at 13:22
  • @ChrisWohlert .... I agree. I'm not saying that refactoring the method to increase its level of abstraction wouldn't help make it cleaner. But the idea of separating the processing into one process that handles valid lines and one that handles invalid one is not the right approach, and is driven by a misunderstanding of SRP. Probably, I'd have some kind of Document class (perhaps FeatureAttributes), with an addLine method called by this method for each line. The actual parsing would occur there. – Jules Apr 5 '17 at 13:29
  • @Jules, The parsing process is a single, atomic concern. Which means this atomic process should be separated from the rest of the logic, the looping, checking for empty, and so on. – Chris Wohlert Apr 5 '17 at 13:37
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.
  • Sorry about the mapOfFeatures mistake. That should have been called mapOfAttributes. I changed it in the question. You say 'you can do the extraction in a seperate method, so that you don't have duplicate code for this.' How would I do this? This is not clear to me. I agree on you saying 'Also you dont necessarily need to put it again in a map for duplicate detection.' but still I have to check for duplicates in some other way. – Andreas Braun Apr 5 '17 at 14:26
  • Note that your check for duplicates only returns the last duplicate. If there are multiple duplicates is that o.k.? – user949300 Apr 5 '17 at 14:32
2

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.

  • Thanks, your answer already helped a lot. How would you deal with the output argument issue? – Andreas Braun Apr 5 '17 at 14:35
  • I'll edit the answer - This is the same solution Jules proposed. – Chris Wohlert Apr 6 '17 at 6:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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