Time conversion program optimization

Question

I have just finished working on a problem on hackerrank dealing with time conversion in C++.

I'm still fairly new to programming as I only have a few semesters of programming classes under my belt and had a pretty challenging time with this program.

Below is a link to the problem itself and my code. I solved the problem but I feel like my time complexity is unnecessarily high. It works to solve all the test cases the site provides but I am looking to become a better developer so I want to learn to write more efficient and effective code.

Input Format

A single string containing a time in 12-hour clock format (i.e.:hh:mm:ssAM or hh:mm:ssPM), where 01 <= hh <= 12.

Output Format

Convert and print the given time in 24-hour format, where 00 <= hh <= 23.

Please tell show me how I can optimize this code for a better run time.

Link to the problem

#include <cmath>
#include <cstdio>
#include <vector>
#include <iostream>
#include <algorithm>
#include <sstream>
#include<iomanip>
using namespace std;
int main(){
 string time;
 cin >> time;
 char delim = ':'; // delimiter for extracting colons from input
 stringstream newTime; // stream to read string that holds time info
 newTime << time;
 int hours,minutes,seconds; // variables to hold data
 string ampm;
 newTime >> hours;
 newTime >> delim;
 newTime>> minutes;
 newTime >> delim;
 newTime>> seconds;
 newTime>> ampm;
 if(ampm == "PM"){ // for changing hours to 0-23 scale
 switch(hours){
 case 1:hours = 13;
 break;
 case 2:hours = 14;
 break;
 case 3:hours = 15;
 break;
 case 4:hours = 16;
 break;
 case 5:hours = 17;
 break;
 case 6:hours = 18;
 break;
 case 7:hours = 19;
 break;
 case 8:hours = 20;
 break;
 case 9:hours = 21;
 break;
 case 10:hours = 22;
 break;
 case 11:hours = 23;
 break;
 }
 }
 else if(ampm == "AM" && hours == 12 ){ // for changing 12am to 00am
 hours = 0;
 }
 //use of iomanip functions below to received desired output
 cout << setw(2)<< setfill('0') << hours << ":" << setw(2)<< setfill('0') << minutes << ":" << setw(2)<< setfill('0') << seconds << endl; 
 return 0;
}

Answer 1

If you were writing this for real world use¹, I think the right way would be to use std::get_time and std::put_time, something on this order:

#include <iostream>
#include <iomanip>
#include <ctime>
int main() {
 struct std::tm t;
 std::cin >> std::get_time(&t, "%I:%M:%S%p");
 std::mktime(&t);
 std::cout << std::put_time(&t, "%H:%M:%S");
}

(Note: it's open to question whether the call to mktime is really necessary here, but at worst it's still pretty harmless).

Switch Statement

Looking more specifically at your code, the thing that probably sticks out the most is the big switch statement. I'd eliminate that in favor of a tiny bit of math:

if (ampm == "PM")
 hours += 12;

Reading Delimiters

Right now, the code to initially read the data is made somewhat ugly and unreadable (and arguably failure-prone) by it's somewhat poor handling of delimiters. It currently just reads a character and assumes it's a delimiter. In real life, you probably want to at least compare what you read to what you expected so ensure that the input is correctly formatted (and react appropriately if it's not).

Personally, I think I'd write a little operator overload like this:

std::istream &operator >> (std::istream &is, char const *pat) {
 char ch;
 while (isspace(static_cast<unsigned char>(is.peek())))
 is.get(ch);
 while (*pat && is && *pat == is.peek() && is.get(ch)) 
 ++pat; 
 // if we didn't reach the end of the pattern, matching failed (mismatch, premature EOF, etc.)
 if (*pat) {
 is.setstate(std::ios::failbit);
 }
 return is;
}

At least to me, this supports much cleaner code to read the data:

cin >> hour >> ":" >> minute >> ":" >> second >> ampm;

...and the operator overload takes care of reading data, comparing to what was passed, and setting the stream's failbit if it didn't match. As it stands right now, I've also included skipping any leading white-space like normal extraction operators do, but it would be trivial to eliminate that if you didn't want it (but it shouldn't make any difference for solving the HackerRank problem).

---

^{1. HackerRank is apparently using an old/broken standard library, so it won't accept this, even when you try to tell it you want to use C++14.}

• Edward: I understand you're probably trying to keep it simple, but for the get_time version, you'd probably want to initialize t. I'd omit struct and just write std::tm t{};. Also, in Real Life, we'd check std::cout.fail() to see if the conversion was successful, right? Finally, just to be pedantic, you'd first want to imbue the stream with a locale that supports AM/PM time -- not all do and without specifying, it's up to the current operating system settings.
• Jerry Coffin: @Edward: You'd probably want to check cin.fail. If cout fails, chances are good you can't do much (though it may be possible for cerr to continue working even when/if cout has failed). As far as locales go, it always starts out in the "C" locale, so at least in this case (doing the reading immediately upon entry in main) there's no need for an explicit imbue. Other than that though, yes.
• Edward: FYI, playing with this code inspired me to ask this question on SO.

Answer 2

Declarations

Always do declarations at the top of a function, even if you do not initialize that variable right there and then. This helps you figure out what a type of a variable is more easily when reading back the code.

Namespace

Do not use the std namespace. See this question on Stackoverflow for more information on why this is bad practice.

Whitespace

Be consistent with your whitespace. If you use whitespace around an operator, use whitespace around all operators.

Code improvements

You are juggling strings and stringstreams. Remember that std::cin is a stream too, so you can simply read directly from std::cin, skipping time and newTime completely.

std::cin >> hours >> delim >> minutes >> delim >> seconds >> ampm;

The giant switch statement is not necessary. There are two special cases: Midnight and noon. For all other PM times we simply add 12.

Error checking

You are not doing any error checking at all. You are not checking if the given input is within bounds, nor are you checking if the given input even made sense.

The first error is easy to check: Just do some tests to see if the numbers you read into hours, minutes and seconds are within the boundaries you expect them.

The second error puts the stream in an error state. We can catch this by putting the entire thing in an if-statement.

if( !(std::cin >> hours >> delim >> minutes >> delim >> seconds >> ampm) ) {
 //Written something in input stream that does not follow this pattern
 return 1;
} else if( hours <= 0 || hours > 12 || minutes < 0 || minutes >= 60 || seconds < 0 || seconds >= 60 ) {
 //Input not within bounds
 return 1;
}

You are treating any string that is not "PM" as if it is "AM". Obviously any string can be entered behind your time. You should check if it is "PM" or "AM", possibly in the same if-statement as above.

Includes everywhere

You are including a lot of libraries. Only iostream and iomanip are required. In your code you probably need sstream too. You are not using vectors, or math, or c-style input/output such as fprintf, so don't include them.