2
\$\begingroup\$

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;

        }
}
\$\endgroup\$
  • \$\begingroup\$ I don't know what this means: "my main method prints the strings without the queue". Your main method calls Imp, which uses the stack. What requirements are you trying to meet? \$\endgroup\$ – Eric Stein Sep 18 '18 at 14:53
2
\$\begingroup\$

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;
    }
}
\$\endgroup\$

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.