A few suggestion for python - clock core exercise

[GreatBahram]

I'd like to add a few suggestion to make the notes better:

• I Think one possbile solution and also Pythonic (at the same time) could be this one:

  
  MINUTES_PER_HOUR = 60
  MINUTES_PER_DAY = 1440

class Clock: def init(self, hour, minute): self.hour, self.minute = divmod((hour * MINUTES_PER_HOUR + minute) % MINUTES_PER_DAY, 60)

def __str__(self):
    return f"{self.hour:02}:{self.minute:02}"

def __eq__(self, other):
    if isinstance(other, Clock):
        return (self.hour, self.minute) == (other.hour, other.minute)
    return NotImplemented

def __add__(self, minutes):
    return Clock(self.hour, self.minute + minutes)

def __sub__(self, minutes):
    return Clock(self.hour, self.minute - minutes)

* It's not good idea to do this as the return statement:
```python
return self.__class__(self.h, self.m - minutes) # Because Explicit is better than implicit!
# So, this is the something that you can find in python built-in modules.
return Clock(self.h, self.m - minutes) 

• Secondly, this is the oldest way that someone could substitute string in python, why this one?, you cold string format method or even f-string

  def __str__(self):
      return '%02d:%02d' % (self.h, self.m)
  # Instead
  return f"{self.hour:02}:{self.minute:02}"

• No need to mention this is too mathematic:

  self.h, self.m = divmod((hour * 60 + minute) % (24 * 60), 60)

• And this one could much more simpler, like:

  return self.h == other.h and self.m == other.m
  # Instead
  return (self.hour., self.minute) == (other.hour, other.minute) # I really don't get it why h and m? why not hour and minute?
  # tuple comparison is well worth teaching.

• Most people are using python 3, why you guys always subclass object:

  class Clock(object):
  # Instead
  class Clock:

• Again, no need to mention, divmod is here to solve this problem:

  class Clock(object):
  def __init__(self, hour, minute):
      while minute >= 60:
          hour += 1
          minute -= 60
      while minute < 0:
          hour -= 1
          minute += 60
      self.minute = minute
      self.hour = hour % 24

Having said this, I saw this implementation which is far better but maybe hard for a newbie python developer to understand:

from datetime import date, datetime, time, timedelta

class Clock:
    def __init__(self, hours, minutes):
        start = datetime.combine(date.today(), time(0, 0))
        self.dt = start + timedelta(hours=hours, minutes=minutes)

    def __str__(self):
        return self.dt.strftime('%H:%M')

    def __eq__(self, other):
        if not isinstance(other, Clock):
            return NotImplemented
        return (self.dt.time()) == (other.dt.time())

    def __add__(self, minutes):
        self.dt += timedelta(minutes=minutes)
        return self

    def __sub__(self, minutes):
        self.dt -= timedelta(minutes=minutes)
        return self

[GreatBahram]

@jeffdparker

[jeffdparker]

Thanks - I am away on vacation for a few weeks

Jeff Parker

On Fri, Jun 21, 2019 at 2:30 AM Bahram Aghaei notifications@github.com wrote:

@jeffdparker https://github.com/jeffdparker

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/exercism/website-copy/issues/1112?email_source=notifications&email_token=AEHO6CKHMS5ONA6CJC76O2TP3TCX7A5CNFSM4H2RIFW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYIKSYQ#issuecomment-504408418, or mute the thread https://github.com/notifications/unsubscribe-auth/AEHO6CJK23RNTXMLZIIHU63P3TCX7ANCNFSM4H2RIFWQ .

--

• Jeff Parker

[GreatBahram]

Oh, I am very sorry, we discus about it later.

[yawpitch]

@GreatBahram just came across this; overall I like your suggested implementation, but:

def __repr__(self):
    return f"{self.hour:02}:{self.minute:02}"

It's really __str__ that should be overloaded here, not __repr__ ... the former should usually provide the pretty-printed string, while the latter should usually provide an unambiguous representation ... "12:35" is not unambiguously a Clock. As mentioned here it's been somewhat traditional that eval(repr(thing)) should give back an identical copy of thing where possible. Thus repr(Clock(12, 21)) should really return Clock(12, 21).

@hour.setter
def hour(self, value):
    _, value = divmod(value, HOUR_PER_DAY)
    self._hour = value

Bit weirded out by the fact that this class sees itself as mutable to attribute assignment, but immutable to addition and subtraction. I'd prefer to see it either be mutable throughout or immutable throughout. BTW, wouldn't self._hour = value % HOUR_PER_DAY be sufficient here? Also personally I've never been a fan of creating entirely new attributes on a class instance inside a method.

It's not good idea to do this as the return statement:

return self.__class__(self.h, self.m - minutes) # Because Explicit is better than implicit!
# So, this is the something that you can find in python built-in modules.
return Clock(self.h, self.m - minutes) 

That's not entirely true ... any class that inherits from your Clock will give back an instance of its parent class on addition or subtraction, any class that inherits from the previous version will give back an instance of that class. Your approach is fine if the class is never inherited from though.

Again, no need to mention, divmod is here to solve this problem:

class Clock(object):
    def __init__(self, hour, minute):
        while minute >= 60:
            hour += 1
            minute -= 60
        while minute < 0:
            hour -= 1
            minute += 60
        self.minute = minute
        self.hour = hour % 24

I believe that block is there to demonstrate how not to do it.

Having said this, I saw this implementation which is far better but maybe hard for a newbie python developer to understand:

from datetime import date, datetime, time, timedelta

class Clock:
    def __init__(self, hours, minutes):
        start = datetime.combine(date.today(), time(0, 0))
        self.dt = start + timedelta(hours=hours, minutes=minutes)

    def __str__(self):
        return self.dt.strftime('%H:%M')

    def __eq__(self, other):
        if not isinstance(other, Clock):
            return NotImplemented
        return (self.dt.time()) == (other.dt.time())

    def __add__(self, minutes):
        self.dt += timedelta(minutes=minutes)
        return self.__str__()

    def __sub__(self, minutes):
        self.dt -= timedelta(minutes=minutes)
        return self.__str__()

Seems a bit odd to me that two instance of Clock created on different days and having fundamentally different self.dt values can compare as equivalent just because a subset of the data they encapsulate is equivalent ... but what really bugs me here is that addition and subtraction return a str instead of self.

[GreatBahram]

Thanks @yawpitch for your great feedback!

Your point about repr method is completely right. I didn't consider that situation. And also about the final example you are totally right, I should have not return self.__str__ I've just fixed the problems.

However, the whole point in the last implementation is that we can solve the problem by using the timedelta as well. It's true that for two different date with the same time the object return true for comparison this is because the problem just cares about the time, in my opinion.

Anyway, thanks again

[yawpitch]

However, the whole point in the last implementation is that we can solve the problem by using the timedelta as well. It's true that for two different date with the same time the object return true for comparison this is because the problem just cares about the time, in my opinion.

Sure, we can solve it with datetime.timedelta (and datetime.datetime and datetime.time and datetime.date as well), but should we? And should we recommend that students do so? Given the constraints of the problem and the fact that we ultimately want a date-naive implementation, is such a solution not overkill? Aren't we just dragging in dependencies to ultimately do more complicated work and consume more memory without noticeable gain?

If we're going that far, why not just subclass datetime.time?

from datetime import time

class Clock(time):
    def __new__(cls, hour, minute):
        return super().__new__(cls, *divmod((hour * 60 + minute) % (24 * 60), 60))

    def __str__(self):
        return self.strftime("%H:%M")

    def __eq__(self, other):
        if isinstance(other, Clock):
            return self.hour == other.hour and self.minute == other.minute
        return NotImplemented

    def __add__(self, minutes):
        if isinstance(minutes, int):
            return self.__class__(self.hour, self.minute + minutes)
        return NotImplemented

    def __sub__(self, minutes):
        if isinstance(minutes, int):
            return self.__class__(self.hour, self.minute - minutes)
        return NotImplemented

That at least gives the user the ability to treat a Clock as a time and inspect its meaningful attributes without burdening it with what are, really, just extraneous details about the year, month, and date of instantiation.

I can see using timedelta if we were not building a date naive clock, but if you look under the hood of that thing it's __init__ is 99 lines (including comments) of edge case resolution and assertions that ultimately ends up doing the same thing as that divmod call if you've just got hours and minutes anyway.