Problem
I’m going to job interviews soon. How I can improve my coding style and write better code?
package com.amoghantarkar.Stack;
import java.util.*;
class ArrayStack{
private int capacity;
private int top;
private int[] array;
public ArrayStack(int capacity){
top = -1;
this.capacity = capacity;
array = new int[capacity];
}
public boolean isEmpty(){
return (top == -1);
}
public boolean isFull(){
return (top == (capacity-1));
}
public void push(int data){
if(isFull()){
System.out.println("Stack Overflow");
}
else{
array[++top] = data;
}
}
public void pop(){
if(isEmpty()){
System.out.println("Stack Underflow");
}
else{
System.out.println(array[top--]);
}
}
public void deleteStack(){
top = -1;
}
public void printTop(){
if(!isEmpty()){
System.out.println(array[top]);
}
else{
System.out.println("Stack is empty");
}
}
}
public class StackImplementation{
public static void main(String[] args){
ArrayStack testStack1 = new ArrayStack(10);
Scanner in = new Scanner(System.in);
while(true){
System.out.println("Stack Operations");
System.out.println(" 1.isEmptyt 2.isFullt 3. pusht 4.popt 5.printStackt 6.Delete Stack");
int ch = in.nextInt();
switch(ch){
case 1:
boolean ans = testStack1.isEmpty();
System.out.println(ans);
break;
case 2:
ans = testStack1.isFull();
System.out.println(ans);
break;
case 3:
System.out.println("Enter data to push");
int data = in.nextInt();
testStack1.push(data);
System.out.println("Pushed data");
break;
case 4:
testStack1.pop();
System.out.println("Popped data");
break;
case 5:testStack1.printTop();
break;
case 6:testStack1.deleteStack();
break;
default: System.out.println("Enter valid number choice");
break;
}System.out.println("want to continue");
String str = in.next();
if(!str.equals("y")){
break;
}
}
}
}
Solution
private int capacity;
private int top;
private int[] array;
Both capacity and array never change after construction, so they can be final. You only need to leave them variable if you are going to implement a resize method.
final private int[] stack;
final private int capacity;
private int top;
As a general rule, try to avoid names that just restate what the variable is. Try to pick names that explain how the variable will be used. In this case, the variable is to hold stack information, so that’s a better name than array.
It’s also worth noting that I’d expect something called an ArrayStack to handle generic data. Yours only handles int values. You may want to put that into the name or change it to use generics. The argument against generics is that primitive types may be more efficient than object types. If efficiency were really important to you, you probably would be managing the array directly rather than abstracting it.
if(isFull()){
System.out.println("Stack Overflow");
}
else{
array[++top] = data;
}
Rather than print output to the screen, you should probably throw an exception in this case. An exception will force the caller to either handle the exception or halt the program.
if (isFull()) {
throw new IndexOutOfBoundsException("Stack Overflow");
}
array[++top] = data;
Even better, you could define your own StackFullException. This would allow you to better control the handling.
Note that we can do away with the else if we throw the exception. It will end execution of the method for you.
Some may suggest moving the ++top to its own line. That improves readability but lengthens the code. You may want to try to learn what is important to this organization before writing code for them.
public void pop(){
The pop operation normally returns a value.
public int pop() {
This leaves it up to the caller what to do with it. A stack that can only print its contents is not the most useful.
public void printTop(){
This method would normally be called peek and return a value.
public int peek() throws IndexOutOfBoundsException {
Again, I think that this should throw an exception if called on an empty stack.
case 4:
testStack1.pop();
System.out.println("Popped data");
break;
case 5:testStack1.printTop();
break;
You should only change indentation formats very rarely. There doesn’t seem reason enough to do so here. The case 4 format is closer to what I’ve seen elsewhere. I’d suggest using it for all of them.
}System.out.println("want to continue");
First, never put unrelated commands after a closing }. The only statements that should appear on the same line as a } are things like else and catch which are directly related to the block.
Second, when writing for a potential employer, always capitalize and punctuate correctly. E.g. "Want to continue?".
Third, if you are going to accept only one single valid response, say what it is. Perhaps, "Press y to continue."
Note that your code won’t accept Y or yes as responses and it won’t be evident to a user why not.
One of the biggest problems that I note with this code is that it looks like it is intended to be run by a developer. You will never write code like that for an employer (unless you are a systems analyst or similar who only writes code that you run yourself). Any employer who asks you to write code will be expecting you to write code that will be used by non-developers. You don’t tell them things like “Stack Overflow” which they won’t understand. Nor should they have to read the code to understand the response that you want.
And yes, I realize that a developer actually will be running your code in this case. But a good evaluator should mark you down for not writing realistic code.