6
\$\begingroup\$

I made a postfix calculator in Java.

The code performs the functions I planned without problems, However, I'm not satisfied with it, because it's too verbose and I couldn't split it up into methods properly, especially PostfixCalculator.changeNotation().

Any help would be greatly appreciated.


Main.java


public class Main 
{
    public static void main(String[] args) 
    {
        PostfixCalculator postCalc = new PostfixCalculator();
    }
}

Stack.java (I didn't use the library because I'm studying data structures.)

import java.util.ArrayList;
import java.util.Scanner;

public class Stack 
{
    Scanner scan = null;
    public ArrayList<String> stack;

    public Stack() 
    {
        scan = new Scanner(System.in);
        stack = new ArrayList<String>();
    }

    public boolean isEmpty() 
    {
        if (stack.size() == 0) 
        {
            return true;
        } 
        else 
        {
            return false;
        } 
    }

    public void push(String element) 
    {
        stack.add(element);
    }

    public void pop() 
    {   
        if(stack.isEmpty()) 
        {
            return;
        }
        else 
        {
            stack.remove(stack.size()-1);
        }
    }

    public int size() 
    {
        return stack.size();
    }

    public String peek() 
    {
        if (stack.isEmpty()) 
        {
            return null;
        } 
        else 
        {
            return stack.get(stack.size() - 1); 
        }
    }
}

PostfixCalculator.java

import java.util.ArrayList;
import java.util.Scanner;

public class PostfixCalculator 
{
    String[] infixNotation = null;

    String postfixNotation = "";
    ArrayList<String> aListPostfixNotation = null;

    Scanner scan = null;
    Stack stk = null;

    public PostfixCalculator() 
    {
        aListPostfixNotation = new ArrayList<String>();  
        stk = new Stack();
        scan = new Scanner(System.in);
        String myInput = scan.nextLine();
        infixNotation = myInput.split("",0);

        postfixCalculate(changeNotation());
    }

    public String changeNotation() // Change the entered code from infixnotation to postfixnotation 
    {
        for (int i = 0; i < infixNotation.length; i++) 
        {   
            if(infixNotation[i].equals(" "))  
            {
                continue;
            }
            else if( !infixNotation[i].equals("+") &&  // when infixNotation[i] is operand
                    !infixNotation[i].equals("-") &&
                    !infixNotation[i].equals("*") &&
                    !infixNotation[i].equals("/") &&
                    !infixNotation[i].equals("(") &&
                    !infixNotation[i].equals(")") )   
            {
                postfixNotation = postfixNotation + infixNotation[i];
            }
            else  // when infixNotation[i] is operator or bracket
            {
                if(stk.isEmpty())  // It can push()ed, when stack is empty    // Don't care ")~~~", because the ")" can't be first char  
                {
                    stk.push(infixNotation[i]);
                }
                else 
                {
                    if (infixNotation[i].equals("("))   // Just push() when it's '('
                    {
                        stk.push(infixNotation[i]);
                    }
                    else if (infixNotation[i].equals(")")) // When bracket is balanced (left and right), pop() a pair of bracket
                    {
                        stk.push(infixNotation[i]);
                        whenRightPush();
                    }
                    else  // Have to the priority, when infixNotation[i] is operator 
                    {
                        if (stk.peek().equals("("))  
                        {   
                            stk.push(infixNotation[i]);
                        }
                        else 
                        {
                            if ( infixNotation[i].equals("*") || infixNotation[i].equals("/") )  
                            {
                                if ( stk.peek().equals("+") || stk.peek().equals("-") )
                                {
                                    stk.push(infixNotation[i]);
                                }
                                else if ( stk.peek().equals("*") || stk.peek().equals("/") )
                                {
                                    postfixNotation = postfixNotation + stk.peek();
                                    stk.pop();
                                    stk.push(infixNotation[i]);
                                }
                            }
                            else if ( infixNotation[i].equals("+") || infixNotation[i].equals("-") )
                            {
                                if ( stk.peek().equals("+") || stk.peek().equals("-")) 
                                    // Equal level's operators can't enter the stack twice sequentially, 
                                    // so they need to be considered only once.
                                {
                                    postfixNotation = postfixNotation + stk.peek();
                                    stk.pop();
                                    stk.push(infixNotation[i]);
                                }
                                else if ( stk.peek().equals("*") || stk.peek().equals("/") )
                                {
                                    postfixNotation = postfixNotation + stk.peek(); 
                                    stk.pop();  

                                    if ( stk.peek().equals("+") || stk.peek().equals("-") )  // ex + * -
                                    {
                                        postfixNotation = postfixNotation + stk.peek(); 
                                        stk.pop();
                                    }
                                    stk.push(infixNotation[i]);
                                }
                            }
                        }
                    }
                }
            }

            if (i == infixNotation.length-1)  // All elements is pop()ed, when 'i' have last value
            {
                while(!stk.isEmpty())  
                {
                    if(stk.peek().equals("("))  
                    {
                        stk.pop();
                    }
                    else if (stk.peek().equals(")"))  
                    {
                        stk.pop();
                    }
                    else 
                    {
                        postfixNotation = postfixNotation + stk.peek();
                        stk.pop();
                    }
                }
            }
        }

        System.out.println(postfixNotation);
        return postfixNotation; 
    }

