6
\$\begingroup\$

I have written a simple stack implementation and would like some feedback as to what I could improve in my code and coding practices.

# A simple stack in python with ints.
class stack():
    def __init__(self, maxSize=16):
        self.maxSize = maxSize
        self.data = []

    def isEmpty(self):
        if len(self.data) == 0:
            return True
        else:
            return False

    def isFull(self):
        if len(self.data) == self.maxSize:
            return True
        else:
            return False

    def push(self, data):
        if not self.isFull():
            self.data.append(data)
            return "OK"
        else:
            return "ERR_Stack_Full"

    def pop(self):
        if not self.isEmpty():
            output = self.data[len(self.data) -1]
            del self.data[len(self.data) -1]
            return output, "OK"
        else:
            return "ERR_Stack_Empty"

    def npop(self, n):
        output = []
        if len(self.data) >= n:
            for i in range(n):
                output.append(self.pop()[0])  
            return output, "OK"

Based on the input given here is my modified code (Sorry about the entire thing being indented, the code sample button was being difficult):

    class EmptyStackError(Exception):
        def __init__(self):
                super().__init__("Stack is empty: cannot pop from an empty stack!")


    class FullStackError(Exception):
            def __init__(self):
                    super().__init__("Stack is full: cannot push to a full stack!")


    # A simple stack in python with ints.
    class Stack():
        def __init__(self, max_size=16):
            self.max_size = max_size
            self.data = []

        def is_empty(self):
            if len(self.data) == 0:
                return True

        def is_full(self):
            if len(self.data) == self.max_size:
                return True

        def push(self, data):
            if not self.is_full():
                self.data.append(data)
                return data
            else:
                raise FullStackError()

        def pop(self):
            if not self.is_empty():
                output = self.data[len(self.data) -1]
                del self.data[len(self.data) -1]
                return output
            else:
                raise EmptyStackError()           
\$\endgroup\$
5
\$\begingroup\$

Better to follow a common stack API

The common API for a stack:

  • push(item): add an item on the top of the stack and then return the item
  • pop(): remove the top item from the stack and return it

My objection is against what your methods return. push returns a message, pop returns an (item, message) tuple. If you think about it, the message is useless. Users of this class would have to check the output of each call, and evaluate success or failure using string comparison. This is very weak.

Instead of using the return value to indicate success and failure, it would be better to follow the common API, and raise exceptions in case of failures. For example like this:

class EmptyStackError(Exception):
    def __init__(self):
        super().__init__("Stack is empty: cannot pop an empty stack")


class StackFullError(Exception):
    def __init__(self):
        super().__init__("Stack is full")


class stack():
    # ...

    def push(self, data):
        if self.isFull():
            raise StackFullError()
        self.data.append(data)
        return data

    def pop(self):
        if self.isEmpty():
            raise EmptyStackError()
        item = self.data[len(self.data) -1]
        del self.data[len(self.data) -1]
        return item

Python conventions

Follow PEP8, the official coding style guide of Python. For example:

  • The convention for class names is PascalCase, so stack should be Stack
  • The convention for method names is snake_case, so isFull should be is_full
\$\endgroup\$
3
\$\begingroup\$

Hard coding a magic number for stack size has code smell.

Stack semantics derive from automata theory. As an abstraction, stacks do not have a fixed size [of 16 or anything else] and cannot be filled only emptied. An object oriented implementation should perhaps reflect this at the top of the inheritance hierarchy to avoid conflating implementation details with stack semantics.

When it is necessary to implement a stack of limited size, consider taking size as a parameter to the constructor not hard coded as a magic number. This will allow greater code reuse and more clearly separate implementation dependencies from the higher level of abstraction.

Finally, why not implement stack as a list?

Example code:

class Stack():
    def __init__(self):
        self.stack = []

    def push(self, val):
        self.stack.append(val)

    def pop(self):
        try:
            return self.stack.pop()
        except IndexError:
            print "Sorry, cannot pop an empty stack"

At this point, we are free to implement specific business logic such as that for modeling an HP11C calculator (I've had mine since 1988).

class MyStack(Stack):

    def __init__(self, size):
        Stack.__init__(self)
        self.size = size

    def push(self, val):
        if len(self.stack) < self.size:
            Stack.push(self, val)
        else:
            print "Sorry, stack is full"

    def peek(self):
        temp = self.pop()
        self.push(temp)
        return temp

    def is_empty(self):
        return len(self.stack) == 0

    def flush(self):
        self.stack = []

A sound inheritance model allows for C style stack semantics where pop simply deletes the top of the stack without returning a value and peek is the way to read the top of the stack.

class MyCStyleStack(MyStack):

    def pop(self):
        MyStack.pop(self)
\$\endgroup\$
  • \$\begingroup\$ The code is for use in an RPN calculator project I am doing on an AVR. I wrote it first in python so I could wrap my head around what I would have to write in C++. Traditionally RPN calculators (or at lest the HP ones I have owned) only had a 4-16 index stack. Because of storage limitations I needed to have an arbitrary limit to the stack size. \$\endgroup\$ – Billylegota Feb 28 '15 at 16:34
  • 1
    \$\begingroup\$ @Billylegota To me, implementing code to understand the mechanics of a system is all the more reason to separate the data structure from the implementation details [see edited answer]. YMMV. \$\endgroup\$ – ben rudgers Feb 28 '15 at 20:55
3
\$\begingroup\$

This method can be simplified:

def isEmpty(self):
    if len(self.data) == 0:
        return True
    else:
        return False

This is sufficient:

def isEmpty(self):
    return len(self.data) == 0

The same applies to isFull(self).

The rest looks good to me, except you should be consistent about using using spaces around your operators according to the PEP 8 style:

output = self.data[len(self.data) -1]

\$\endgroup\$
  • 1
    \$\begingroup\$ maxSize=16 is already using the standard spacing (though not the recommended capitalization). \$\endgroup\$ – 200_success Feb 28 '15 at 7:59
  • 2
    \$\begingroup\$ maxSize=16, being a keyword argument, follows PEP8 as it is (in terms of spacing, at least). Adding spaces there would violate PEP8. It's not that this is "inconsistent", it's that some rules depend on the context. \$\endgroup\$ – janos Feb 28 '15 at 16:43
  • \$\begingroup\$ @janos Oh, that is interesting. \$\endgroup\$ – Hosch250 Feb 28 '15 at 16:59

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.