5
\$\begingroup\$

Requirements were to create a constructor for a complex number, that receives a string. For example: \$3+5i\$. The regex extracts positive or negative numbers from the string with optional decimal.

My professor told me that my regex was "beyond wrong":

public Complex(String str) {


        ArrayList<Double> list = new 
            ArrayList<Double>();

        Pattern p = Pattern.compile("[-+]?[0-9]*\\.?[0-9]");
        // toString() handles the letter i

        // find positive and negative doubles in string and add to list
        Matcher m = p.matcher(str);
        while (m.find()) {
            double num = Double.parseDouble(m.group());
            list.add(num);
        }
        this.re = list.get(0);
        this.im = list.get(1);
}

Here is a link to the class if you need a better understanding of the code.

He then posted his chosen correct solution, which I found to be repetitive. He may argue that I am not checking for i, but I see no point of storing i if we are not using it later.

In addition, if he argues that I am not checking for and storing i, that would mean that if I did store them, I would explicitly have to go exclude the i from the string when I perform mathematical operations on the numbers. It seems counter-productive.

Here is the solution he preferred:

public Complex(String c) {

      String numberNoWhiteSpace = c.replaceAll("\\s","");

      // Matches complex number with BOTH real AND imaginary parts.  
      // Ex: -3-2.0i
      Pattern patternA = Pattern.compile("([-]?[0-9]+\\.?[0-9]?)([-|+]+[0-9]+\\.?[0-9]?)[i$]+");

      // Matches ONLY real number.
      // Ex: 3.145
      Pattern patternB = Pattern.compile("([-]?[0-9]+\\.?[0-9]?)$");

      // Matches ONLY imaginary number.
      // Ex: -10i
      Pattern patternC = Pattern.compile("([-]?[0-9]+\\.?[0-9]?)[i$]");

      Matcher matcherA = patternA.matcher(numberNoWhiteSpace);
      Matcher matcherB = patternB.matcher(numberNoWhiteSpace);
      Matcher matcherC = patternC.matcher(numberNoWhiteSpace);

      if (matcherA.find()) {
          real = Double.parseDouble(matcherA.group(1));
          imaginary = Double.parseDouble(matcherA.group(2));
      } else if (matcherB.find()) {
          real = Double.parseDouble(matcherB.group(1));
          imaginary = 0;
      } else if (matcherC.find()) {
          real = 0;
          imaginary = Double.parseDouble(matcherC.group(1));
      }   
}
\$\endgroup\$
5
\$\begingroup\$

Both of your solutions are wrong, in different ways. Yours is worse, though, because it fails in a way that can actually succeed in giving completely wrong answers.

Your solution

Your technique is, as your professor noted, sloppy. It falls apart in several cases that I would consider reasonable inputs:

  • Pure imaginary numbers such as "5i"
  • No imaginary component, such as "1"
  • No imaginary coefficient, such as "5+i"
  • Imaginary number with a space, such as "3 - 3i"
  • Decimals more precise than one-tenth, such as "3.14"

Furthermore, using an ArrayList seems extravagant. You shouldn't need one.

Professor's solution1

Sanitizing the input by discarding spaces — is that a good idea? Why? Is there another way to handle spaces?

Why three Patterns, three Matchers, and three .find()s? Can it be done with fewer?

Within each pattern, is your regex more correct, or his? Can you spot any bugs in his character classes (i.e. the stuff inside [square brackets])?

Are there any inputs that also fail with his code? Does his code accept any inputs that should be considered invalid?


1 We shouldn't really be criticizing code that you didn't write, so I'll give you some questions to ponder instead.

\$\endgroup\$
  • \$\begingroup\$ Don't forget about scientific notation, like 5e1 or 1.3e-5. \$\endgroup\$ – Ismael Miguel Mar 3 '16 at 9:44
1
\$\begingroup\$
        ArrayList<Double> list = new 
            ArrayList<Double>();

I'm not sure why this is on two lines.

Also, if you are on the latest Java, you don't need the second Double. Let the compiler figure it out for you. This also makes it easier to change to a different type later (e.g. BigDecimal).

    List<Double> list = new ArrayList<>();

Finally, you usually don't define variables as an implementation type but as the interface. This makes it much easier if you choose to change implementations in the future.

Note: I agree with the previous comment that you don't need a collection here. Consider the following code:

        while (m.find()) {
            double num = Double.parseDouble(m.group());
            list.add(num);
        }
        this.re = list.get(0);
        this.im = list.get(1);

Why not just say

        if (m.find()) {
            real = Double.parseDouble(m.group());
            imaginary = m.find() ? Double.parseDouble(m.group()) : 0.0;
        }

You replace six lines of code with four, although one is more complicated than it had been. You save use of the List entirely. You handle something with a real part but no imaginary part better. It still doesn't handle numbers with only an imaginary part or invalid input with two imaginary parts. Nor invalid input with three or more parts. But while you processed more parts, you were throwing away everything after the first two.

Note that I changed your variable names to be longer and more descriptive. I also removed the unnecessary this.. General practice in java is to leave it off unless needed to disambiguate with a method parameter. E.g. this.name = name; is one of the few times that you'd see this. used with an object field.

\$\endgroup\$
  • \$\begingroup\$ A lot of people like to use some convention to distinguish between local and instance variables. Personally, I find the this.var convention a lot less obnoxious than _var or some Hungarian prefix like mVar — at least it doesn't pollute the name itself, and you can leave it off when you don't feel that such emphasis is necessary. \$\endgroup\$ – 200_success Mar 3 '16 at 7:51
  • \$\begingroup\$ It was on two lines because phone screen automatically adjusted, but your suggestions are spot on \$\endgroup\$ – rickyjoepr Mar 3 '16 at 11:33
0
\$\begingroup\$

I Prefer the second one. But there's still some suggestions for the second one:

  1. patternA/B/C should use better self-explanation names.
  2. PatternA/B/C should extract to static final fields.

BTY: what is the purpose of this code snappet? Used in real project? For regex excise? For coding excise?

\$\endgroup\$
  • \$\begingroup\$ For homework we made a gui program to perform operations on complex numbers. Here is a link to the class pastebin.com/ta1ihjQT. \$\endgroup\$ – rickyjoepr Mar 3 '16 at 5:31
  • 2
    \$\begingroup\$ So why not use Apache Commons Math lib, which include Complex number support include parsing. So similar libs. \$\endgroup\$ – Colin Mar 3 '16 at 5:36

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.