5
\$\begingroup\$

I have a program which is supposed to convert standard time to traditional time. (e.g: 15:24:31 = 3:25PM [rounded seconds])

I would like for someone to error check my code for me as well as give me ideas on how to condense it.

I'm not sure if I'm allowed to post a program here because of safety issues but here is the exe.

Here is the program. Excuse the spelling mistakes regarding "Standared".

  private void button1_Click(object sender, EventArgs e)
{
    double Hours = double.Parse(textBox1.Text);
    double Minutes = double.Parse(textBox2.Text);
    double Seconds = double.Parse(textBox3.Text);

    if (Hours > 24 || Minutes >= 60 || Seconds >= 60)
    {
        label1.Text = "Please type in proper values.";
    }
    else if (Hours == 0 && Minutes == 0 && Seconds == 0)
    {
        label1.Text = "The traditional time is 12:00 AM";
    }
    else if (Hours == 0 && Minutes < 10)
    {
        label1.Text = "The traditional time is 12:0" + Minutes + " AM";
    }
    else if (Hours == 23 && Minutes == 59 && Seconds >= 30)
    {
        label1.Text = "The traditional time is 12:00 AM";
    }
    else if (Hours == 12 && Minutes == 59 && Seconds >= 30)
    {
        label1.Text = "The traditional time is 1:00 PM";
    }
    else
    {
        label1.Text = "The traditional time is " + Converter(Hours);
    }
}
public string Converter(double Input)
{
    String Final;
    double H = double.Parse(textBox1.Text);
    double M = double.Parse(textBox2.Text);
    double S = double.Parse(textBox3.Text);
    string AMPM = "";
    if (M == 59 && S == 59)
    {
        H = H + 1;
        M = 0;
        S = 0;
    }
    if (H == 12)
    {
        AMPM = "PM";
    }
    else if (H == 24)
    {
        H = 12;
        AMPM = "AM";
    }
    else if (H > 12)
    {
        H = H - 12;
        AMPM = "PM";
    }
    else if (H < 12)
    {
        AMPM = "AM";
    }         
    if (S >= 30)
    {
        S = 1;
        if (S==1)
        {
            M = S + M;
        }
    }
    else
    {
        S = 0;
    }

    if (M == 0)
    {
         Final = H.ToString() + ":" + M.ToString() + "0 " + AMPM;
    }
    else if (M == 1 || M == 2 || M == 3 || M == 4 || M == 5 || M == 6 || M == 7 || M == 8 || M == 9)
    {
        Final = H.ToString() + ":0" + M.ToString() + " " + AMPM;
    }
    else
    {
        Final = H.ToString() + ":" + M.ToString() + " " + AMPM;
    }


    return Final;
}
}
}
\$\endgroup\$
5
  • \$\begingroup\$ Can you say with confidence whether or not your code works? \$\endgroup\$
    – nhgrif
    Commented Nov 26, 2014 at 21:47
  • \$\begingroup\$ @nhgrif I don't see any reason to suspect that it's broken. \$\endgroup\$ Commented Nov 26, 2014 at 22:06
  • \$\begingroup\$ @200_success I would like for someone to error check my code \$\endgroup\$
    – nhgrif
    Commented Nov 26, 2014 at 22:07
  • 2
    \$\begingroup\$ Are you aware of datetime formatting or were you prohibited of using this? \$\endgroup\$ Commented Nov 26, 2014 at 22:08
  • \$\begingroup\$ So the user is supposed to input standard time and the program spits out the time in traditional. The code is working but there may be some errors during the conversion \$\endgroup\$ Commented Nov 26, 2014 at 22:26

3 Answers 3

4
\$\begingroup\$

DateTime handling

It's possible that you're prohibited from using the DateTime struct in this method since this is apparently homework. But, in case you're not aware, problems which are as common as parsing and formatting dates and times generally already have very sophisticated and well-designed solutions. In this case, it's .NET's System.DateTime, in particular the overload of ToString which takes a format string, and TryParse. You can find these both documented on the msdn site. Using that, I believe you would be able to condense all your code down to just a few lines.

Naming

By convention, variables local to methods are named in camelCase- i.e. the first letter should be lower case. So Hours should be hours, etc. This also applies for method parameters- Input should be input.

You seem to be using the default naming for your controls- label1, button1, etc. These should be given descriptive names, like all other variables.

You change conventions inside Converter. Why is something which was Hours in a different method now H? Prefer the more descriptive name (hours, once the casing is fixed).

Another convention is that classes should be named as nouns, and methods as verbs. So instead of Converter, you should use Convert. Often, something in the form VerbNoun is the most descriptive: ConvertTime, for example.

Numeric types

Looking at your code, it seems like your hours, minutes and seconds are expected to be integers. So why are you parsing them into doubles? This is going to lead to all sorts of potential issues- rounding can cause unexpected results when doing equality comparisons for example. If you want a variable which only stores whole numbers, it's very important you use an integer type- generally int unless you have a good reason to pick another.

Cleaning up

