[Tutor] don't repeat yourself; question about code optimization

[Alan Gauld]

<tpc247 at gmail.com> wrote

> one part where I believe I violate the prime directive of coding, 
> which is
> not to repeat yourself:

The prime directive of coding is make it readable!
The DRY principle is just that a principle. If repeating makes for
more maintainable or readable code then repeat yourself.

Getting too hung up on a catchy acronym is a dangerous
thing IMHO. It can lead to too much effort going into satisfying
the principle and not enough into thinking about the problem at
hand and how the code will be maintained.

Remember 80% of the cost of software is in maintenance not initial
development. DRY is one way to improve maintainablility and that's
its purpose - to avoid having to fix things in two places - but it is 
not
a rule that must be slavishly followed at the expense of
readability/maintainability. It's in the same category as
Do not use GOTO,
Do not use global variables,
Functions should be less than 25 lines long

These are all useful principles which usually improve code quality
but there are exceptions in every case.

Now in your example removing repetition will improve things slightly,
but I am always concerned when I see terms like "prime directive of
coding" being applied to something like DRY.

PS The most serious problem with your code from my perpspective
is that your variable names are way too long. That affects maintenance
and readability more that the repetition (and in the case of email it
causes line wrapping that makes it even worse!) The names are
also not accurate of their function, for example "total_num_of_items"
is not a number at all but a list of numbers...

I'd therefore suggest a rewrite of your init method like:

    def __init__(self, num_of_items, max_per_row):
        self.numbers = range(num_of_items)
        self.max_per_row = max_per_row

And the modified create method looks like:

    def create_grid(self):
        table = []
        row = []
        count = 0
        while self.numbers:
            row.append(self.numbers.pop(0))
            count += 1
         if (not self.numbers) or (count == self.max_per_row):
                table.append(tuple(row))
                row = []
                count = 0
        return table

Which makes the indentation error more obvious...

The use of a while loop here could be replaced by a Python
for loop which eliminates your repetition and is more natural:

    def create_grid(self):
        table = []
        row = []
        for number in self.numbers:
            row.append(number)
            if len(row) == self.max_per_row:
               table.append(tuple(row))
               row = []
        if len(row) != 0   # not sure if you want partially completed 
rows or not
           table.append(tuple(row))
        return table

However, since you are in effect trying to create a list of lists
I suggest a combination of list comprehension and slicing would be
a better solution.

    def create_grid(self):
        start = 0
        table = []
        num_rows = len(self.numbers)/max_per_row    # this should 
really be in init!
        for n in range(num_rows):
            row = [num for num in 
self.numbers[start:start+self.max_per_row]]
            table.append(row)
            start += self.max_per_row
        return table

Which happens to meet the DRY principle. But we got there, not by 
trying
to avoid DRY but by improving the algorithm and structure of the code.
DRY was merely a side efffect.

Finally the class is not a good class in an OOP sense since it is 
nearly a verb.
It is better done as a simple function in my view, just pass in the 
parameters to
the create method. Like this:

def create_table(num_items, row_size):
    start = 0
    table = []
    num_rows = num_items/row_size
    for n in range(num_rows):
        row = [num for num in range(start,start+row_size)]
        table.append(tuple(row))
        start += row_size
    return table

Or better still build a table class that knows how to create
itself, but also knows how to maniplulate itself too - doing whatever
it is you intend doing to the table you just created! This reflects 
another
programming "rule" - the law of demeter" - one of the fundamentals of 
OOP.

-- 
Alan Gauld
Author of the Learn to Program web site
http://www.freenetpages.co.uk/hp/alan.gauld