6
\$\begingroup\$

Please review my code and let me know of any additions or modifications that would make this even more useable.

The initial idea was to create a simple Time class that has only one function: To represent an instant in time.

I've created this mainly because I found NodaTime to be way more than I needed.

So here it is, the Time class:

public class Time
{
    private int _hour, _minute, _second;

    public Time(int hour, int minute, int second)
    {
        Hour = hour;
        Minute = minute;
        Second = second;
    }

    #region ToString methods

    public override string ToString() {
        return string.Format("{0}:{1}:{2}", this.Hour.ToString().PadLeft(2, '0'), this.Minute.ToString().PadLeft(2, '0'), this.Second.ToString().PadLeft(2, '0'));
    }

    public string ToString(string format)
    {
        string preFormat = format.Replace("hh", "{0}")
                                 .Replace("mm", "{1}")
                                 .Replace("ss", "{2}")
                                 .Replace("h", "{3}")
                                 .Replace("m", "{4}")
                                 .Replace("s", "{5}");
        return string.Format(preFormat, 
                                 this.Hour.ToString().PadLeft(2, '0'), 
                                 this.Minute.ToString().PadLeft(2, '0'), 
                                 this.Second.ToString().PadLeft(2, '0'),
                                 this.Hour.ToString(),
                                 this.Minute.ToString(),
                                 this.Second.ToString());
    }

    #endregion

    #region Properties

    public int Hour
    {
        get { return _hour; }
        set
        {
            // fix out of range values
            if (value < 0)
            {
                value = 24 - System.Math.Abs(value % 24);
            }
            if (value > 24)
            {
                value = System.Math.Abs(value % 24);
            }

            _hour = value;
        }
    }

    public int Minute
    {
        get { return _minute; }
        set
        {
            // fix out of range values
            if (value < 0)
            {
                Hour -= (int)Math.Floor(System.Math.Abs((double)value / 60));
                value = 60 - System.Math.Abs(value % 60);
            }
            if (value > 59)
            {
                Hour += System.Math.Abs((int)Math.Floor((double)value / 60));
                value = System.Math.Abs(value % 60);
            }
            _minute = value;
        }
    }

    public int Second
    {
        get { return _second; }
        set
        {
            // fix out of range values
            if (value < 0)
            {
                Minute -= (int)Math.Floor(System.Math.Abs((double)value / 60));
                value = 60 - System.Math.Abs(value % 60);
            }
            if (value > 59)
            {
                Minute += System.Math.Abs((int)Math.Floor((double)value / 60));
                value = System.Math.Abs(value % 60);
            }
            _second = value;
        }
    }

    #endregion

    #region Static Methods

    /// <summary>
    /// Parses a string in the form of 10:25:45 to a Time instance
    /// </summary>
    /// <param name="time">e.g. 10:25:45</param>
    /// <param name="result">a boolean indicating the parsing succeeded</param>
    /// <returns></returns>
    public static bool TryParse(string time, out Time result)
    {
        Contract.Requires<ArgumentNullException>(time != null, "Can not parse null to Time");

        var parts = time.Split(':');
        int hour = 0;
        int minute = 0;
        int second = 0;
        if (parts.Length > 0)
            int.TryParse(parts[0], out hour);
        if (parts.Length > 1)
            int.TryParse(parts[1], out minute);
        if (parts.Length > 2)
            int.TryParse(parts[2], out second);

        result = new Time(hour, minute, second);

        return true;
    }

    /// <summary>
    /// Parses a string in the form of 10:25:45 to a Time instance 
    /// </summary>
    /// <param name="time">e.g. 10:25:45</param>
    /// <returns>an instance of Time</returns>
    public static Time Parse(string time)
    {
        Time output;
        Time.TryParse(time, out output);

        return output;
    }

    #endregion

    #region Operators

    public static implicit operator Time(string time)
    {
        return Time.Parse(time);
    }

    public static Time operator +(Time first, Time second)
    {
        return new Time(first._hour + second._hour, first.Minute + second.Minute, first.Second + second.Second);
    }

    public static Time operator -(Time first, Time second)
    {
        return new Time(first._hour - second._hour, first.Minute - second.Minute, first.Second - second.Second);
    }

    #endregion
}

My Unit tests to validate all it's functions:

[TestClass]
public class TimeTests
{
    [TestMethod]
    public void CreateTimeFromString()
    {
        string timeString = "10:30:45";
        Time time = Time.Parse(timeString);
        Assert.IsTrue(time.Hour == 10 && time.Minute == 30 && time.Second == 45);
    }