Okay, here's the big one. It's pretty unpleasant how button1_Click handles a bunch of special cases, then if it's not any of those, hands over to Converter, which itself has a whole lot of complex logic. Let's work out how we can do this better.

As I said earlier, the best way to do this is to use DateTime, but that's not really saying much useful about the code you wrote, and for all I know you may have a requirement not to use those in-built methods. So I'll work on the assumption you have to do all the tricky time handling yourself.

Probably the first thing to notice is that the handling of seconds, minutes and hours are almost completely independent from each other. Here's our aim for each one:

  • Seconds: Decide whether we need to round up to the next minute
  • Minutes: Add one if we've rounded up the seconds. Check for the special case that we went from 59 minutes to 60, in which case we need to tick over to the next hour.
  • Hours: Add one if we rounded up a minute from 59. Decide whether it's AM or PM. If it's PM, subtract 12 from the number.

Let's address these one by one.

Seconds

As you can see from the description above. Seconds are really simple. We really only need to decide whether they're greater than or equal to 30, then we can forget about them. But look at how much complexity they have in the code:

if (M == 59 && S == 59)

and

if (S >= 30)
{
    S = 1;
    if (S==1)
    {
        M = S + M;
    }
}
else
{
    S = 0;
}

Plus the repeated individual checks in button1_click.

So forget all that, we want a super-simple way to ask "do I need to round up my minutes?":

private bool DoMinutesRequireRoundUp(int seconds)
{
    return seconds >= 30;
}

There we go, that's all we need for seconds!

Minutes

Now we get a bit more complex. We have two questions to ask here:

Will rounding up minutes put us into the next hour? What will the minutes value be after we've done any necessary rounding of seconds?

In your code you've handled this with a bunch of special cases sprayed all over the place of minutes being 59. But again, there's no need for that!

Let's start with the first, again as its own method:

private bool DoMinutesRoundUpToNextHour(int minutes, bool roundUpMinutes)
{
    return roundUpMinutes && minutes==59;
}

Now we have that, we're better equipped to tackle the second

private int GetRoundedMinutes(int minutes, bool roundUpMinutes)
{
    if(DoMinutesRoundUpToNextHour(minutes, roundUpMinutes))
    {
        return 0;
    }
    if(roundUpMinutes)
    {
        minutes++;
    }
    return minutes;
}

See how we've separated out this question from any concern of what exactly the seconds are, or- even more irrelevant- what the hours are.

Hours

And finally, the hours! In your code, you try to do things in a very mixed-up way, in which AM and PM checking is all jumbled up with subtracting 12, rounding minutes, and so on. There are a few steps here, but actually we can address them quite separately as long as we do them in the right order.

Here's what we need to do, and note how each task may be dependent on tasks before it, but not after it:

  1. Add one if the minutes were rounded up from 59 to 00
  2. Subtract 24 if the hours are greater than 23
  3. Decide whether we should display "AM" or "PM"
  4. Subtract 12 if the hours are greater than 12

Not trivial, but taking them one at a time, they're not too bad either. First, numbers 1 and 2:

private int GetRoundedHours(int hours, bool incrementHours)
{
    if(incrementHours)
    {
        hours++;
    }
    return hours % 24;
}

Then 3:

private string GetPeriod(int hours)
{
    return hours < 12 ? "AM" : "PM";
}

And finally 4:

private int Get12HourTime(int hours)
{
    return hours <= 12 ? hours : hours - 12;
}

Gluing it together

So that's a bunch of methods! But now putting it all together is very easy. Let's look at how they'd work in Convert.

First, I'm going to fix the Input thing. This is pretty confusing, as it's never used. Instead I'm going to have the time elements actually passed in and used.

private string ConvertTime(int seconds, int minutes, int hours)
{
    var roundUpMinute = DoMinutesRequireRoundUp(seconds);
    var incrementHour = DoMinutesRoundUpToNextHour(minutes, roundUpMinute);
    var roundedHour = GetRoundedHours(hours, incrementHour);
    
    var displayMinutes = GetRoundedMinutes(minutes, roundUpMinute);
    var displayHours = Get12HourTime(roundedHour);
    var period = GetPeriod(roundedHour);

    return displayHours.ToString() + ":" + displayMinutes.ToString() + " " + period;
}

One thing this doesn't address is the leading zero for minutes < 10, but this can very easily be done in a few extra lines.

It's hard to do a line-by-line comparison of this to your version, because it's quite structurally different, but the key is to see how we've separated out each individual question we need answered into its own piece of code, and decoupled those questions from each other. We don't do a check like:

if (Hours == 23 && Minutes == 59 && Seconds >= 30)

Because that's conflating 3 different questions ("Do we need to round up to the next minute?", "Do we need to round up to the next hour?", "Do we need to round up to the next day"?) which we should answer separately.

\$\endgroup\$
2
  • \$\begingroup\$ Here where you wrote: return hours <= 12 ? hours : hours - 12; gives an error saying cannot convert int to string. Here where you wrote: var roundedHour = GetRoundedHours(hours, roundUpHour); says roundUpHour does not exist \$\endgroup\$ Commented Nov 27, 2014 at 0:40
  • \$\begingroup\$ This is my issue now: imgur.com/ZCxOEuV \$\endgroup\$ Commented Nov 27, 2014 at 4:07
