Problem
I made a queue that prints out strings a, b, c, and d. It works, but I want to make sure I don’t have any unnecessary code. I realized that my main method prints the strings without the queue. Is there any better way to write the main method and Imp method so that it uses my queue class?
public class Main
{
//this method reverses the string
public static void Imp(StringBuffer str)
{
int n = str.length(); //str.length recognizes the number of
//characters in n, which is also the stack array
Queue obj = new Queue(n); //we set n to a new variable, "obj"
// Push all characters of string
// to stack
int i;
for (i = 0; i < n; i++)
obj.push(str.charAt(i));
// Pop all characters of string
// and put them back to str
for (i = 0; i < n; i++)
{
char ch = obj.pop();
str.setCharAt(i,ch);
}
}
public static void main(String args[])
{
//create a new string
//StringBuffer also takes recognizes the amount of characters
StringBuffer a= new StringBuffer("bob");
StringBuffer b= new StringBuffer("eat too much");
StringBuffer c= new StringBuffer("I love greasy food");
StringBuffer d= new StringBuffer("FORTRAN 77 RULES");
//call Imp method
Imp(a);
Imp(b);
Imp(c);
Imp(d);
//print the reversed string
System.out.println(a + "n" + b + "n" + c + "n" + d);
}
}
//this method creates the stack
//This method creates the queue
class Queue {
int size;
int top;
int bottom;
char[] queue;
Queue(int n)
{
top = 0; //set stack top pointer to 0
bottom = -1; //set bottom pointer to less than the top pointer
size = n; //set size to variable n
queue = new char[size]; //make int n convert to char queue by using size
}
char push(char x){
queue[++bottom] = x; //increment the queue pointer and place x on the bottom of x
return x; //return that x value
}
char pop(){
char x = queue[top++]; //get x from the top of the queue and increment the queue
return x;
}
boolean isEmpty()
{
boolean empty; //make variable "empty" a boolean
empty = false; //"empty" is set as false
if (top >= bottom){ //if top is greater than bottom, then the stack is empty
empty = true;
}
return empty;
}
}
Solution
Virtually all of your comments are visual noise. Comments should explain why things happen, not what is happening. If you can’t read the code to see what is happening, the code is not clear enough.
Use curly braces and whitespace consistently. Open curly braces don’t take a line by themselves. Put whitespace before the open curly brace.
Queue
Your class is called Queue
but it is a Stack
.
All your member variables should be private
. Your methods should probably be public
and not default scope.
queue
should be final, since it’s never being reassigned.
top
and bottom
can have their base values assigned when they’re declared as member variables. top
will be assigned zero by default if you don’t explicitly assign it, but there’s some disagreement as to whether that’s more or less clear.
size
does nothing for you and can be removed. n
should be renamed size
.
x
is a poor name for a char variable. Traditionally it would be c
.
pop
doesn’t need a variable in it. Just return this.queue[this.top++]
.
You should consider whether using ++
as part of your array index value is a good idea or if it makes the code hard to read. The code might be more clear if you separate the array indexing from the variable change.
The body if isEmpty()
can be reduced to return this.top > this.bottom
.
Your class will only work for one round of pushing and popping. The class breaks because bottom and top only get larger. To make the class reusable, you need only one variable which goes up and down as items are pushed and popped. You’ll still have out of bounds errors if they try to pop from an empty stack or push to a full one.
Main
Imp
is a poor name for a method that reverses a String
. Why wouldn’t you name it reverse
?
Methods should start with a lowercase letter.
Your comment says Imp
“reverses theString
”, but it doesn’t. It reverses a StringBuffer
.
Curly braces should never be treated as optional. That’s a bug waiting to happen.
StringBuffer
is thread-safe, and should really only be used in multithreaded environments for performance reasons. Prefer StringBuilder
.
Unless there’s a reason you need to do the reverse in O(1)
space, it would be more idiomatic to create a new StringBuilder
and push the characters into that. Your method could accept a CharSequence
, and interface which String
, StringBuilder
, and StringBuffer
all implement. I’ll assume O(1)
space is a hard requirement, since it would really be preferable to not mutate the input parameter.
n
is a poor variable name. You don’t really need a variable there anyway, since you can just reference length()
directly when you need it.
obj
is a poor variable name. Variable names should describe what is being referenced. stack
would be generic, but at least it’s a bit more descriptive.
You should declare i
inside the scope of the for
loop.
The ch
variable does nothing for you and can be folded into the assignment.
If you were to apply all my suggestions, your Main
class might look something like:
public class Main {
public static void reverse(final StringBuilder characters) {
final Stack stack = new Stack(characters.length());
for (int i = 0; i < characters.length(); i++) {
stack.push(characters.charAt(i));
}
for (int i = 0; i < characters.length(); i++) {
characters.setCharAt(i, stack.pop());
}
}
public static String reverse(final CharSequence charSequence) {
final Stack stack = new Stack(charSequence.length());
for (int i = 0; i < charSequence.length(); i++) {
stack.push(charSequence.charAt(i));
}
final StringBuilder reversedCharSequence = new StringBuilder(charSequence.length());
while (!stack.isEmpty()) {
reversedCharSequence.append(stack.pop());
}
return reversedCharSequence.toString();
}
public static void main(final String args[]) {
final StringBuilder a = new StringBuilder("bob");
final StringBuilder b = new StringBuilder("eat too much");
final StringBuilder c = new StringBuilder("I love greasy food");
final StringBuilder d = new StringBuilder("FORTRAN 77 RULES");
reverse(a);
reverse(b);
reverse(c);
reverse(d);
System.out.println(a + "n" + b + "n" + c + "n" + d);
}
}
and your Queue
class might look something like:
class Stack {
private final char[] stack;
private int top = -1;
public Stack(final int size) {
this.stack = new char[size];
}
public char push(final char c) {
this.top++;
this.stack[this.top] = c;
return c;
}
public char pop() {
final char c = this.stack[this.top];
this.top--;
return c;
}
public boolean isEmpty() {
return this.top >= 0;
}
}