0
\$\begingroup\$

I am just testing out a couple of small classes for learning purposes. Here is my current working code:

class Permutation:
     '''An int list a permutation if starting at any index, one can reach every other index by
     repeated index accesses. The list [4, 0, 3, 1, 2] is a permutation: for example index 0
     stores a 4; index 4 stores a 2; index 2 stores a 3; index 3 stores a 1; index 1 stores a 0,
     bringing us back to that starting position after visiting every index.'''
     def __init__(self, p, start):
         self.p = p
         self.start = start

     def __iter__(self):
         class P_iter:
             def __init__(self, p, start):
                 self.p = p
                 self.start = start
                 self.curr = start
                 self.stahp = False

             def __next__(self):
                 if not self.stahp:
                     current = self.curr
                     self.curr = self.p[self.curr]
                     self.stahp = self.curr == self.start
                     return current
                  else:
                      raise StopIteration
         return P_iter(self.p, self.start)


class Permutation2:
     '''similar to the description above, but its __iter__ method should be implemented by a
     generator (not a P_iter nested class)'''
     def __init__(self,p,start):
         self.p = p
         self.start = start

     def __iter__(self):
         curr = self.start
         stahp = False
         while not stahp:
             yield curr
             curr = self.p[curr]
             stahp = curr == self.start
         else:
             raise StopIteration

I'm looking to clean the code a bit and maybe make it simpler. The code is fully working currently and generates the desired output.

\$\endgroup\$
  • 2
    \$\begingroup\$ Your indentation looks off a bit in a number of places. You'll want to check that. \$\endgroup\$ – Jeff Mercado Feb 3 '15 at 7:30
  • \$\begingroup\$ @JeffMercado - I updated the code formatting. \$\endgroup\$ – LucyBen Feb 3 '15 at 8:33
  • \$\begingroup\$ @200_success - The current code is not broken, it works fine. Just need a review. Thanks \$\endgroup\$ – LucyBen Feb 3 '15 at 8:34
  • 1
    \$\begingroup\$ Whitespace is significant in Python, therefore it was broken code. Thanks for fixing it. I've reopened the question. \$\endgroup\$ – 200_success Feb 3 '15 at 8:35
  • 1
    \$\begingroup\$ @200_success: Still not quite bug-free. The doc strings, the whole __iter__() method of the first implementation... in this particular case, it's clear to see the intent... but it's still broken code. \$\endgroup\$ – Jeff Mercado Feb 3 '15 at 8:38
2
\$\begingroup\$

Well Permutation has basically equivalent code to Permutation2, but is doing things the hard way. Therefore, I'll just look at Permutation2, particularly __iter__().

 def __iter__(self):
     curr = self.start
     stahp = False
     while not stahp:
         yield curr
         curr = self.p[curr]
         stahp = curr == self.start
     else:
         raise StopIteration

You shouldn't need to explicitly raise StopIteration. Iteration automatically stops when the flow of control reaches the end of the function.

I find the LoLcat spelling of "stop" amusing. However, flag variables are generally a bad idea. You would be better off writing if curr == self.start: break, for example. However, I'd just choose to write it like this:

def __iter__(self):
    yield self.start
    curr = self.p[curr]
    while curr != self.start:
        yield curr
        curr = self.p[curr]

(Once again, the lack of a do-while loop in Python makes life difficult.)

Take care to indent your code very carefully in Python, as whitespace is significant. Four spaces is a very strong norm, established in PEP 8. You've used five spaces at the first level.

\$\endgroup\$
1
\$\begingroup\$
  • P_iter should have an __iter__ method to fully comply with the iterator protocol:

    def __iter__(self):
        return self
    
  • Defining the P_iter class inside a method hurts readability and makes the class unavailable for anyone who wants to check the type of an iterator object.

  • In Permutation2.__iter__ I would use this approach to avoid the stahp variable :

    def __iter__(self):
        curr = self.start
        while True:
            yield curr
            curr = self.p[curr]
            if curr == self.start:
                return
    
\$\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.