patternjavaMinor
Stack from Linked List
Viewed 0 times
listfromlinkedstack
Problem
I have written a program that takes a linked list and then converts it to a stack.
I would like a code review for my program. Thanks in advance.
Stack.java
LinkedList.java
```
/**
* Created by Ninan John J P on 2/3/2016.
*/
import java.util.Scanner;
public class LinkedList {
boolean createFlag=false;
int noOfInitialNodes;
Node head;
int count=0;
private void findElement() {
String toFind;
boolean found=false;
int index=0;
Scanner in = new Scanner(System.in);
System.out.println("Enter Number to Find");
toFind=in.next();
if(count" + currentNode.getData());
}
}
private void insertNew() {
Scanner in = new Scanner(System.in);
String data;
System.out.println("Enter Data for Node:");
data=in.next();
Node tempNode= new Node(data);
Node currentNode=head;
if(currentNode!=null){
while(currentNode.g
I would like a code review for my program. Thanks in advance.
Stack.java
/**
* Created by Ninan John J P on 2/8/2016.
*/
public class Stack {
int stackArray[];
int topOfStack=-1;
LinkedList source;
int stackSize;
public Stack(LinkedList l1) {
source=l1;
stackSize=l1.count;
stackArray= new int[stackSize];
}
private void put(int data) {
stackArray[++topOfStack]=data;
}
private int pop(){
return stackArray[topOfStack--];
}
public static void main(String...a){
LinkedList l1= new LinkedList();
l1.getInitialData();
l1.create();
Stack s1= new Stack(l1);
Node currentNode=l1.head;
s1.put(Integer.parseInt(currentNode.getData()));
while(currentNode.getNextNode()!=null) {
currentNode=currentNode.getNextNode();
s1.put(Integer.parseInt(currentNode.getData()));
}
System.out.println("The Stack is:");
while(s1.topOfStack>=0)
System.out.print(s1.pop() + "\t");
}
}LinkedList.java
```
/**
* Created by Ninan John J P on 2/3/2016.
*/
import java.util.Scanner;
public class LinkedList {
boolean createFlag=false;
int noOfInitialNodes;
Node head;
int count=0;
private void findElement() {
String toFind;
boolean found=false;
int index=0;
Scanner in = new Scanner(System.in);
System.out.println("Enter Number to Find");
toFind=in.next();
if(count" + currentNode.getData());
}
}
private void insertNew() {
Scanner in = new Scanner(System.in);
String data;
System.out.println("Enter Data for Node:");
data=in.next();
Node tempNode= new Node(data);
Node currentNode=head;
if(currentNode!=null){
while(currentNode.g
Solution
Mains in classes
I don't like having
Public interface
Think about what it is you're trying to model and what that means for the interface to your classes and what the classes are responsible for. When I think of a stack class, I think of something that is going to let me push objects onto it and pop them back off. So for me, a Stack would at minimum have public
Separation of Concerns
You linked list is asking the user to supply data, then using that to create nodes and add them to the list. This is too much responsibility for the one class and it makes it difficult for you to reuse the list in other ways. If you had a list builder that was responsible for driving the process of collecting input from the user in order to construct the list via it's public interface, this would lead to a more focused implementation of the list. This would for example allow you to reuse the list in order to construct the stack class. A linked list is an easy structure for implementing a stack. To achieve stack like behaviour, you always add new elements to the head of the list (push) and only allow the first element to be removed from the list (pop). You can't currently implement stack like this however because of the user interaction embedded in your linked list class.
Bounds Checking
Currently your push method on your stack doesn't check if it's going to overflow the internal array. This is OK at the moment, because the method is private, however as I've said it's odd for a stack class not to have public push/pop methods. If they do become public then they need to start doing bounds checking to prevent an overflow.
Only implement what you need
As far as I can tell, you don't use your
I don't like having
main methods in non-application classes, which is what you've done in both your linked list and stack classes. Having a main method in the class blurs the boundary of what the class is responsible for. At a minimum it's responsible for being a 'stack' and being an application entry point, it feels wrong.Public interface
Think about what it is you're trying to model and what that means for the interface to your classes and what the classes are responsible for. When I think of a stack class, I think of something that is going to let me push objects onto it and pop them back off. So for me, a Stack would at minimum have public
push and pop methods. Yours doesn't, the methods are private. I suspect this is because you're only using them from your main which is in the same class.Separation of Concerns
You linked list is asking the user to supply data, then using that to create nodes and add them to the list. This is too much responsibility for the one class and it makes it difficult for you to reuse the list in other ways. If you had a list builder that was responsible for driving the process of collecting input from the user in order to construct the list via it's public interface, this would lead to a more focused implementation of the list. This would for example allow you to reuse the list in order to construct the stack class. A linked list is an easy structure for implementing a stack. To achieve stack like behaviour, you always add new elements to the head of the list (push) and only allow the first element to be removed from the list (pop). You can't currently implement stack like this however because of the user interaction embedded in your linked list class.
Bounds Checking
Currently your push method on your stack doesn't check if it's going to overflow the internal array. This is OK at the moment, because the method is private, however as I've said it's odd for a stack class not to have public push/pop methods. If they do become public then they need to start doing bounds checking to prevent an overflow.
Only implement what you need
As far as I can tell, you don't use your
setData method on the Node class. Try to avoid creating extra work for yourself by creating methods that you're not calling. It just gives your more friction if you decide you need to refactor the way your code works.Context
StackExchange Code Review Q#131431, answer score: 3
Revisions (0)
No revisions yet.