views:

178

answers:

2

I have some code which is the same except that a certain sequence of assignments happen in slightly different orders.

It is easy to factor this out into a method parameter of type int[], which denotes the order.

However, I find it isn't the clearest.

Another option was to factor them out into a object of type AssignmentOrders. I can also do validation on the values in the object constructor that I wasn't able to do with the array. This would be the "Introduce Parameter Objects" refactoring from the book, Refactoring.

I am wondering if this particular refactoring is overkill and I should just stick with the int[]?

CODE:

Three Samples of the Originals:

private static PersonDetails parseLine(String line, String deliminator, int[] orderOfSections) 
            throws Exception {
     String[] sections = line.split(deliminator);

     String value1 = sections[0].trim();
     String value2 = sections[1].trim();
     String value3 = sections[4].trim();
     String value4 = sections[2].trim();
     String value5 = sections[3].trim();

     //........
    }

private static PersonDetails parseLine(String line, String deliminator) 
            throws Exception {
     String[] sections = line.split(deliminator);

     String value1 = sections[1].trim();
     String value2 = sections[0].trim();
     String value3 = sections[2].trim();
     String value4 = sections[3].trim();
     String value5 = sections[4].trim();

     //........
    }

private static PersonDetails parseLine(String line, String deliminator, int[] orderOfSections)


        throws Exception {
     String[] sections = line.split(deliminator);

     String value1 = sections[0].trim();
     String value2 = sections[1].trim();
     String value3 = sections[2].trim();
     String value4 = sections[4].trim();
     String value5 = sections[5].trim();

     //........
    }

How I've refactored the above 3 into this:

private static PersonDetails parseLine(String line, String deliminator, int[] orderOfSections) 
            throws Exception {
     String[] sections = line.split(deliminator);

     String value1 = sections[orderOfSections[0]].trim();
     String value2 = sections[orderOfSections[1]].trim();
     String value3 = sections[orderOfSections[2]].trim();
     String value4 = sections[orderOfSections[3]].trim();
     String value5 = sections[orderOfSections[4]].trim();

     //........
    }

How I could theoretically refactor it into a parameter object:

private static PersonDetails parseLine(String line, String deliminator, OrderOfSections order) 
        throws Exception {
     String[] sections = line.split(deliminator);

     String value1 = sections[order.getValue1Idx].trim();
     String value2 = sections[order.getValue2Idx].trim();
     String value3 = sections[order.getValue3Idx].trim();
     String value4 = sections[order.getValue4Idx].trim();
     String value5 = sections[order.getValue5Idx].trim();

     //........
    }

What I was thinking of doing was creating a specific class instead of using int[]... But wondered if that would be overkill.

The benefits are that it would be more readable. Instead of orderOfSections[0], it might be orderOfSections.value1SectionIdx... I could also put some validation code into the class.

I believe this is what Martin Fowler calls Introducing a Parameter Object.

EDIT:

Another option would be to use a Dictionary. Lighter-weight than a new class, but more descriptive... Then I could use something like orderOfSections["value1"]

+1  A: 

IMHO, the simplest and most readable way is to pass a map instead of an int array.

And depending on what your fields for PersonDetails look like, you could even use reflection and assign the values in a loop.

JRL
For my purposes I think this suits me best.
Alex Baranosky
+4  A: 

Instead of passing in a Class or array that simply indicates the ordering of the items in the raw String and how they should be assigned, I would delegate the parsing of the input line to this Class. It would be much more readable to do the following:

private static PersonDetails parseLine(String line, String deliminator, 
                         SectionsReader reader) throws Exception 
{
    reader.setLine(line);
    String value1 = reader.getValue1();
    String value2 = reader.getValue2();
    String value3 = reader.getValue3();
    String value4 = reader.getValue4();
    String value5 = reader.getValue5();

    //........
}

In the end, this would not be overkill, and you will thank yourself in 3 months' time when you go back to this code and find it more intelligible.

akf
ooooh, very nice, thank you. See I knew this code could be refactored for better readability and thus better maintainability. Thank you.
Alex Baranosky