4
\$\begingroup\$

I'm working on a project where I needed to extract the indices for the headers of a CSV file to initialise objects. I thought that I should work on my documentation process and learn how to document code, seeing as I'm always reading about people not documenting code.

/**
 * Returns the indices of searched for headers, e.g. if the file looks like this: (Name Email
 * Phone Age) and we call the method: findIndices("Name", "Age", "Email", "Phone"), if returns
 * [0,3,1,2].
 *
 * Ignores case of letters, i.e. Age and AGE "is" the same.
 *
 * @param input Strings to search for in the header
 * @return int[] with indices for the columns
*/
public int[] findIndices(String... input) throws IllegalArgumentException {
    int[] indices = new int[input.length];
    if (input.length > this.headers.length) {
      throw new IllegalArgumentException("Amount of searched for headers, " + input.length
      + ", exceeded the actual amount of headers: " + this.headers.length);
    }
    boolean found = true; // Assume we will find all headers.
    int k = 0; // Counter variable.
    for (int i = 0; i < this.headers.length; i++) {
        for (String s : input) {
            if (s.equalsIgnoreCase(this.headers[i])) {
                indices[k++] = i;
                found = true;
            } else {
                found = false;
            }
        }
    }
    if (!found) {
        throw new IllegalArgumentException(
            "One or more of input arguments could not be found in the headers.");
    }
    return indices;
}

My own thoughts:

  1. The found flag seems poorly handled.
  2. This is part of a custom CSV-reader class, with a headers private field. This seems a bit weird to me, this could maybe be static and take a String[] as input as well.
  3. Should this actually throw an exception or should it just return a "bad" index? But I'm using an array to represent it. Null, maybe?

Edit: Found a bug which did not show up in my unit tests due to a misunderstanding of the JUnit api, found -> !found.

\$\endgroup\$
  • \$\begingroup\$ Could you post your tests as well? It might be beneficial to code review the tests as well. I don't think you're testing properly \$\endgroup\$ – dustytrash Sep 30 at 20:16
1
\$\begingroup\$

Since you mentioned unit tests, here are 2 unit tests that proves your code does not work (Based on name, age, email, phone header):

@Test
public void testFindIndicesThrowsException()
{
    ClassName x = new ClassName();

    try
    {
        int[] result = x.findIndices("Name", "age", "FieldThatDoesNotExist", "Phone");
        fail("expected exception was not thrown!");
    }
    catch (IllegalArgumentException exception)
    {
        assertEquals("One or more of input arguments could not be found in the headers.", exception.getMessage());
    }
}

@Test
public void testFindIndices()
{
    ClassName x = new ClassName();

    int[] result = x.findIndices("Email", "Phone", "Name", "age");
    assertEquals(4, result.length);
    assertEquals(2, result[0]);
    assertEquals(3, result[1]);
    assertEquals(0, result[2]);
    assertEquals(1, result[3]);
}

Explanation: You are setting a boolean in a loop. This means it could be set to false on one iteration, then set to true on the next.

Instead, you can set the index to the one found, and if it's not found, throw an error:

int k = 0;

for (String s : input) 
{
    indices[k] = -1;

    for (int i = 0; i < this.headers.length; i++)
    {
        if (s.equalsIgnoreCase(this.headers[i])) 
        {
            indices[k] = i;
        }
    }

    // If we never found the item
    if (indices[k] == -1)
    {
        throw new IllegalArgumentException(
            "One or more of input arguments could not be found in the headers.");
    }

    k++;
}
\$\endgroup\$
  • \$\begingroup\$ Thank you. My test had the failing header as last argument, which obviously then causes faulty behaviour. I have changed my code to this solution, but I will be going with the HashMap solution above. Thank you! \$\endgroup\$ – Edwin Carlsson Oct 1 at 5:15
1
\$\begingroup\$

Too me the biggest thing to look at in your code is the complexity. Right now it's O(n²). This is very inefficient and can be greatly improved upon, by taking a step back and changing the headers to a HashMap<String, Integer>. Now, since lookups for HashMap is O(1) you only need to count the loop through the input array, which will give a complexity of O(n). The code could look something like this:

HashMap<String, Integer> headers = new HashMap<>();

public int[] findIndices(String... input) throws IllegalArgumentException {
    int headerSize = headers.size();
    if (input.length > headerSize) {
        throw new IllegalArgumentException("Amount of searched for headers, " + input.length
                + ", exceeded the actual amount of headers: " + headerSize);
    }
    int[] indices = new int[input.length];
    int index = 0;
    for (String s : input) {
        if (headers.containsKey(s)) {
            indices[index++] = headers.get(s);
        } else {
            throw new IllegalArgumentException("One or more of input arguments could not be found in the headers.");
        }
    }
    return indices;
}
\$\endgroup\$
  • \$\begingroup\$ This is great - thank you. Of course, the HashMap is better suited for this particular problem. Is containsKey(s) O(1)? \$\endgroup\$ – Edwin Carlsson Oct 1 at 5:10
  • \$\begingroup\$ Yes, all the lookup functions are O(1). \$\endgroup\$ – tinstaafl Oct 1 at 6:44
  • \$\begingroup\$ Careful, this changes it to be case sensitive -- though it's easily fixed. \$\endgroup\$ – Jeremy Hunt Oct 1 at 22:51
0
\$\begingroup\$
    if (input.length > this.headers.length) {
      throw new IllegalArgumentException("Amount of searched for headers, " + input.length
      + ", exceeded the actual amount of headers: " + this.headers.length);
    }

Why is this needed? At the moment I get [0,1,0] for findIndices("name", "age", "name"), but IllegalArgumentException when I call findIndices("name", "age", "name", "email", "phone").

\$\endgroup\$

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.