Writing .ppm images to a file

Question

I am struggling with commenting and variable naming. My teacher says my comments need to be more explanatory and explicit. He says my variable names are also sometimes confusing. I was just wondering whether you could go through my code and see whether you are able to understand the comments and add in comments/edit where you feel I should add comments or improve them. Lastly, are there any general rules to follow with commenting?

class PPM(object):
    def __init__(self, infile, outfile):
        self.infile=infile
        self.outfile=outfile

        #Read in data of image
        data= open(self.infile,"r")
        datain=data.read()
        splits=datain.split(None, 4)

        #Header info and pixel info
        self.type=splits[0]
        self.columns=int(splits[1])
        self.rows=int(splits[2])
        self.colour=int(splits[3])

        #(Return a new array of bytes)
        self.pixels=bytearray(splits[4])

    def grey_scale(self):
            for row in range(self.rows):
                for column in range(self.columns):
                    start = row * self.columns * 3 + column * 3
                    end = start + 3
                    r, g, b = self.pixels[start:end]
                    brightness = int(round( (r + g + b) / 3.0 ))
                    self.pixels[start:end] = brightness, brightness, brightness

    def writetofile(self):
        dataout= open(self.outfile, "wb")
        #Use format to convert back to strings to concatenate them and Those {} in the write function get's replaced by the arguments of the format function.
        dataout.write('{}\n{} {}\n{}\n{}'.format(self.type,
                                                 self.columns, self.rows,
                                                 self.colour,
                                                 self.pixels))

sample = PPM("cake.ppm", "Replica.ppm")
sample.grey_scale()
sample.writetofile()

Answer 1

To steal an old quote: "There are 2 hard things in computer science. Naming, cache invalidation, and off-by-one errors".

That being said, there is room for improvement here. Firstly, I'm assuming the class name, PPM, is short for Portable Pixmap Format. However, this isn't immediately obvious, and if you aren't familiar with that format (I'm not), it required a search. Hence, the first thing I'd do is change the name to something a bit more descriptive, and add a docstring explaining something about the format:

class PortablePixmap(object):
    '''A class encapsulating basic operations on images that use the
       portable pixmap format (PPM). 

    '''

Python itself has a style guide known as PEP8 that you should try to follow as much as possible. Generally the convention in python is to name ClassesLikeThis, methods_like_this, and variables_like_this. Hence, another change I'd make is to rename infile and outfile to in_file and out_file respectively.

Continuing on, the first comment under __init__ is fairly obvious:

#Read in data of image
data= open(self.infile,"r")
datain=data.read()

As a minor aside, try and keep the whitespace around operators like = consistent. Again, as per PEP8, these should be:

data = open(self.infile, "r")
data_in = data.read()

I'd also consider renaming data_in to something like raw_image_data.

Back to the comments. The next line has no comment, but needs it far more than the previous 2 lines:

# Break up the image data into 4 segments because ...
splits = datain.split(None, 4)

The comment #(Return a new array of bytes) is both obvious and misleading: this is __init__; you're constructing the object - assigning to self.pixels isn't returning anything!

For grey_scale, your indentation moves to 8 spaces instead of 4. Be careful with this - especially in Python, where whitespace can modify the semantics (meaning) of your program. This function should again have a docstring:

def grey_scale(self):
    '''Converts the supplied image to greyscale.'''

The final function, def writetofile(self): should again use _ as separators to make it easier to read. I'd also probably move the out_file parameter to this function, rather than passing it in __init__:

def write_to_file(self, output_location):
    '''Writes the image file to the location specified output location.
       The file is written in <some description of the file format>.

    '''

Watch the length of your comments (they should stick to the same line lengths as everything else in your program).

#Use format to convert back to strings to concatenate them and Those {} in the write function get's replaced by the arguments of the format function.

The comment itself is also difficult to understand:

"convert back to string to concatenate them and Those {} ..."

That made me do a double take. Try and write comments as complete sentences.

Answer 2

Yuushi links to PEP8. It has a section specifically on comments, which might be useful since you asked about general rules for commenting.

I would say comments should talk more about the why of the code than the what. After reading in the header, why split it into four pieces? What does the header look like (when dealing with a particular data format, I find it extremely helpful to provide an example in the comments)? In the last line of the constructor, why do we need the same data in another bytearray?