3
\$\begingroup\$

var keyword:

Although others might not prefer to, use var instead of declaring your variables explicitly. Let the compiler do this for you and your code will look cleaner.

Naming conventions:

Local variables and fields in methods and classes use pascalCase. So, in button1_Click, Hours will become hours and so on.

Naming of controls:

What does textBox1 or button1 mean? Right, nothing! Change the name to what the control represents. For example this would become HoursTextBox and for the button ConvertButton.

Parsing:

If I type text in the textbox (non-numeric input), your code will throw a FormatException with following message:

Input string was not in a correct format.

This is because you don't validate and/or catch possible exceptions. To capture the input correctly use the Double.TryParse() method:

double hours, minutes = 0, seconds = 0;

var isValidInput = Double.TryParse(HoursTextBox.Text, out hours) &&
                   Double.TryParse(MinutesTextBox.Text, out minutes) &&
                   Double.TryParse(SecondsTextBox.Text, out seconds);

if (isValidInput)
{
    //Process input
}

Cleaner check for equality:

Instead of this line:

else if (hours == 0 && minutes == 0 && seconds == 0)

Create a method that checks if all input fields are equal to a given value:

private static bool AllEqualTo(double equalTo, params double[] values)
{
    return values.All(x => x == equalTo);
}

//Usage:
else if (AllEqualTo(0.0, hours, minutes, seconds))

Make sure you add the System.Linq namespace for using the Enumerable.All() method.

using System.Linq;

Converter method:

Here you can apply previous tips like the naming, using of var, the use of the AllEqualTo method...

You pass the hours to the method but you never use it, plus you parse the values of the textboxes again. Just pass them from the button click event and use the values, there's no use in parsing them again.

In following:

if (s >= 30)
{
    s = 1;
    if (s == 1)
    {
        m = s + m;
    }
}

Setting s to 1 and then checking if it is 1, is kind of ridiculous. Just change it to this:

if (s >= 30)
{
    s = 1;
    m = s + m;
}

Following check:

else if (m == 1 || m == 2 || m == 3 || m == 4 || m == 5 || m == 6 || m == 7 || m == 8 || m == 9)

can be replaced by this:

else if (m >= 1 && m <= 9)

String.Format

Don't concatenate variables with hardcoded values like that, use the String.Format() method instead. Following code:

if (m == 0)
{
    final = h.ToString() + ":" + m.ToString() + "0 " + ampm;
}
else if (m == 1 || m == 2 || m == 3 || m == 4 || m == 5 || m == 6 || m == 7 || m == 8 || m == 9)
{
    final = h.ToString() + ":0" + m.ToString() + " " + ampm;
}
else
{
    final = h.ToString() + ":" + m.ToString() + " " + ampm;
}

will become:

if (m == 0)
{
    final = String.Format("{0}:00 {1}", h, ampm);
}
else if (m >= 1 && m <= 9)
{
    final = String.Format("{0}:0{1} {2}", h, m, ampm);
}
else
{
    final = String.Format("{0}:{1} {2}", h, m, ampm);
}

Above code is for inserting a leading zero when the minutes variable is smaller than 10. This this can be shortened to following code:

var finalMinute = (m >= 0 && m <= 9) ? String.Format("0{0}", m) : m.ToString();
final = String.Format("{0}:{1} {2}", h, finalMinute, ampm);

The final code of the converter method will look like this:

public string Converter(double hours, double minutes, double seconds)
{
    var ampm = "";

    if (AllEqualTo(59, minutes, seconds))
    {
        hours = hours + 1;
        minutes = 0;
        seconds = 0;
    }
    if (hours == 12)
    {
        ampm = "PM";
    }
    else if (hours == 24)
    {
        hours = 12;
        ampm = "AM";
    }
    else if (hours > 12)
    {
        hours = hours - 12;
        ampm = "PM";
    }
    else if (hours < 12)
    {
        ampm = "AM";
    }
    if (seconds >= 30)
    {
        seconds = 1;
        minutes = seconds + minutes;
    }

    var finalMinute = (minutes >= 0 && minutes <= 9) ?
                      String.Format("0{0}", minutes) :
                      minutes.ToString();
    var final = String.Format("{0}:{1} {2}", hours, finalMinute, ampm);
    return final;
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ If you use a format of {0:0#}, you don't have to check the value of the number - anything 0-9 will automatically pre-pend the zero. See Custom Numeric Format Strings. \$\endgroup\$
    – Dan Lyons
    Commented Nov 26, 2014 at 23:37
3
\$\begingroup\$

There are some bugs. Here are some sample inputs where the code gives the wrong answer

 h,  m,  s | Expected |   Got
--------------------------------
 0,  0, 31 | 12:01 AM | 12:00 AM
 0, 45, 36 | 12:46 AM |  0:46 AM
 5, 59, 52 |  6:00 AM |  5:60 AM
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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