Kindergarten Garden challenge: ugly zip-group-flatten?

I wrote up a solution for the "Kindergarten Garden" problem on exercism.io.


My class will be handed a string that describes two rows of plants. Each student (either in a pre-set list or given as an argument) (sorted alphabetically) owns a 2x2 of plants. The method Garden.plants accepts a student name and returns a list of which plants they have, in the order:

12
34

My code just feels ugly and unreadable, though. I tried to refactor some of it out into a method, but it still didn't feel great to me, so I separated my ugly one-liner into a bunch of assignments to temporary variables, but it still feels like it could be better.

```
from functools import partial

def grouper(iterable, n):
return zip([iter(iterable)] n)
def flatten(seq):
for item in seq:
if hasattr(item, '__iter__') and not isinstance(item, str):
yield from flatten(item)
else:
yield item

class Garden(object):
plant_names = {"G":"Grass",
"C":"Clover",
"R":"Radishes",
"V":"Violets"}
def __init__(self, rows, students=None):
self.rows = rows.splitlines()
if students is None:
students = ["Alice", "Bob", "Charlie", "David", "Eve", "Fred",
"Ginny", "Harriet", "Ileana", "Joseph",
"Kincaid", "Larry"]
self.students = sorted(students)
self.plant_assignments = {student: list(plant_group) for student,plant_group
in zip(self.students, self.plant_groups(self.rows))}
def get_plant_name(self,plant_letter):
try:
return self.plant_names[plant_letter]
except KeyError:
raise ValueError("Plant letter {} is not defined".format(plant_letter))
def plants(self,name):
return self.plant_assignments[name]
def plant_groups(self,rows):
"""Group each row in twos, then zip, flatten,
and map in the expected way so:

"

It's not a bad solution, but it doesn't feel quite fluently expressed in Python. In particular, I feel like the conversions between lists and generators are unnatural. It would also help if you toned down the functional thinking in favour of list comprehensions.

I'll start by mentioning that blank lines between method definitions would make your code more readable, as suggested in PEP 8. Also, constants should be named using ALL_CAPS.

Let's start with the constructor:

def __init__(self, rows, students=None):
    self.rows = rows.splitlines()
    if students is None:
        students = ["Alice", "Bob", "Charlie", "David", "Eve", "Fred",
                    "Ginny", "Harriet", "Ileana", "Joseph",
                    "Kincaid", "Larry"]
    self.students = sorted(students)
    self.plant_assignments = {student: list(plant_group) for student,plant_group
            in zip(self.students, self.plant_groups(self.rows))}

• You aren't taking full advantage of the default parameter for students.

• self.rows and self.students are never used outside of the constructor. As they aren't useful as part of the state of the Garden object, they could just be local variables.

• The dict(iterable) constructor would be a more succinct way to define self.plant_assignments than the set comprehension.

The "public" method, plants(), would be clearer if you renamed name to student.

The functions get_plant_name() and plant_groups() are for "internal" use. Interspersing them among methods in the public interface makes your code less readable.

They are also "pure" functions that don't depend on any object state. I suggest declaring them as static methods.

The heart of the solution is plant_groups():

def plant_groups(self,rows):
    """(Docstring omitted)"""
    grouper_by_two = partial(grouper, n=2)
    grouped_by_two = map(grouper_by_two, rows)
    cubewise = zip(*grouped_by_two)
    flat_cube = flatten(cubewise)
    flat_cube_with_names = map(self.get_plant_name, flat_cube)
    yield from grouper(flat_cube_with_names, 4)

In Python, map() is usually slightly better written as a list comprehension instead; partial() makes the case for a list comprehension even more compelling.

Instead of cubewise, I suggest the name plots, which forms a clear and simple mental image that is highly relevant to the problem being solved.

flatten() works recursively. I suggest using a list comprehension (see implementation below) so that such recursion would no longer be necessary. In fact, by flattening all the way to a linear list, you need to reorganize the individual pots into groups of 4.

Using yield from is an unnecessary complication. It just requires the caller to convert the results back into a list using list(plant_group).

Putting everything together…

from itertools import chain

def grouper(iterable, n):
    return zip(*[iter(iterable)] * n)

def flatten(seq):
    yield from chain(*seq)

class Garden(object):
    PLANT_NAMES = {
         "G": "Grass",
         "C": "Clover",
         "R": "Radishes",
         "V": "Violets",
    }

    STUDENTS = [
        "Alice", "Bob", "Charlie", "David", "Eve", "Fred",
        "Ginny", "Harriet", "Ileana", "Joseph",
        "Kincaid", "Larry"
    ]

    @staticmethod
    def get_plant_name(plant_letter):
        try:
            return Garden.PLANT_NAMES[plant_letter]
        except KeyError:
            raise ValueError("Plant letter {} is not defined".format(plant_letter))

    @staticmethod
    def plant_groups(rows):
        grouped_by_two = [grouper(row, 2) for row in rows]
        plots = zip(*grouped_by_two)
        return [
            [Garden.get_plant_name(plant) for plant in flatten(plot)]
            for plot in plots
        ]

    def __init__(self, rows, students=STUDENTS):
        rows = rows.splitlines()
        students = sorted(students)
        self.plant_assignments = dict(zip(students, self.plant_groups(rows)))

    def plants(self, student):
        return self.plant_assignments[student]

StackExchange Code Review Q#65035, answer score: 3