clock: canonical test data has been improved

[kytrinyx]

The JSON file containing canonical inputs/outputs for the Clock exercise has gotten new data.

There are two situations that the original data didn't account for:

• Sometimes people perform computation/mutation in the display method instead of in add. This means that you might have two copies of clock that are identical, and if you add 1440 minutes to one and 2880 minutes to the other, they display the same value but are not equal.
• Sometimes people only account for one adjustment in either direction, meaning that if you add 1,000,000 minutes, then the clock would not end up with a valid display time.

If this track has a generator for the Clock exercise, go ahead and regenerate it now. If it doesn't, then please verify the implementation of the test suite against the new data. If any cases are missing, they should be added.

See exercism/problem-specifications#166

[remcopeereboom]

@kytrinyx, @kotp

Sometimes people perform computation/mutation in the display method instead of in add. This means that you might have two copies of clock that are identical, and if you add 1440 minutes to one and 2880 minutes to the other, they display the same value but are not equal.

I am not sure what you mean here exactly. Perhaps you could clarify or add some example code? From what I understand, this seems to be two issues. One the problem of having mutation where it does not belong and another, where people mistake identity for equality (surely we want equality here?).

I think part of the problem lies in the fact that the tests for the add operation all check the return value against a String. The ruby tests call #to_s on the mutated clocks and the Go tests call .String(). This makes the tests nice to read, but does rely on #to_s and it's equivalents to work properly. If we find that this is not the case, then perhaps the easiest solution is to change clocks to check equality with other clocks. We could even add tests to make sure that #to_s does not mutate.

Example because code == good:

  # From
  def test_wrap_around_at_midnight
    skip
    clock = Clock.at(23, 30) + 60
    assert_equal '00:30', clock.to_s
  end

  # To
  def test_wrap_around_at_midnight
    skip
    clock = Clock.at(23, 30) + 60
    assert_equal Clock.at(00, 30), clock
  end

I also still feel strongly that the addition operator should not be used like this, because it goes against ruby style to have self-mutating operators and because addition here is non-commutative. I'd much rather see a named method used or a check against mutation. It's worth pointing out that ruby's core classes all implement addition in various ways that are also non-commutative, but are very much non-mutating as well.

[kotp]

Code examples are likely going to be on the exercism.io site, in Go Track (reference: exercism/problem-specifications#166)

Changed state comment, history of discussion, ref: below...

I guess looking at my wind up clock, when I add 15 minutes to it, the state of the clock changes. So I am using the arrow that has a plus on it that indicates that I am adding minutes (or hours if I pull it out) that tells me I am changing the state of the clock. I can't think of any time I have laid hands on my clock to add minutes that it simply returns a new time without changing the state of the clock.

[kytrinyx]

@remcopeereboom this was just the one problem about equality--not that they checked identity instead of equality but that equality failed. I totally misrepresented the problem--they weren't mutating the clock, they were just computing the correct display value based on the desired test output... this meant that they could have two clocks that were not equal, but displayed the same value.

Here's an example:

package clock

import "fmt"

const testVersion = 3

type Clock struct {
    hour   int
    minute int
}

func New(hour, minute int) Clock {
    return Clock{hour: hour, minute: minute}
}

func (c Clock) String() string {
    hour := c.hour
    minute := c.minute

    for minute < 0 {
        hour -= 1
        minute = minute + 60
    }

    for minute >= 60 {
        minute -= 60
        hour += 1
    }

    for hour >= 24 {
        hour -= 24
    }

    for hour < 0 {
        hour = hour + 24
    }

    c.hour = hour
    c.minute = minute

    return fmt.Sprintf("%02v:%02v", c.hour, c.minute)
}

func (c Clock) Add(minutes int) Clock {
    c.minute += minutes
    return c
}

[remcopeereboom]

@kytrinyx

Got it, thanks! Seems to me handling equality in that way is a valid if perhaps inelegant thing to do.

[kytrinyx]

No, the clocks aren't equal they only look equal. They fail with clock1 == clock2.

[remcopeereboom]

@kytrinyx

Yes I understood that, I should have been more clear perhaps. I mean that implementing the conversion to limit minutes to 0-60 and hours to 0-24 in the string method and then overriding equality to compare the strings should be okay (in the ruby version at least, where you can't just read the hours and minutes).

[remcopeereboom]

@kotp

Might it make sense to add hour and minute attributes to the clock in the tests?

[kytrinyx]

@remcopeereboom Aaah, yeah, that could work.

[kotp]

@kytrinyx The clocks are ending up == and === in the tests. I was surprised about the ===.

Of course their object id's are different.

[kotp]

I have the generator data being used for the tests, now, see the referenced commit above. Did not address the comments here, necessarily though, wanted to get the generator in place.