Constructor for a Time object

Question

I am trying to write a constructor method that takes dd, hh, mm and ss as parameters and change it as a normal type of Time struct (e.g you enter 25 hours as the hh parameter, it changes it to 1 day and 1 hour).

struct Time
{
 public Time(int dd, int hh, int mm, int ss)
 { 
 seconds= ss % 60;
 minutes = mm % 60;
 hours = hh % 24;
 days = dd % 7;
 if (ss > 60)
 {
 minutes++;
 }
 if (mm > 60)
 {
 hours++;
 }
 if (hh > 24)
 {
 days++;
 }
 }
 private int days, hours, minutes, seconds;
 public void Write()
 {
 Console.WriteLine("It has been " + days + " days, " + hours + " hours, " + minutes + " minutes, " + seconds + " seconds.");
 }
}
class Program
{
 static void Main(string[] args)
 {
 Time t = new Time(5, 28, 65, 55);
 t.Write();
 Console.ReadKey();
 }
}

The program works, but it feels very simple and amateurish. Is there anything I could do to make it more robust?

• Sam Marion: Simple is what you want in your programs. Never try to make something more robust because you feel like it would make less 'amaturish'. If its easy to read, efficent and does what you want there's nothing wrong with it. I would change it from struct to class though, no real reason to use struct here.
• n8wrl: Score 1 for "works." Score another for simple = easy to maintain. As you use it and test it you'll discover the edges to guard against. Robustness will come.
• Luis Lavieri: Why not use DateTime?
• AliTeo: @LuisLavieri I am trying to improve myself, this is just an exercise.
• Tomas Paul: I don't know if the statement e.g. Time(5, 49, 138, 185) could be called. If yes, you should cover this case. Another exercise for you.

Answer 1

I can see several issues in your code:

1. Days

   I guess the row with code days = dd % 7; is some historic rest of code when converting to weeks (?).

2. Boundaries

   If you call Time(0, 0, 0, 60), the condition if (ss > 60) is not true and the addition minutes++; is not executed. You should use if (ss >= 60). The same error is repeated in other if statements.

3. Incorrect additions

   Try to call your constructor with Time(0, 0, 0, 86400). If my understanding is correct, you should get 1 day, but you get only 1 minute.

The correct logic should be something like this:

public Time(int dd, int hh, int mm, int ss)
{
 seconds = ss % 60;
 minutes = mm % 60;
 hours = hh % 24;
 days = dd;
 if (ss >= 60)
 {
 minutes = mm + ss / 60;
 }
 if (minutes >= 60)
 {
 hours = hh + minutes / 60;
 minutes %= 60;
 }
 if (hours >= 24)
 {
 days = dd + hours / 24;
 hours %= 24;
 }
}

But, as Jamal mentioned, the constructor is for setting data members and other logic should be splitted into other method(s). That's why my example is not perfect from the design point of view. I also used your variable names in my example. I can imagine better names for them.

Answer 2

Why not just spell out all the variable names? Yes, they may seem obvious for a program such as this, but there's not much to gain from shortening them.

Also, consider putting those conditionals into a separate private method, especially if you plan on adding more (months and years, for instance). The constructor should primarily set the data members and call other methods if necessary.

Answer 3

You already know what's wrong with your code, you know about the naming etc. so let me just show you another simpler way to solve this.

There's no need to reinvent the wheel because you can use the TimeSpan struct that can do the job for you.

static TimeSpan CreateTimeSpan(int days, int hours, int minutes, int seconds)
{
 return 
 TimeSpan.FromDays(days) +
 TimeSpan.FromHours(hours) +
 TimeSpan.FromMinutes(minutes) +
 TimeSpan.FromSeconds(seconds);
}

---

In order to create a string from a TimeSpan you can write an extension that does exactly that. If you can use C# 6 then you can also use the new string interpolation.

public static string Stringify(this TimeSpan timeSpan)
{
 return $"It has been {timeSpan.Days} days, {timeSpan.Hours} hours, {timeSpan.Minutes} minutes, {timeSpan.Seconds} seconds.";
}

---

You can make it even more reusable by creating a series of extensions to work with the TimeSpan:

static class TimeSpanExtensions
{
 public static string Stringify(this TimeSpan timeSpan)
 {
 return $"It has been {timeSpan.Days} days, {timeSpan.Hours} hours, {timeSpan.Minutes} minutes, {timeSpan.Seconds} seconds.";
 }
 public static TimeSpan AddDays(this TimeSpan timeSpan, int days)
 {
 return timeSpan + TimeSpan.FromDays(days);
 }
 public static TimeSpan AddHours(this TimeSpan timeSpan, int hours)
 {
 return timeSpan + TimeSpan.FromHours(hours);
 }
 public static TimeSpan AddMinutes(this TimeSpan timeSpan, int minutes)
 {
 return timeSpan + TimeSpan.FromMinutes(minutes);
 }
 public static TimeSpan AddSeconds(this TimeSpan timeSpan, int seconds)
 {
 return timeSpan + TimeSpan.FromSeconds(seconds);
 }
}

So you can use them like this:

var ts = TimeSpan.Zero.AddDays(5).AddHours(28).AddMinutes(65).AddSeconds(55);
Console.WriteLine(ts.Stringify());