Other than that, what Yuushi said. :)

Answer 3

In python, docstrings are common-place to use to assist in automatic doc generation. You can typically expect a docstring for each class and each function. Take a look at http://epydoc.sourceforge.net/manual-docstring.html

As for doc coding standards, there are many. Here's one:

http://www.python.org/dev/peps/pep-0257/

Getting fancy, you can structure your docstrings using epytext markup language:

http://epydoc.sourceforge.net/manual-epytext.html

Example using only docstrings:

class PPM(object):
    '''What does the PPM class do? Comment me here.'''
    def __init__(self, infile, outfile):
        '''what am I expecting for the infile/outfile? is it strings or a file handler'''
        self.infile=infile
        self.outfile=outfile

        #Read in data of image - it is good practice to indent comments so you can know where they line up
        data= open(self.infile,"r")
        datain=data.read()
        splits=datain.split()


        # Reads the headers info and writes it
        self.type=splits[0]
        self.columns=int(splits[1])
        self.row=int(splits[2])
        self.colour=splits[3]



        # Splits the pixels and adds them to the matrix using the columns and row information obatined from the image
        pixels= splits[4:]
        self.Lines = []

        for rowCount in range(self.row):
            rowStart = rowCount * self.columns*3
            rowEnd = rowStart + (self.columns * 3)

            self.Lines.append( pixels[rowStart : rowEnd] )


    # consider making this write_to_file - be consistent in the spacing (or lack of spacing in your function names)
    def writetofile(self):
        '''what does this function do?'''

        #Opens the output file and writes out the header information
        dataout= open(self.outfile, "w")
        dataout.write(self.type+"\n" + str(self.columns) + "\n" + str(self.row) +"\n"+ self.colour + "\n")

        for line in self.Lines:
            dataout.write(" ".join(line) + "\n")

        dataout.close()

    def horizontal_flip(self):
        '''what does this function do?'''
            # inconsistent tabbing (3 tabs spaces here, 2 normally)
            for line in self.Lines:
                FlippedLine = line[::-1]
            # First join values so writing is possible and than, loop through the values to write all data to output.
            for data in FlippedLine:
                Reversed= " ".join(data) + " "
                userInstance.writetofile(Reversed)


    def flatten_red(self):
        '''Grabs every red value and sets it equal to 0'''
        #Loops till end of row  in order to grab all red values
        for row in range(self.row):
            #loops by 3 in order to grab only red values
            for col in range(0, self.columns*3, 3):
                self.Lines[row][col]= str(0)

        print(self.Lines)




    def negate_red(self):
        '''Grabs every red value and converts it to its negative value by subtracting the colour value from the red value'''
        for row in range(self.row):
            for col in range(0, self.columns*3, 3):
                remainder = int(self.colour) -int((self.Lines [row][col]))
                self.Lines[row][col] = str(remainder)
        print(self.Lines)


    def grey_scale(self):
    '''Converts the supplied image to greyscale.'''

        for row in range(self.row):
            for col in range(0, self.columns*3, 3):

                sum = int(self.Lines[row][col]) + int(self.Lines[row][col+1]) + int(self.Lines[row][col+2])
                avg = int(sum/3)

                self.Lines[row][col] = str(avg)
                self.Lines[row][col+1] = str(avg)
                self.Lines[row][col+2] = str(avg)








print ("Portable Pixmap Image Editor")

input_file = input("Enter the name of image file: ")

output_file = input("Enter the name of the output file: ")

userInstance = PPM(str(input_file), str(output_file))

print ("Here are your choices: [1] Convert to grayscale  [2] Flip horizontally  [3] Negation of red  [4] Elimination of red")

option1 = input("Do you want [1]? (write y or n)")

option2 = input("Do you want [2]? (write y or n)")

option3 = input("Do you want [3]? (write y or n)")

option4 = input("Do you want [4]? (write y or n)")

if option1 == "y":
    userInstance.grey_scale()

if option2 == "y":
    userInstance.horizontal_flip()

if option3 == "y":
    userInstance.negate_red()

if option4 == "y":
    userInstance.flatten_red()

userInstance.writetofile()

print(output_file + " created.")

This example for starters.