-2
\$\begingroup\$

I've got a class with few fields:

public class Helper {
    String first;
    String second;
    String third;
    String fourth;
    String fifth;
    String sixth;
}

And helper service with method which I try to refactor.

public class HelperService {

    public Helper toRefactor(String[] attributes) {
        Helper testObject = new Helper();

        if (attributes.length > 0) {
            testObject.first = attributes[0];
            if (attributes.length > 1) {
                testObject.second = attributes[1];
            }
            if (attributes.length > 2) {
                testObject.third = attributes[2];
            }
            if (attributes.length > 3) {
                testObject.fourth = attributes[3];
            }
            if (attributes.length > 4) {
                testObject.fifth = attributes[4];
            }
            if (attributes.length > 5) {
                testObject.sixth = attributes[5];
            }
        }
        return testObject;
    }
}

My ideal solution would've look like:

    for (int i = 0; i > attributes.length; i++) {
        testObject.array[i] = attributes[i];
    }

But this gnerates syntax error on testObject.array[i] part

\$\endgroup\$
12
  • \$\begingroup\$ Are you sure first, second, third... should be separate fields? Are you sure attributes should be an array of unknown length? Translating data from an ordered sequence to an object with named fields will likely require bulk. It may be better to look at why things are set up the way they are. \$\endgroup\$ Feb 12, 2019 at 21:14
  • \$\begingroup\$ What do you mean by "Translating data from an ordered sequence to an object with named fields will likely require bulk."? And yeah, I'm sure that they need to be separated fields. I will try to investigate why attributes are unknown array length... \$\endgroup\$
    – degath
    Feb 12, 2019 at 21:27
  • \$\begingroup\$ I mean you are moving data between two structures that are holding data differently. Somewhere, you need to have code that says attributes[0] should be stored in first and so on. If you can guarantee attributes will always have 5 elements, you could give the class a constructor and do new Helper(attributes[0], attributes[1], attributes[2], attributes[3], attributes[4], attributes[5]) or something similar. \$\endgroup\$ Feb 12, 2019 at 21:37
  • \$\begingroup\$ Yeah, but I don't know how many attributes there will be, so constructor isn't the way :( \$\endgroup\$
    – degath
    Feb 12, 2019 at 21:41
  • \$\begingroup\$ Will this code be run super often, or only occasionally? \$\endgroup\$ Feb 12, 2019 at 21:43

2 Answers 2

1
\$\begingroup\$

I'm not sure how much neater you consider this (if at all), but a switch could be used here:

public static Helper toRefactor2(String[] attributes) {
    Helper testObject = new Helper();

    switch(attributes.length) {
        case 6: testObject.sixth = attributes[5];
        case 5: testObject.fifth = attributes[4];
        case 4: testObject.fourth = attributes[3];
        case 3: testObject.third = attributes[2];
        case 2: testObject.second = attributes[1];
        case 1: testObject.first = attributes[0];
        case 0: break

        default: throw new IllegalArgumentException("Invalid array of length " + attributes.length)
    }

    return testObject;
}

This takes advantage of the fall-through.

Thanks to @AJ for rubustness suggestions.


You could also standardize the length of the array by making a copy of it. This does away with the need for length checks if you can't guarantee the length ahead of time:

public static Helper toRefactor3(String[] attributes) {
    Helper testObject = new Helper();

    String[] std = Arrays.<String>copyOf(attributes, 5);

    testObject.first = std[0];
    testObject.second = std[1];
    testObject.third = std[2];
    testObject.fourth = std[3];
    testObject.fifth = std[4];
    testObject.sixth = std[5];

    return testObject;
}

I'm not sure how people would feel about this though. It's quite fast at least. I benchmarked copying an array that small using Criterium (in Clojure, but that shouldn't matter much). It's basically instantaneous:

(let [arr (to-array ["ABCDEFG"
                     "HIJKLMNOP"
                     "QRSTUVWXYZ"
                     "QWERTYUIOP"
                     "ASDFGHJKL"])]

  (cc/bench
    (Arrays/copyOf arr 5)))

Evaluation count : 2908877280 in 60 samples of 48481288 calls.
             Execution time mean : 17.184167 ns
    Execution time std-deviation : 1.008121 ns
   Execution time lower quantile : 15.294929 ns ( 2.5%)
   Execution time upper quantile : 19.609513 ns (97.5%)
                   Overhead used : 3.609363 ns

Found 5 outliers in 60 samples (8.3333 %)
    low-severe   1 (1.6667 %)
    low-mild     4 (6.6667 %)
 Variance from outliers : 43.4678 % Variance is moderately inflated by outliers

And I agree with @AJ. In the current context, that method should be static.

\$\endgroup\$
2
  • \$\begingroup\$ I like the switch statement approach. Fast & type safe. And no unnecessary reflection, so using an IDE to rename a field won’t break the code by missing a string that happens to contain the field name. The only fragile part is if you pass in an array of 7 or more items, nothing will get copied. So, I’d add a case 0: break; followed by default: throw new IllegalArgumentException(); just to be safe. \$\endgroup\$
    – AJNeufeld
    Feb 13, 2019 at 0:34
  • \$\begingroup\$ @AJNeufeld Oh, good point. Sec. \$\endgroup\$ Feb 13, 2019 at 0:51
1
\$\begingroup\$

Your HelperService.toRefactor() doesn't seem to be accessing any data in its own HelperService instance, so it probably should be a static method.

Your "ideal" solution would not work well, since i > attributes.length would never be true; the loop would never start.

    for (int i = 0; i > attributes.length; i++) {
        testObject.array[i] = attributes[i];
    }

Perhaps you meant i < attributes.length.


Reflection may be useful here. It is close to your 'ideal' solution.

private final static String[] fields = { "first", "second", "third", "fourth", "fifth", "sixth" };

public static Helper toRefactor(String[] attributes) {
    Helper testObject = new Helper();

    for (int i=0; i<attributes.length; i++) {
        Field field = Helper.class.getDeclaredField(fields[i]);
        field.setAccessible(true);
        field.set(testObject, attributes[i]);
    }

    return testObject;
}
\$\endgroup\$

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