current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 8 down vote favorite

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()
share|improve this question
add comment

2 Answers

up vote 12 down vote accepted

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.

share|improve this answer
    
upvoted twice ... nice. –  rolfl♦ Jan 10 at 7:09
add comment
up vote 2 down vote

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. :)

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.