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

Constructor for a Time object

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

Problem

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?

Solution

I can see several issues in your code:

-
Days

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

-
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.

-
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.

Code Snippets

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;
    }
}

Context

StackExchange Code Review Q#146884, answer score: 4

Revisions (0)

No revisions yet.