Postfix calculator in Java

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.

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.

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);
    }
}