Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

It make sense to not allow links to external code hosting, so I restated my question. This is classical cons list in python using Abstract Base Class interface. Constructor is in fact out of the Cons class, but I didn't see the way to put recursive code inside init.

# immutable, cons list and few methods
class Nil:
    """class Nil, the empty list"""

    def is_empty(self):
        return True
    def head(self):
        return Exception("Empty")
    def tail(self):
        return Exception("Empty")
    def get_next(self):
        return None
    def __str__(self):
        return "()"

class Cons:
    """Class cons, the non empty list: (head, list)"""

    def __init__(self, _head, _tail):
        self.head = _head
        self.tail = _tail

    def is_empty(self):
        return False

    def get_next(self):
        return self.tail

    def __str__(self):
        current = self.tail
        out = "(" + str(self.head)
        while current != Nil:
            out += ' ' + str(current.head)
            current = current.get_next()
        return out +')'



class List(metaclass=ABCMeta):
    """Abstarct class List"""
    @abstractmethod
    def is_empty():
        pass
    def head():
        pass
    def tail():
        pass

List.register(Nil);
List.register(Cons)

def nth(n , xs):
    """Returns nt-h (0 based indexing) elemt of the list, 
    throws an exception when out of range"""
    if xs is Nil:
        return Exception("Out Of Bound")
    if n == 0:
        return xs.head
    else:
        return nth(n - 1, xs.tail)

def length(xs):
    """Returns length of a list O(n)"""
    if xs is Nil:
        return 0
    else:
        return 1 + length(xs.tail)

def cons(elem ,xs):
    """Cons element elem to the list"""
    return Cons(elem, xs)

def subs_in_expr(new, old, xs): 
    """ this helper function works put single    
     list element which maybe list itself"""
    if xs is not Cons:
        if xs == old:
            return new
        else:
            return xs
    else:
        return subst(new, old, xs)

def subst(new, old, xs):
     """substitutes new as old in a list(possible    nested) """
    if xs is Nil:
        return Nil
    else:
        return cons(subs_in_expr(new, old, xs.head) , subst(new ,old,    xs.tail))

def Create_list(args):
    """Crates immutable list from an iterable"""
    if len(args) == 0:
        return Nil()
    tmp = len(args) - 1
    def helper(xs, cnt, limit):
        if cnt > limit:
            return Nil
        else:
            return cons(xs[cnt], helper(xs, cnt + 1, limit))
    return helper(args, 0, tmp)

l1 = Create_list(range(10))
print(l1)
print(nth(3, l1))
print(length(l1))
l2 = subst("A CAT", 0, l1)
print(l2)
'''
output --->
(0 1 2 3 4 5 6 7 8 9)
3
10
(A CAT 1 2 3 4 5 6 7 8 9)
<---
'''

I wrote few methods, they work so far, I think it's OK?

Added after edit:

def remove_nth(n, xs):
"""Removes the n-th elem in the list, throw an Exception
if index out of bounds"""
if isinstance(xs, Nil):
    return Exception("Out of Bounds")
else:
    if n == 0:
        if xs.tail is not Nil:
            return cons(xs.tail.head, xs.tail.tail)
        else:
            return Nil
    else:
        return cons(xs.head, remove_nth(n - 1, xs.tail))
share|improve this question
    
So what prevents you from using a tuple as an immutable list? It has indexing, len, etc... – Graipher Dec 7 '16 at 19:11
    
