HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Class that represents an instant in Time

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
timethatinstantclassrepresents

Problem

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 24)
{
value = System.Math.Abs(value % 24);
}

_hour = value;
}
}

public int Minute
{
get { return _minute; }
set
{
//

Solution

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.

Code Snippets

private int _hour, _minute, _second;
private int _hour;
private int _minute;
private int _second;
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'));
}
public override string ToString() {
    return ToString("hh:mm:ss");
}

Context

StackExchange Code Review Q#37215, answer score: 6

Revisions (0)

No revisions yet.