    [TestMethod]
    public void CreateEmptyTimeFromString()
    {
        string timeString = "";
        Time time = Time.Parse(timeString);
        Assert.IsTrue(time.Hour == 0 && time.Minute == 0 && time.Second == 0);
    }

    [TestMethod]
    public void CreateTimeFromWeirdString()
    {
        string timeString = "8:7:9";
        Time time = Time.Parse(timeString);
        Assert.IsTrue(time.Hour == 8 && time.Minute == 7 && time.Second == 9);
    }

    [TestMethod]
    public void CreateTimeFromWeirdButNotValidString()
    {
        string timeString = "not a time";
        Time time = Time.Parse(timeString);
        Assert.IsTrue(time.Hour == 0 && time.Minute == 0 && time.Second == 0);
    }

    [TestMethod]
    [ExpectedException(typeof(ArgumentNullException))]
    public void CreateTimeFromNullString()
    {
        string timeString = null;
        Time time = Time.Parse(timeString);
    }

    [TestMethod]
    public void AddTimeInstancesTogether()
    {
        Time time = new Time(1, 2, 3);
        time += new Time(1,2,3);
        Assert.IsTrue(time.Hour == 2 && time.Minute == 4 && time.Second == 6);
    }

    [TestMethod]
    public void SubtractTimeInstances()
    {
        Time time = new Time(1, 2, 3);
        time -= new Time(1, 2, 3);
        Assert.IsTrue(time.Hour == 0 && time.Minute == 0 && time.Second == 0);
    }

    [TestMethod]
    public void TestInvalidTimeInstanceThroughSubtraction()
    {
        Time first = new Time(1, 0, 0);
        first -= new Time(2, 1, 1);
        Assert.IsTrue(first.Hour == 23 && first.Minute==59 && first.Second == 59);
    }

    [TestMethod]
    public void TestStringDefaultFormat()
    {
        Time first = new Time(1, 0, 0);
        Assert.IsTrue(first.ToString()=="01:00:00");
    }

    [TestMethod]
    public void TestStringCustomFormat()
    {
        Time first = new Time(1, 0, 0);
        Assert.IsTrue(first.ToString("hh:mm:ss h:m:s") == "01:00:00 1:0:0");
    }
}
\$\endgroup\$
  • 6
    \$\begingroup\$ No offense, but your code is a LOT more confusing than DateTime.ParseExact(s, "H:m:s", null);. It's also more likely to have bugs (like in your code 0h:30m:0s + 0h:30m:0s = 0h:0m:0s, but DateTime gets it right). \$\endgroup\$ – Matt Dec 12 '13 at 12:18
  • 7
    \$\begingroup\$ @PeterKiss: Use a TimeSpan to represent a duration of time. Use DateTime to represent a point in time. \$\endgroup\$ – Matt Dec 12 '13 at 12:41
  • 2
    \$\begingroup\$ TimeSpan can be used to point a specific time (point in time is relative) like DateTime structure does with it's TimeOfDay property and also all .NET ORM mapper maps time SQL type to TimeSpan. \$\endgroup\$ – Peter Kiss Dec 12 '13 at 12:47
  • 2
    \$\begingroup\$ Have a look at Noda Time - might have what you're after. (Not used it myself though so it might not...) \$\endgroup\$ – MrKWatkins Dec 12 '13 at 12:59
  • 2
    \$\begingroup\$ Regarding re-inventing the wheel... date and time is one of those topics which seems simple but is incredibly hard to get right. Therefore there is a lot of merit in attempting to roll your own, even if it's only to better appreciate the immense complexity of the subject - complexity which every software developer needs to be aware of. \$\endgroup\$ – MattDavey Dec 12 '13 at 19:34
6
\$\begingroup\$
private int _hour, _minute, _second;

I'd suggest to not declare multiple variables in the same line for clarity.

private int _hour;
private int _minute;
private int _second;

In case you prefixed them so that they don't collide in the constructor, that's unnecessary, as you always get the parameter except if you use this.

private int value;
public Constructor(int value)
{
    this.value = value;
}

public override string ToString() {
    return string.Format("{0}:{1}:{2}", this.Hour.ToString().PadLeft(2, '0'), this.Minute.ToString().PadLeft(2, '0'), this.Second.ToString().PadLeft(2, '0'));
}

You could use your own formatting method:

public override string ToString() {
    return ToString("hh:mm:ss");
}

Are you aware that your TryParse will never fail? That's a bad design, as other developers might implement it like this:

String userInput = ...;
Time time;
if (!Time.TryParse(userInput, out time))
{
    // Will never happen!
}

