1
\$\begingroup\$

I'm looking for some constructive (harsh) criticism of a homework I've done (the code works fine). Also Is there a better/another way to solve the exercise?

Exercise:

Read an arithmetic expression grouped by parentheses and verify if syntactically it is well formed (Using stacks).

To consider:

  1. the parentheses close and open correctly, there are pairs of them.

  2. Letters should be used as literals (uppercase or lowercase).

  3. Review the proper use of operators (requiring two operands).

  4. The parentheses open and close properly the expressions: (a + b) and not (a +) b.

public class Hmwrk {

    public static void main(String[] args) {

        Scanner x = new Scanner(System.in);
        System.out.println("Give the size of your arithmetic expression");
        int n = x.nextInt();

        LinkedList pila = new LinkedList();
        LinkedList pila1 = new LinkedList();
        LinkedList pila2 = new LinkedList();
        LinkedList pila3 = new LinkedList();
        LinkedList pila4 = new LinkedList();
        LinkedList pila5 = new LinkedList();
        LinkedList pp = new LinkedList();
        LinkedList ppo = new LinkedList();
        LinkedList pilaC = new LinkedList();

        x.nextLine();
        for (int i = 0; i < n; i++) {
            System.out.println("Introduce an element");

            String e = x.nextLine();
            pila.push(e);
            pila1.push(e);
            pila2.push(e);
            pila3.push(e);
            pila4.push(e);
            pila5.push(e);

        }

        int a = 0, b = 0;

        if (pila1.size() == 1) {
            String u = (String) pila1.pop();
            if (0 == u.compareTo("+") || 0 == u.compareTo("-") || 0 == u.compareTo("*") || 0 == u.compareTo("/")
                    || 0 == u.compareTo("(") || 0 == u.compareTo(")")) {
                System.out.println("incorrect expression");
            }
        } else if (pila5.size() == 2) {
            String xx = (String) pila5.pop();
            String yy = (String) pila5.pop();
            if (0 == xx.compareTo("+") || 0 == xx.compareTo("-") || 0 == xx.compareTo("*") || 0 == xx.compareTo("/")
                    || 0 == yy.compareTo("+") || 0 == yy.compareTo("-") || 0 == yy.compareTo("*") || 0 == yy.compareTo("/")) {
                System.out.println("incorrect expression");
            }
        } else {

            while (!pila.isEmpty()) {
                String element = (String) pila.pop();

                if (0 == element.compareTo(")") && !pila.isEmpty()) {
                    String sim = (String) pila.pop();
                    if (0 == sim.compareTo("+") || 0 == sim.compareTo("-") || 0 == sim.compareTo("*") || 0 == sim.compareTo("/")) {
                        a++;
                    }
                }
            }

            if (a != 0) {
                System.out.println("incorrect expression");
            } else {

                while (!pila2.isEmpty()) {
                    String simbolo = (String) pila2.pop();
                    if ((0 == simbolo.compareTo("+") || 0 == simbolo.compareTo("-") || 0 == simbolo.compareTo("*") || 0 == simbolo.compareTo("/")) && !pila2.isEmpty()) {
                        String el = (String) pila2.pop();
                        if (0 == el.compareTo("(") || 0 == el.compareTo("+") || 0 == el.compareTo("-") || 0 == el.compareTo("*") || 0 == el.compareTo("/")) {
                            b++;
                        }
                    }
                }

                if (b != 0) {
                    System.out.println("incorrect expression");
                } else { 
                    String w = null;
                    int c = 0, d = 0, k = 0;
                    while (!pila4.isEmpty()) {
                        String v = (String) pila4.pop();
                        if (0 == v.compareTo("(") && !pila4.isEmpty()) {
                            w = (String) pila4.pop();
                            if (0 == w.compareTo("+") || 0 == w.compareTo("-") || 0 == w.compareTo("*") || 0 == w.compareTo("/") || 0 == w.compareTo("(")) {
                                c++;
                            } else {
                                k++; 
                            }
                        }

                    }

                    if (k != 0) {
                        System.out.println("incorrect expression");
                    } else {

                        while (!pila3.isEmpty()) {
                            String e = (String) pila3.pop();
                            if (0 == e.compareTo("(") || 0 == e.compareTo(")")) {
                                pp.push(e);
                            }
                        }
                        while (!pp.isEmpty()) {
                            ppo.push(pp.pop());
                        }

                        int ww = 0;
                        while (!ppo.isEmpty() && ww != 1) {
                            String s = (String) ppo.pop();
                            if (0 == s.compareTo("(") && !pilaC.isEmpty()) {
                                pilaC.pop();
                            } else if (0 == s.compareTo("(") && pilaC.isEmpty()) {
                                System.out.println("incorrect expression");
                                ww = 1;
                            } else {
                                pilaC.push(s);
                            }
                        }
                        if (ww == 1)
                        {
                            System.out.println();
                        } else if (!pilaC.isEmpty()) {
                            System.out.println("incorrect expression");
                        } else {
                            System.out.println("correct expression");
                        }

                    }

                }
            }

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

Your main method should be much shorter. Ideally only one or two lines:

public static void main(String []args) {
    Scanner in = new Scanner(System.in);
    System.out.println(isWellformedExpression(in.nextLine()));
}

Nothing more. All the rest of your code should go into a separate method. This has several benefits:

  • That method is simpler since it doesn't have anything to do with input/output.
  • That method has a name describing what it does.
  • That method can be called in some testing code. You wouldn't want to set up a testing environment for the main method since providing System.in and System.out is quite difficult.

The testing code could look like this:

static void testIsWellformedExpression() {

    assertTrue(isWellformedExpression("1"));
    assertTrue(isWellformedExpression("1+2"));
    assertTrue(isWellformedExpression("-5"));
    assertTrue(isWellformedExpression("((((((((1))))))))"));

    assertFalse(isWellformedExpression(""));
    assertFalse(isWellformedExpression("+"));
    assertFalse(isWellformedExpression("1+"));
    assertFalse(isWellformedExpression(")"));
}

You would have to define the methods assertTrue and assertFalse yourself, or better yet add JUnit (a testing framework) to your project.


The expression 0 == x.compareTo("+") looks strange, as if it came from another programming language (C) and from outer space (because of the Yoda style). In plain English you wouldn't say “if zero is the same as …”, but you would say “if … is zero” instead. So should your code. You can just say x.equals("+"), which is more to the point.

I have no idea why you need 6 different lists. One should be enough.

The names you chose for these variables are terrible since they tell the reader nothing. They are not even in English, so most of this site's readers won't understand them at all. What is pila3? What is ww? Think of better names for them. What words would you use to describe their purpose to a human?

Instead of using a raw LinkedList, you should rather use a LinkedList of strings, which is written as LinkedList<String> in Java. Then you don't need these (String) type casts whenever you take something out of the list.

\$\endgroup\$
  • \$\begingroup\$ "I have no idea why you need 6 different lists. One should be enough." how ? \$\endgroup\$ – user178403 Sep 10 '17 at 18:19
  • 1
    \$\begingroup\$ @Michelle Probably, we all don't understand what you do with the lists - and that's because their names don't tell us. \$\endgroup\$ – Ralf Kleberhoff Sep 10 '17 at 19:22
  • \$\begingroup\$ @RalfKleberhoff really? Have you tested the code (I guess no and that's why you don't understand what do I do with the lists)? They are all have an stack behaviour btw. \$\endgroup\$ – user178403 Sep 10 '17 at 19:32
  • \$\begingroup\$ @RalfKleberhoff I suppose Roland understand the behaviour of the code and that's why he said that only one list would be enough \$\endgroup\$ – user178403 Sep 10 '17 at 19:34
  • \$\begingroup\$ No, I don't understand the code. And I didn't care, exactly like you said, because the variables have so bad names. I also know of other solutions to this problem that only use a single stack, therefore I know the code can be simpler. \$\endgroup\$ – Roland Illig Sep 10 '17 at 21:07

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.