    public void whenRightPush()  
    // This method will work when ')' is push()ed // I can't find proper name for this method
    {
        stk.pop();  

        while(true)
        {
            if ( stk.peek().equals("(") )  
            {
                stk.pop();
                break;
            }
            else  // 연산자일 경우 후위표기식 문자열에 붙인 후 pop()
            {
                postfixNotation = postfixNotation + stk.peek();
                stk.pop();
            }
        }
    }


    public void postfixCalculate(String postNotation)   // Calculate the postfixnotation
    {  

        int operatorCount = 0;
        int resultTemp = 0;

        String[] arrayPostfixNotation = postNotation.split("", 0);  

        for (String str : arrayPostfixNotation)  
        {
            aListPostfixNotation.add(str);
        }

        for (String str : aListPostfixNotation)  
        {
            if (str.equals("+") || str.equals("-") || str.equals("*") || str.equals("/"))
            {
                operatorCount++;
            }
        }


        while(operatorCount > 0) {  

            for(int i = 0; i < aListPostfixNotation.size(); i++)  
            {
                if (aListPostfixNotation.get(i).equals("+") || 
                        aListPostfixNotation.get(i).equals("-") || 
                        aListPostfixNotation.get(i).equals("*") || 
                        aListPostfixNotation.get(i).equals("/"))
                {

                    if(aListPostfixNotation.get(i).equals("+"))
                    {
                        resultTemp = Integer.parseInt(aListPostfixNotation.get(i-2)) 
                                + Integer.parseInt(aListPostfixNotation.get(i-1));
                    }
                    else if(aListPostfixNotation.get(i).equals("-"))
                    {
                        resultTemp = Integer.parseInt(aListPostfixNotation.get(i-2)) 
                                - Integer.parseInt(aListPostfixNotation.get(i-1));
                    }
                    else if(aListPostfixNotation.get(i).equals("*")) 
                    {
                        resultTemp = Integer.parseInt(aListPostfixNotation.get(i-2)) 
                                * Integer.parseInt(aListPostfixNotation.get(i-1));
                    }
                    else
                    {
                        resultTemp = Integer.parseInt(aListPostfixNotation.get(i-2)) 
                                / Integer.parseInt(aListPostfixNotation.get(i-1));
                    }
                    aListPostfixNotation.remove(i-2);    // Remove the used operator and operand
                    aListPostfixNotation.remove(i-2);     
                    aListPostfixNotation.remove(i-2);    
                    aListPostfixNotation.add(i-2, Integer.toString(resultTemp));  // Add the result of operation into the arraylist

                    operatorCount--;  

                    break;

                }
            }
        }

        System.out.println(resultTemp);
    }
}

