5
\$\begingroup\$

Input - Standard input - HH:MM:SS[AM|PM] format.

Examples:

  • 12:00:00AM
  • 12:00:00PM
  • 01:00:10AM
  • 01:00:10PM

The program reads one line per instance.

Output - military format (24 hour format):

  • 00:00:00
  • 12:00:00
  • 01:00:10
  • 13:00:10

using System;

class Solution {
    static void Main(String[] args) {
        var time = Console.ReadLine().Trim();
        MilitaryTime mt = new MilitaryTime(time);
        Console.WriteLine(mt);
    }

    class MilitaryTime{
        string newTime = null;
        string originalTime = null;

        string correctHour(string time){
            var ampmLen = 2;
            var ampm = time.Substring(time.Length-ampmLen, ampmLen);
            var hourIndex = 0;
            var hour = time.Split(':')[hourIndex];
            var h = hour;
            if(ampm.Equals("PM")){
                h = (int.Parse(hour) + 12).ToString();
            }
            if(hour.Equals("12") || hour.Equals("24")){
                if(ampm.Equals("AM")){
                    h = "00";
                } else if(ampm.Equals("PM")){
                    h = "12";
                }
            }
            return h;
        }
        string minutesAndSeconds(string time){
            var ampmLen = 2;
            var hourLen = 2;
            var startIndex = hourLen; // since we are taking after hour HH:MM:SS 
            return time.Substring(startIndex, time.Length - ampmLen - hourLen);
        }
        public MilitaryTime(string time){
            originalTime = time;
        }
        public override string ToString(){
            if(string.IsNullOrEmpty(newTime)){
                newTime = correctHour(originalTime) + minutesAndSeconds(originalTime);
            }
            return newTime;
        }
    }
}
  1. Is this code smelly?
  2. I re-factored code to this state, but did I miss anything?
  3. Is there a better way to solve this problem, using .net classes and features I might have missed?
  4. Does it look like that it's written by someone junior or new to programming? If yes, then how would an experienced programmer write it?
\$\endgroup\$
  • 1
    \$\begingroup\$ Reinventing the wheel because you do not want to use DateTime.TryParseExact()? Can I ask why? It does more. Also I would store time as...time, not string. Moreover no need to also store original string, your function is reversible. Finally if you just store a long why it's a class instead of a struct? Very last point: if it does only formatting you should consider to make IFormatter instead of another time class... \$\endgroup\$ – Adriano Repetti Nov 9 '15 at 6:41
  • \$\begingroup\$ In short what I mean is: do not confuse/mix presentation with storage. 12/24 hours is a presentation details. Do not use if for storage. \$\endgroup\$ – Adriano Repetti Nov 9 '15 at 9:52
  • 2
    \$\begingroup\$ @AdrianoRepetti Your comment will make for a good answer. \$\endgroup\$ – Anubhav Nov 9 '15 at 10:40
  • \$\begingroup\$ Well it's more a question itself than an answer (and honestly too short to become an answer...) \$\endgroup\$ – Adriano Repetti Nov 9 '15 at 11:07
5
\$\begingroup\$

Here's how to parse dates and times using built-in .Net libraries:

static void Main()
{
    string strTime = "01:00:10AM";
    DateTime time;
    if (DateTime.TryParseExact(strTime, "hh:mm:sstt", null, 
        System.Globalization.DateTimeStyles.None, out time))
    {
        Console.WriteLine("Parsed \"{0}\" as {1:HH\\:mm\\:ss}", strTime, time);
    }
    else
    {
        Console.WriteLine("Could not parse \"{0}\" as a DateTime", strTime);
    }
    Console.ReadLine();
}

Some points to note:

  • I'm not reinventing the wheel. The .Net DateTime library is the best way to handle points in time. (Use TimeSpan for duration)
  • I'm using custom DateTime format strings for both parsing the string and displaying the parsed DateTime result.
  • I'm using the TryParse pattern the .Net supplies for converting strings to various built-in types: e.g., int.TryParse, double.TryParse, etc.
  • In the output, I want to display the parsed time as HH:mm:ss (note that capital H is 24 hour time and lowercase h is 12 hour time). I have to escape the colons in there because of how string.Format works. You escape colons using backslashes like this HH\:mm\:ss. The problem is that you then have to escape the backslashes (or use verbatim strings). You do that with an extra backslash like this HH\\:mm\\:ss
\$\endgroup\$
  • \$\begingroup\$ Code example can do without else part or throw an exception. \$\endgroup\$ – Anubhav Nov 9 '15 at 19:49
  • 1
    \$\begingroup\$ @AnubhavSaini, it's better to rely on conditionals than on exceptions for program flow. Exceptions are slow by comparison. Conditionals make your intent more clear. Your original code can throw exceptions in quite a few places and it could be a bit of work to figure out why and to fix it without causing problems elsewhere. If you really wanted to use my answer, but throw an exception if something goes wrong, use ParseExact() instead of TryParseExact() \$\endgroup\$ – user2023861 Nov 9 '15 at 19:54
  • \$\begingroup\$ I agree with slowness. But consider this: I am expecting a correct time string out of it. Instead, I'm getting a warning message that doesn't help much with input or output except saying "I did not work as expected." \$\endgroup\$ – Anubhav Nov 9 '15 at 20:42
  • \$\begingroup\$ If program just crashes, I'd have incentive to pass welformed input, test rigorously, and be done quicklier. \$\endgroup\$ – Anubhav Nov 9 '15 at 20:43
  • \$\begingroup\$ @AnubhavSaini, if you can be sure that the input will be well-formed always and forever, then go ahead and use the ParseExact() method. That has the feel of a junior programmer solution however. Where is your input coming from? Is a user typing in a string? That's error-prone. Are you reading it from a file? How much control do you have over the file format? Will it ever change? Are you reading it from a database? Even then the format could change. Your code should fail gracefully and TryParseExact() allows this. \$\endgroup\$ – user2023861 Nov 9 '15 at 21:13
4
\$\begingroup\$

Is this code smelly?

A little bit, yes: It has too many conditionals, which could be fewer and simpler, I think.

I re-factored code to this state, but did I miss anything?

Just one detail: correctHour should be left-padded with zeroes.

Does it look like that it's written by someone junior or new to programming?

Yes, because of some little details. Specially when you repeat constant values:

var amppmlen=2
var hourLen=2

Or when you execute some time-cost operations as Split in several fields which you use just the fist of them. Instead, it could be replaced by a simple Substring operation.

Is there a better way to solve this problem, using .net classes and features I might have missed? How would an experienced programmer write it?

(I joint this two questions to answer them together).

Well, I think the .NET runtime does more things, like validating the input string has the right format, but you seem to assume the input string has already a right format. In this case, you have a good chance to provide your own way to transform the hour representation, in a more optimized way than the .NET runtime.

An experienced programmer would split the main amount of logic into two functions: one for parsing the initial string, and thus obtaining the important fields just once, and another to transform the hours.

Also, if performance is an issue, you should do your best to shorten the algorithm and avoid unnecessary operations: In the correctHour method you should receive the hour field and the ampm field as input parameters (as said), and then transform the hour in just two operations, which might be like this:

var increment = (ampm.equals("PM") ? 12 : 0);
hour = hour + increment;

BTW: I don't think hour can ever be equals to "24". Can it?

\$\endgroup\$
  • \$\begingroup\$ I've improved my suggestions in my answer. \$\endgroup\$ – Little Santi Nov 9 '15 at 9:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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