1
\$\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\$
8
  • 2
    \$\begingroup\$Your indentation looks off a bit in a number of places. You'll want to check that.\$\endgroup\$CommentedFeb 3, 2015 at 7:30
  • \$\begingroup\$@JeffMercado - I updated the code formatting.\$\endgroup\$
    – LucyBen
    CommentedFeb 3, 2015 at 8:33
  • \$\begingroup\$@200_success - The current code is not broken, it works fine. Just need a review. Thanks\$\endgroup\$
    – LucyBen
    CommentedFeb 3, 2015 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\$CommentedFeb 3, 2015 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\$CommentedFeb 3, 2015 at 8:38

2 Answers 2

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\$

      Start asking to get answers

      Find the answer to your question by asking.

      Ask question

      Explore related questions

      See similar questions with these tags.