patterncsharpMinor
Rounding and formatting hours, minutes, seconds as HH:MM AM/PM
Viewed 0 times
formattingminuteshourssecondsandrounding
Problem
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 = 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 = 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 ||
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 = 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 = 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 ||
Solution
DateTime handling
It's possible that you're prohibited from using the
Naming
By convention, variables local to methods are named in camelCase- i.e. the first letter should be lower case. So
You seem to be using the default naming for your controls-
You change conventions inside
Another convention is that classes should be named as nouns, and methods as verbs. So instead of
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
Cleaning up
Okay, here's the big one. It's pretty unpleasant how
As I said earlier, the best way to do this is to use
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:
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:
and
Plus the repeated individual checks in
So forget all that, we want a super-simple way to ask "do I need to round up my minutes?":
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:
Now we have that, we're better equipped to tackle the second
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, bu
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, bu
Code Snippets
if (M == 59 && S == 59)if (S >= 30)
{
S = 1;
if (S==1)
{
M = S + M;
}
}
else
{
S = 0;
}private bool DoMinutesRequireRoundUp(int seconds)
{
return seconds >= 30;
}private bool DoMinutesRoundUpToNextHour(int minutes, bool roundUpMinutes)
{
return roundUpMinutes && minutes==59;
}private int GetRoundedMinutes(int minutes, bool roundUpMinutes)
{
if(DoMinutesRoundUpToNextHour(minutes, roundUpMinutes))
{
return 0;
}
if(roundUpMinutes)
{
minutes++;
}
return minutes;
}Context
StackExchange Code Review Q#70955, answer score: 4
Revisions (0)
No revisions yet.