Either replace it with something different (like a Parse which states in it's documentation that it will never fail and instead return an 'empty' Time), or make it fail if it can't be parsed.

At the moment there's no way to check for an error during parse (because input could have been "00:00:00", which is a valid time after all).


Food for thoughts: Consider making the class immutable, as DateTime and TimeSpan are both immutable.

|improve this answer|||||
\$\endgroup\$
  • 5
    \$\begingroup\$ There is no official naming convention for private fields. I've seen no prefix, _ and m_, and I think all are acceptable. \$\endgroup\$ – svick Dec 12 '13 at 13:58
  • \$\begingroup\$ @svick: True, I just looked it up, private fields are exclusively excluded. Will rephrase that part. \$\endgroup\$ – Bobby Dec 12 '13 at 14:02
  • 2
    \$\begingroup\$ @Bobby: It's somewhat shorter to write _value = value than this.value = value. Also the human brain tends to be better at pattern matching than scoping, so prefixed variables are easier to see as class members rather than having to "parse" the this. and make the connection from there. In the end I guess it comes down to personal preference. \$\endgroup\$ – ChrisWue Dec 13 '13 at 2:31
5
\$\begingroup\$

I agree with others that to represent a point in time within a day, you should use TimeSpan, just like DateTime.TimeOfDay does. Or maybe a custom class that's a simple wrapper around TimeSpan (to forbid times outside of the [0, 24h) range).

Anyway, here is my review of your code (I'm not going to repeat things mentioned by others):

public string ToString(string format)
{
    string preFormat = format.Replace("hh", "{0}")
                             .Replace("mm", "{1}")
                             .Replace("ss", "{2}")
                             .Replace("h", "{3}")
                             .Replace("m", "{4}")
                             .Replace("s", "{5}");
    return string.Format(preFormat, 
                             this.Hour.ToString().PadLeft(2, '0'), 
                             this.Minute.ToString().PadLeft(2, '0'), 
                             this.Second.ToString().PadLeft(2, '0'),
                             this.Hour.ToString(),
                             this.Minute.ToString(),
                             this.Second.ToString());
}
  • This code is pretty fragile (what if format already contains {0}? though that's probably quite unlikely) and also not general enough (how would you represent format like “19 h 36 min”?).

  • I think you should use numeric format strings instead of PadLeft(), e.g. {0:d2}. When you do that, you can then omit all those ToString()s and also specify each property just once in the call to Format().

  • All those Replace()s produce some unnecessary garbage. If that becomes a problem, use a StringBuilder instead (it also has a Replace() method).

public int Hour
{
    get { return _hour; }
    set
    {
        // fix out of range values
        if (value < 0)
        {
            value = 24 - System.Math.Abs(value % 24);
        }
        if (value > 24)
        {
            value = System.Math.Abs(value % 24);
        }

        _hour = value;
    }
}
  • Are you sure you want to treat -12 and 36 exactly the same as 12? Consider making values outside of the allowed range into errors instead.

    If you want to support things like adding hours that overflows midnight, a method like AddHours() might be better than time.Hours += x.

  • You can write just Math, no need to spell out the namespace.

  • I don't see why are you using Math.Abs() so much. You already know the sign of the number, so you can just write 24 + value % 24 (when you know that value is negative) or value % 24 (when you know that it's positive).

Minute -= (int)Math.Floor(System.Math.Abs((double)value / 60));

That's a mouthful. You could write just Minute += value / 60 instead.

if (parts.Length > 0)
    int.TryParse(parts[0], out hour);

Does this mean that empty string is a valid time string? I'm not sure that's a good choice.

public static implicit operator Time(string time)

I'm not sure about this. Most types don't have such conversions, probably mostly because calling Parse() is more explicit. And you should use TryParse() most of the time anyway.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback. You've given me some new insights to think about and that's what I'm here for. \$\endgroup\$ – Peter Dec 13 '13 at 7:19
3
\$\begingroup\$

You are re-inventing a true and tested wheel. When doing so you need to remember that your wheel won't be as tested as the wheel everyone else is using, and thus more likely to hold hidden bugs. You also need to remember that your wheel looks exactly like every other wheel with no apparent benefit... Now, I can see how this would be useful if your usecase means you cannot implement the use of:

  • An immutable or constant DateTime. You mentioned you needed something to represent an instance of any given time. By creating an immutable or a constant DateTime object you are now effectively representing a fixed instance of some time.
  • A DateTime wrapper with a single read-only instance of a DateTime to make sure that once instantiated (and for as long as the wrapper reference does not change or is re-assigned) the internal DateTime will never change.

Your code doesn't seem to meet these (admittedly, arbitrarily) bullet points. I'd love to discuss a specific user case for why you cannot use any of the aforementioned solutions - this is CodeReview afterall...

|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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