3
\$\begingroup\$

I made this nice bit of code that I can insert into any of my projects requiring user input. It seems quite long for what it does and I would love some insight as to how I can do this more efficiently i.e. fewer lines of code.

I love to make my code elegant, but I don't see how to do this any better.

String x = (JOptionPane.showInputDialog(". Each parameter should be separated with a ',' and without spaces. (eg. 1,2,3)") + ",");
int w = 0, a = 0, y = -1;
for(int i = 0; i < x.length(); i++)
{
    if(x.charAt(i) == ',')
    {
        w++;
    }
}
Double[] z = new Double[w];
for(int i = 0; i < x.length(); i++)
{
    if(x.charAt(i) == ',')
    {
        z[a] = Double.parseDouble(x.substring((y + 1), i));
        y = i;
        a++;
    }
}
\$\endgroup\$
3
\$\begingroup\$
  • There is absolutely no need to put parentheses around the first statement (small improvement, but it improves readability). "parameter" is also misspelled (improving readability but in a different way).

    String x = JOptionPane.showInputDialog(". Each parameter should be separated with a ',' and without spaces. (eg. 1,2,3)") + ",";
    
  • There is no need to use Double[] when you can use double[]. The difference is, simply put, that a Double can be null while a double can not.

  • In the rest of your code it seems like you are reading all the doubles from the String (separated by commas), which can be done much easier like this:

    String[] splitted = x.split(",");
    double[] result = new double[splitted.length];
    for (int i = 0; i < splitted.length; i++) {
        result[i] = Double.parseDouble(splitted[i]);
    }
    

    The key here is to use the split-method on the String, which does most of the work for you. For example, it splits the string "0.37,42,21.4" into a string array with the three elements "0.37", "42" and "21.4". The rest of the code loops through that array and converts from String to double.

\$\endgroup\$
2
\$\begingroup\$

Regular expressions are designed to make this sort of parsing quite easy. For example, your code could look like:

String response = JOptionPane.showInputDialog("Each parameter should be separated with a ',' (eg. 1,2,3) : ");
String[] parts = responses.split("\\s*,\\s*", -1); // split on commas with optional whitespace.

Double[] values = new Double[parts.length];
for (int i = 0; i < parts.length; i++) {
    if (values[i].length() > 0) {
        // parse places with actual values, empty fields will be treated as 0.0
        values[i] = Double.parseDouble(parts[i]);
    }
}

This is one way to do it, but the parsing could just as easily be done with a StringTokenizer (and some people prefer this method for a variety of reasons even though it is 'Discouraged' in the JavaDoc).

I quite like the simpler process of using String.substring(...). With a simple token system (like a comma) it is easy:

    ArrayList<Double> input = new ArrayList<Double>();
    int current = 0;
    do {
        int next = response.indexOf(',', current);
        if (next < 0) {
            next = response.length();
        }
        String val = response.substring(current, next).trim();
        if (val.length() > 0) {
            input.add(Double.parseDouble(val));
        }
        current = next + 1;
    } while (current < response.length());
    Double[] values = input.toArray(new Double[input.size()]);
    System.out.println(Arrays.toString(values));

Regardless of which system you use, you should choose the one you understand best.

As for the example code you posted, I think the nit-pick I have is that you do two scans through the input. One to count the sections, and the other to parse them. The second small nitpick is the use of the y variable which is initialized to -1. Really, the y variable is the 'start-of-word' variable, and the first character is 0. So, initialize y to 0, and then when you do the substring you can just use plain y and not y+1. Then, you set y to being the start of the next word, which is i+1. This is a small criticisim though.

There are two larger issues I have with your code....

Firstly, it took me a while to figure out that without the + "," at the end of the showInputDialog is a critical component to your loops.... without it you miss the last value. This is a problem because such an important thing is likely to be missed. I think the algorithms above are better for the reason that there is no need for this 'hack'. But, even if you need this hack, you are not documenting it well enough. It should rather be done like:

String x = JOptionPane.showInputDialog(". Each patamater should be separated with a ',' and without spaces. (eg. 1,2,3)");
x = x + ","; // this is needed so that we find the last value in the result.

Secondly, your variable names are challenging to understand. I think the following variables should be renamed:

x -> userinput
w -> words
a -> nextpos
y -> wordstart
z -> values

With the above variable changes (and the change to the indexing of y or wordstart, your code is much more readable:

    String userinput = JOptionPane.showInputDialog(". Each parameter should be separated with a ',' and without spaces. (eg. 1,2,3)");
    userinput += ","; // needed to make the loops capture the last value on the line.
    int wordcount = 0, nextpos = 0, wordstart = 0;
    for(int i = 0; i < userinput.length(); i++)
    {
        if(userinput.charAt(i) == ',')
        {
            wordcount++;
        }
    }
    Double[] values = new Double[wordcount];
    for(int i = 0; i < userinput.length(); i++)
    {
        if(userinput.charAt(i) == ',')
        {
            values[nextpos] = Double.parseDouble(userinput.substring(wordstart, i));
            wordstart = i + 1;
            nextpos++;
        }
    }

EDIT: I thought I would point out that there's a way to avoid the 'hack' with the extra comma, even using your mechanism. First, I start with wordcount == 1 (there's always at least one word, plus another word for each comma). Then, I use <= userinput.length() instead of < userinput.length().... and I use the additional check in the if (i == userinput.length() || ',' == userinput.charAt(i)) {...

    String userinput = JOptionPane.showInputDialog(". Each patamater should be separated with a ',' and without spaces. (eg. 1,2,3)");
    int wordcount = 1, nextpos = 0, wordstart = 0;
    for(int i = 0; i < userinput.length(); i++)
    {
        if(userinput.charAt(i) == ',')
        {
            wordcount++;
        }
    }
    Double[] values = new Double[wordcount];
    for(int i = 0; i <= userinput.length(); i++)
    {
        if(i == userinput.length() || userinput.charAt(i) == ',')
        {
            values[nextpos] = Double.parseDouble(userinput.substring(wordstart, i));
            wordstart = i + 1;
            nextpos++;
        }
    }
\$\endgroup\$
  • \$\begingroup\$ I think regex should be avoided for splitting strings, regex could be used to validate the input though. And of course it could be helpful to split with it to allow for extra whitespace. I personally think that all alternatives here except for the one using split are very hard to get a quick understanding of, but then again we all have our preferences :) \$\endgroup\$ – Simon Forsberg Nov 8 '13 at 22:21

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.