current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

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()
share|improve this question

3 Answers 3

up vote 5 down vote accepted

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
share|improve this answer
up vote 3 down vote

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)
share|improve this answer
    
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. –  Billylegota Feb 28 at 16:34
    
@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. –  ben rudgers Feb 28 at 20:55
    
Ok, thanks for clarifying. –  Billylegota Feb 28 at 21:59
up vote 3 down vote

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]

share|improve this answer
    
maxSize=16 is already using the standard spacing (though not the recommended capitalization). –  200_success♦ Feb 28 at 7:59
1  
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. –  janos♦ Feb 28 at 16:43
    
@janos Oh, that is interesting. –  Hosch250 Feb 28 at 16:59

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.