I need something more functional to mimic Scheme algorithms(strange that I didn't find anything like that in the net), using indexed lists cause problems, and cons in python lists, tuples is O(n). – Tomasz Madry Dec 7 '16 at 19:22
1  
I'm sorry where is the functional programming? I fail to see any here... – Peilonrayz Dec 7 '16 at 19:27
    
Well, the same list construct you can find in scala, scheme... so it's at least functional data structure... – Tomasz Madry Dec 7 '16 at 19:34

The best way to improve this is to change Cons to tuple. But if you don't want to do that then I'd make the code more Pythonic, and by that use class special methods. You should:

  • Use __len__ rather length.
  • Use __getitem__ rather than nth.
  • Use __iter__ rather than manually looping, either via recursion or with a while loop through the linked list.

I'd remove the unneeded ABCMetaclass, and change Nil from a singleton to a class. This is as it can make the code slightly easier to understand.

After this you should notice that Cons is actually a node not a list, and so I'd write an interface for it to act like a list by renaming Cons to ConsNode and creating a list interface Cons.

From this I managed to get:

class Nil:
    def __len__(self):
        return 0

    def __getitem__(self, n):
        raise Exception('Out of bounds')

class ConsNode:
    """Class cons, the non empty list: (head, list)"""
    def __init__(self, _head, _tail):
        self.head = _head
        self.tail = _tail

    def __len__(self):
        return 1 + len(self.tail)

    def __iter__(self):
        current = self
        while not isinstance(current, Nil):
            yield current.head
            current = current.tail

    def __getitem__(self, n):
        if n == 0:
            return self.head
        else:
            return self.tail[n - 1]

class Cons:
    def __init__(self, args):
        """Crates immutable list from an iterable"""
        if len(args) == 0:
            return Nil()
        tmp = len(args) - 1
        def helper(xs, cnt, limit):
            if cnt > limit:
                return Nil()
            else:
                return ConsNode(xs[cnt], helper(xs, cnt + 1, limit))
        self.root = helper(args, 0, tmp)

    def __len__(self):
        return len(self.root)

    def __iter__(self):
        return iter(self.root)

    def __getitem__(self, i):
        return self.root[i]

    def __str__(self):
        return '(' + ' '.join(str(i) for i in self) + ')'

It keeps your recursion in a much easier to read fashion, and adds an abstraction to your recursion, recursion is slow, hard to understand and erroneous if the depth exceeds 1000. So I'd recommend not to use it in Python. It's not the correct language for the job.

Using the above should leave you with two functions, subs_in_expr and subst. This is as we've moved all the other ones into Python's sugar. I changed these to use the __iter__ method, and to instead use isinstance to check that the input is actually your special list. I kept the recursion between both these functions, as it makes sense to keep it. But left me with:

def subs_in_expr(new, old, xs): 
    """ this helper function works put single    
     list element which maybe list itself"""
    if isinstance(xs, Cons):
        return subst(new, old, xs)
    else:
        return new if xs == old else xs


def subst(new, old, xs):
    """substitutes new as old in a list (possible nested) """
    return Cons([subs_in_expr(new, old, n) for n in xs])

Honestly I'd suggest you'd use better variable names xs is horribly cryptic, both the genetic list or data are much better. But if you want to keep nth you'd add:

def nth(n , xs):
    """Returns nt-h (0 based indexing) elemt of the list, 
    throws an exception when out of range"""
    return xs[n]

But I'd recommend that you don't as it is removing syntactic sugar. Which leaves the main of the code to be:

l1 = Cons(range(10))
print(l1)
print(l1[3])
print(len(l1))
l2 = subst("A CAT", 0, l1)
print(l2)

Just a couple of renames on what you provided. But the best part is you can change your code to use tuple instead of Cons, and it work exactly the same. But should give better reliability and better performance. C is hands down faster than Python. And it also remove potential problematic code, that you have to maintain.


Finally the data structure that you're creating is a linked list, which is intended to be mutable, in fact the entire point of a linked list is \$O(1)\$ insertion at any point in the list, if your pointer is already there. And is used in Queues, Stacks and deques due to this benefit, that you're removing. But on the all it's a whole lot less efficient than a properly implemented datatype, and so use a tuple. Cons = tuple will allow you to remove all the classes, and keep all the functions and the module level code.

share|improve this answer
    
Thanks for that, I agree in general. Python doesn't have TCO Support. so them recursive structures will not work for bigger data (but for now tis is just to mimic scheme and keep up with materials). I just disagree that this code is less readable. Generally recursive code is easier to understand and reason with. Just one example, function works on this cons list: remove_nth - which removes the n-th element from it. When it's empty throws exception if not it recurs, consing till the moment when reach element to be removed (when n = 0), then stops recursion and cons the rest of the list – Tomasz Madry Dec 8 '16 at 17:22
    
Now recall code to remove element from double linked list and which one is more readable? – Tomasz Madry Dec 8 '16 at 17:23
    
@TomaszMadry I know which is more readable, and it's not what you've written. – Peilonrayz Dec 8 '16 at 17:31

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.