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.

I've written a Time class that records the time of day and performs simple timing operations (add 2 times, convert a time object to an integer and back again,etc.) following the prompts in How to Think Like a Computer Scientist: Learning with Python.

class Time(object):
"""Attributes: hours, minutes, seconds"""
def __init__(self,hours,minutes,seconds):
    self.hours =hours
    self.minutes=minutes
    self.seconds=seconds

def print_time(self):
    """prints time object as a string"""
    print "%.2d:%.2d:%.2d" % (self.hours, self.minutes, self.seconds)

def _str_(self):
    """returns time object as a string"""
    return "%.2d:%.2d:%.2d" % (self.hours, self.minutes, self.seconds)

def name(self,name):
    """names an instance"""
    self.name=name
    return self.name

def after(t1,t2):
    """checks to see which of two time objects is later"""
    if t1.convert_to_seconds()<t2.convert_to_seconds():
        return "%s is later" %(t2.name)
    elif t1.convert_to_seconds>t2.convert_to_seconds:
        return "%s is later" %(t1.name)
    else:
        return "these events occur simultaneously"

def convert_to_seconds(self):
    """converts Time object to an integer(# of seconds)"""
    minutes=self.hours*60+self.minutes
    seconds=minutes*60+self.seconds
    return seconds

def make_time(self,seconds):
    """converts from an integer to a Time object"""                
    self.hours=seconds/3600
    seconds=seconds-self.hours*3600
    self.minutes=seconds/60
    seconds=seconds-self.minutes*60
    self.seconds=seconds        

def increment_time(self, seconds):
    """Modifier adding a given # of seconds to a time object
    which has been converted to an integer(seconds); permanently
    alters object"""
    sum=self.convert_to_seconds()+seconds
    self.make_time(sum)
    return self._str_()


def add_time(self, addedTime):
    """adds 2 Time objects represented as seconds;
       does not permanently modify either object"""
    import copy
    end_time=copy.deepcopy(self)
    seconds=self.convert_to_seconds()+addedTime.convert_to_seconds()
    end_time.make_time(seconds)
    return end_time._str_()  

Usage examples:

breakfast=Time(8,30,7)
dinner=Time(19,0,0)
smokebreak=Time(19,0,0)
interval=(4,30,0)

print dinner.after(smokebreak)
print breakfast.add_time(interval)
print breakfast.increment_time(3600)

The Time Class example in the text I'm following does not use an init method, but passes straight into creating a time object and assigning attributes. Is there any advantage to including an init function, as I have done? Removing the init method seems to make it easier to adopt a terser and more functional style. Is including a method to name instances bad form, as I suspect? Would it be better write a functional-style version of add_time without importing deepcopy? I would appreciate any advice on best practices in Python 2.x OOP.

share|improve this question
add comment

3 Answers 3

up vote 3 down vote accepted

It is perfectly fine to implement an __init__ method in this case. I think the only thing you should note is, by the way it's defined, the Time class forces the programmer to give values for hours, minutes and seconds to define a Time object. So, with that constraint in mind, it's really up to you as to whether this is an advantage or disadvantage. Do you want to force the programmer (most likely yourself) to enter these values here? Or should you allow him to first construct an object and then define them later? This is your decision; I don't think Pythoneers will try to sway you one way or another.

Alternatives are (1) as you've already implied: removal or (2) giving these variables default values.

For example:

def __init__(self,hours=None,minutes=None,seconds=None):
    self.hours =hours
    self.minutes=minutes
    self.seconds=seconds

With the above you are giving the user the option to define these later or now with no penalty either way. Just keep the following simple philosophy of Python in mind:

Easier to ask for forgiveness than permission

(From the Python Glossary)

I don't really see a point in naming instances. Do you have a rationale for that? Also, if you choose to name them, I think that returning said name after setting is an unexpected behavior and gives the name function more responsibility than it needs.

In your add_time method, I would suggest constructing a new Time object using the values of the self object and then returning that with the incrementation. And, in general, import statements occur at the top of the Python module, unless it is a really special case.

Overall, everything looks pretty good. I hope this was somewhat helpful, if you have any questions be sure to comment.

share|improve this answer
    
Thanks for your comments. The name method is of little utility - I thought that naming instances might be useful in print debugging (tracking objects passed between methods), and I also used the name method in the after method. Perhaps using _str_ would be a better choice. –  K. Olivia Dec 17 '12 at 14:26
add comment

Two small issues:

  1. Why is Time.make_time an instance method? I shouldn't have to have a Time object to make one. Consider making it a static method.
  2. You seem to be using underscore_naming everywhere except def add_time(self, addedTime):
share|improve this answer
add comment

mjgpy3's answer is very good, though here are a few nit-picks of my own:

1) Conventions are a good thing to adhere to, and the best guide I have found for python is here. It is far more important to learn the concepts of a language, however, so if you feel this is too much at once, focus on learning the hard stuff before you get to the syntactical sugar.

2) your print_time and _str_ functions do nearly the same thing: perhapes it would be better to combine them together? Perhaps like this:

def _str_(self):
    """Returns the object as a string"""
    return '{}:{}:{}'.format(self.hours, self.minutes, self.seconds)

2a) As well, the % formatter is commonly used less compared to the .format() option. You could use either, but be sure of the limitations of both.

Apart from those small nit-picks, your code seems fine to me

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.