```
\$\endgroup\$
5
\$\begingroup\$

I didn't go over all the code. Here are my comments so far:

Stack

  1. The class contains a Scanner for no apparent reason.

  2. While it is your implementation, which does not implement the interface from the JDK, you should be aware that in the "standard" contract for Stack, (in Java as in other programming languages) the pop() method returns the removed item. (making peeking sometimes redundant)

  3. any reason why your Stack isn't generic? it's good practice...

PostfixCalculator

  1. This class violates the single responsibility principle. besides doing what it is supposed to do (all the arithmetic calculation), It also receives input from the user, parses it and outputs the result. so your calculator cannot be used in a web app or mobile device because it can only receive input and emit output from/to the console. Your calculator should receive and return a String (or possible return an int or float) and let the caller worry about how to get the input and where to display the output.

  2. The constructor - it does everything. in fact, while there are other public methods, the constructor is the only interface that is used by whoever uses this Calculator, as can be seen by Main. this design poses three problems: 1) it's counter-intuitive. 2) there is no way to reuse the same instance to do multiple calculations and 3) yet another violation of the single responsibility principle. a constructor should only set up the state (instance variables) of the instance. any operation or action should be done by different methods.

  3. avoid literals. the parenthesis and operators should be defined once. either as public static final constants or better yet, as enum. putting then in an enum has the advantage that you can assign "properties" to the values, like precedence.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot. Although I can't understand some parts of your advice, I will continue my studies based on your advice and try to understand your advice that I couldn't understand. Thank you. \$\endgroup\$ – Lim Hyungang May 20 at 5:27
2
\$\begingroup\$

Welcome to codereview.se and thanks for sharing your code.

Constructors

As Mentioned by @SharonBenAsher a constructor should only set up the state (instance variables) of the instance.

This implicitly excludes calling any methods on the arguments passed and the use of the new keyword. A constructor should only have assignments of parameters to instance variables.

The dependencies should be instantiates before calling the constructor. But this only applies for dependencies, that is: classes that provide additional logic. The class ArrayList does not apply to that. It is a data structure something to hold and organize data. But it should be created at the variable declaration:

public class Stack 
{
    Scanner scan;
    public ArrayList<String> stack = new ArrayList<String>();

    public Stack(Scanner scan) 
    {
        this.scan = scan; // I also doubt that the scanner is needed here at all.
    }

When following the other answers suggestions (extract user IO out of this class, explicitly call the method doing the logic) both of your classes en up with default constructors (having no arguments and doing nothing) which you don't even need to write at all.

visibility scopes

Most of your instance variables have no visibility key word which makes them package private (that is: accessible by other classes in same package) or are declared to be public. This violates the information hiding/encapsulation paradigm of object oriented programming. Always restrict the visibility to the least necessary scope. For instance variables should almost ever be declared private (and there should be no getter/setter).

The same applies to methods. A class should have one public method as entry point for a service. This methods may call other methods in the same class but this other methods should be private.

avoid state

Your PostfixCalculator uses an instance variable (infixNotation) to accumulate the result. This is called a mutable state. Mutable state limits the re-usability of an object. That is: each time you need an object of this class you have to create a new instance (using the new operator) instead of passing an existing instance around.

do not initialize with null

You initialize your instance variables with null. This prevents you from using the final key word on them which would indicate to you (and the compiler) that this variable will never change (and therefore) never be null which in turn prevents you from doing Null Checks all over the code.

code duplication

Your code is quite "algorithm driven". That resulted in lots of code duplication like this:

while(!stk.isEmpty())  
{
    if(stk.peek().equals("(")) 
    {
        stk.pop();
    }
    else if (stk.peek().equals(")")) 
    {
        stk.pop();
    }
    else 
    {
        postfixNotation = postfixNotation + stk.peek();
        stk.pop();
    }
}

A little more "OO-ish" approach could look like this:

while(!stk.isEmpty()) 
{
    if(!Arrays.asList( "(" , ")" ).contains(stk.peek())
    {
        postfixNotation = postfixNotation + stk.peek();
    }
    stk.pop();
}

Since Java after all is an object oriented you should start looking for oo-Approaches to your problems.

Naming

Finding good names is the hardest part in programming. So always take your time to think carefully of your identifier names.

Choose your names from the problem domain

You have some identifiers which are named after their technical implementation like this:

 Scanner scan = null;
 Stack stk = null;

They should have names that reveal their task within your application.

Scanner userInput
Stack notationElements

Avoid abbreviations

In your code you use some abbreviations such as stk. Although this abbreviation makes sense to you (now) anyone reading your code being not familiar with the problem (like me) has a hard time finding out what this means. Especially for me as a German native the abbreviation Stk stands for Stück meaning piece or item.

If you do this to save typing work: remember that you way more often read your code than actually typing something. Also for Java you have good IDE support with code completion so that you most likely type a long identifier only once and later on select it from the IDEs code completion proposals.

\$\endgroup\$
  • \$\begingroup\$ Wow, Thanks a lot. Because my English is not good, I am having difficulty in expressing gratitude in various ways :(.. I want you to know that I feel really really gratitude, although there are many duplicate expressions in way of my gratitude. Thank you! \$\endgroup\$ – Lim Hyungang May 20 at 7:10
-1
\$\begingroup\$

Here are a few potential things that could be changed:

Stack.java

The isEmpty() method can be simplified, since your underlying data structure that you are using to implement a stack also exposes this method:

public boolean isEmpty()
{
    return stack.isEmpty();
}

In Java, pop() generally returns the element that was popped, while here it simply removes the element. This necessitates a peek() + pop() combination, where the convention is just pop(). There is nothing really wrong with your approach, so this is just a matter of convention.

One thing though, pop() simply returns when the stack is empty - so there is no direct indication that the operation failed. That is, if the stack is misused, it does not directly indicate it, either using some kind of Exception or by returning false on failure. Generally, pop() "convention" in Java would throw an Exception on an empty stack.

public String pop() throws StackException
{
    if(isEmpty())
        throw new StackException("pop on empty stack");

    return stack.remove(stack.size() - 1);
}

Main.java and PostfixCalculator.java

Moving the I/O work from PostfixCalculator class to your Main class. Ideally, the PostfixCalculator should only perform calculations and postfix related operations - getting the data to pass to it should be done elsewhere.

Therefore, the changeNotation() method could be modified to take in a String as input, rather than obtaining it itself, which ties the class to a particular input method. Deciding to read from a file or from the network rather than Standard Input should not trigger changes to the PostfixCalculator.

public String changeNotation(String infix) { ... }

In addition, it is also possible to make these methods static, since you don't really need to save any state: the calculator simply takes in an input and returns an output.

Within changeNotation():

for(int i = 0; i < infixNotation.length; i++)
{
   ...
}

A local variable can be introduced, since there are quite a lot of references to infixNotation[i] within the loop body, so this can be cleaned up.

There is one point where 'i' is referenced in the code, so converting to a for-each loop would require maintaining a counter, which is why just introducing a local variable would be better here.

for(int i = 0; i < infixNotation.length; i++)
{
    /* 's' could be changed to something more representative */
    String s = infixNotation[i];

    if(s.equals(" "))
        continue;

    ...
}

There is also quite a bit of if-else nesting, which makes the code a bit harder to follow:

for(int i = 0; i < infixNotation.length; i++)
{
    if(infixNotation[i].equals(" "))
    {
        continue;
    }
    else if(!infixNotation[i].equals("+") && /* other conditions here */)
    {
        ...
    }
    else { ... } 

    ...
}

The else if isn't needed here, since the if-body's control flow will go back to the loop header check and cannot fallthrough. Therefore, it's fine to have that else if just be an if.

Towards the end of the method:

System.out.println(postfixNotation);
return postfixNotation; 

I'm not sure whether the calculator should print to standard out, or do any I/O for that matter. Just return the result, and let the caller (in this case, main) to do what it wants, e.g. print to standard out, log to a file, etc.

As stated before, the I/O work should probably be moved to main.

public class Main
{
    public static void main(String[] args)
    {
         /* 
          * Obviously, the user-interaction (printing a prompt, etc.) presented
          * here can be greatly improved upon :)
          */
         Scanner scan = new Scanner(System.in);
         String infix = scan.nextLine();

         PostfixCalculator calc = new PostfixCalculator();
         String postfix = calc.changeNotation(infix);
         System.out.println(postfix);

         /* or if using static methods */
         String postfix = PostfixCalculator.changeNotation(infix);
         System.out.println(postfix);
    }
}

\$\endgroup\$
  • \$\begingroup\$ most of this is a duplicate of my answer \$\endgroup\$ – Sharon Ben Asher May 20 at 5:29
  • \$\begingroup\$ I really appreciate your advice. I'll try to understand and implement based on your codes, although I can't understand right now. I guess It'll be great help to improve me. Thank you :) \$\endgroup\$ – Lim Hyungang May 20 at 5:31
  • \$\begingroup\$ Oh, really? I'll analyze each answer in detail as i understand. Although each answer is similar, I really thank you all of you! \$\endgroup\$ – Lim Hyungang May 20 at 5:35

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.