Better way to repeatedly use istringstream in "counting minutes" challenge

Question

I was doing this easy challenge "Counting Minutes I" from Coderbyte.com where you just calculate the number of minutes between two given times.

The second time is always after the first, although it may look to be before but mean it's during the next day.

The format of their test cases is "hour1:minute1-hour2:minute2" such as "12:30am-12:00pm" where the difference would be 11.5 hours = 1425 minutes.

In converting different parts of the string containing the times to ints I've been using istringstream but creating a new one each time I call the function to set the ints. It seems like this is a sloppy way of accomplishing this, is there a better way like creating one istringstream in my main CountingMinutesI function and passing that as an argument to getNum then clearing it out after setting each particular int? Or is this also a bad idea?

#include <iostream>
#include <sstream>
using namespace std;
void getNum (string timeString, int& pos, int& result) {
 string temp;
 while (isdigit(timeString[pos])) {
 temp += timeString[pos];
 pos++;
 }
 istringstream iss (temp);
 iss >> result;
}
void to24HourTime(string timeString, int& pos, int& hourToConvert) {
 if (timeString[pos] == 'p' && hourToConvert != 12)
 hourToConvert += 12;
 if (timeString[pos] == 'a' && hourToConvert == 12)
 hourToConvert = 0;
}
int CountingMinutesI(string str) { 
 int hour1int, hour2int, min1int, min2int, minDifference;
 int stringPos = 0;
 getNum(str, stringPos, hour1int);
 stringPos++;
 getNum(str, stringPos, min1int);
 to24HourTime(str, stringPos, hour1int);
 stringPos += 3;
 getNum(str, stringPos, hour2int);
 stringPos++;
 getNum(str, stringPos, min2int);
 to24HourTime(str, stringPos, hour2int);
 if (hour2int < hour1int) //second time is the during the next day
 hour2int += 24;
 if (hour1int == hour2int && min1int > min2int) //time difference would look negative, but is really between 23 and 24 hours
 hour2int += 24;
 if (min2int < min1int) { //simplify subtracting minutes to add to total difference
 min2int += 60;
 hour2int--;
 }
 minDifference = (hour2int - hour1int) * 60 + (min2int - min1int);
 return minDifference;
}
int main() { 
 // keep this function call here
 cout << CountingMinutesI(gets(stdin));
 return 0;
}

• ring bearer: how come 11.5 hours = 1425 minutes. ?

Answer 1

I think your whole approach is very C like.

Personally I would define classes that represent the fundamental things you are representing and define input operators for these things so that they can be read directly from the input stream.

This is what I would do:

Note: I am assuming extra white space is acceptable. If it is not then modifying the code to use std::noskipws is trivial.

AMPM

class AMPM
{
 bool am;
 public:
 operator bool isAM() const {return am;}
 operator bool isPM() const {return !am;}
 friend std::istream& operator>>(std::istream& str, AMPM& data)
 {
 std::string ampmval(2, ' ');
 str >> std::ws; /* Skip leading whitespace */
 str.read(&ampmval[0], 2);
 if (ampmval == "am") { data.am = true;}
 else if (ampmval == "pm") { data.am = false;}
 else { str.setstate(std::ios::badbit);}
 return str;
 }
};

Time-Thing

class TimeInMin
{
 int minutesFromMidnight;
 public:
 void add(int minutes)
 {
 minutesFromMidnight += minutes;
 }
 friend std::istream& operator>>(std::istream& str, TimeInMin& data)
 {
 int hour;
 int min;
 char colon;
 AMPM ampm;
 if (str >> hour >> colon >> min >> ampm)
 {
 if (colon != ':')
 {
 str.setstate(std::ios::badbit);
 }
 else
 {
 if (hour == 12)
 { hour = 0;
 }
 if (ampm.isPM())
 { hour += 12;
 }
 data.minutesFromMidnight = hour * 60 + min;
 }
 }
 return str;
 }
 friend int operator-(TimeInMin const& lhs, TimeInMin const& rhs)
 {
 return lhs.minutesFromMidnight - rhs.minutesFromMidnight;
 }
 friend bool operator<(TimeInMin const& lhs, TimeInMin const& rhs)
 {
 return lhs.minutesFromMidnight < rhs.minutesFromMidnight;
 }
};

Now main() is trivial.

int main(int argc, char* argv[])
{
 TimeInMin first;
 TimeInMin second;
 char minus;
 if ((std::cin >> first >> minus >> second) && (minus == '-'))
 {
 /* It worked */
 if (second < first)
 { second.add(60*24);
 }
 std::cout << (second - first) << "\n";
 }
}

Other comments:

Yes you are converting to and from string/stream way to much.

 // reading from string
 // tracking position
 // and copying into another string
 // which will then be copied into a stream
 string temp;
 while (isdigit(timeString[pos])) {
 temp += timeString[pos];
 pos++;
 }

That's way to much work. The problem arises from converting your input from a stream into a string and then trying to parse a string manually and keep track of the position. The original stream would have kept track of your position for you.

If you want to do line based input (a good idea). Then you should read a line into a string. Then convert that to a stream and just use the stream from that point on.

 std::string line;
 std::getline(std::cin, line);
 std::stringstream linestream(line); // now just use this.
 // it will keep track of the position
 // for you.

The other thing you don't do is validate your input. What happens if the input is bad? You should at least attempt to detect bad input. Otherwise your code will go haywire when it sees something it does not expect.

When you use streams this is easy; just set the badbit on the stream. Any other input operation will then just silently do nothing; and any test you use the stream in (if statement condition or loop statement condition) will auto convert the stream to a false value (technically a value that will convert to false) and thus not do that code (see my main() above for an example.

• Instinct: Interesting, hadn't thought of using classes in this way for a time difference problem. Seems to be a lot more clear and would be easier to troubleshoot, good advice!

Answer 2

In to24HourTime(), pos doesn't need to be passed by reference as it's not being modified. Just pass it by value.

Instead of passing hourToConvert by reference (which is being modified), you can just pass by value and then return the result. It may also increase readability as it'll be clear that the third argument is to be reassigned by the function.

Final changes:

hour1int = to24HourTime(str, stringPos, hour1int);
// ...
int to24HourTime(string timeString, int pos, int hourToConvert) {
 if (timeString[pos] == 'p' && hourToConvert != 12)
 hourToConvert += 12;
 if (timeString[pos] == 'a' && hourToConvert == 12)
 hourToConvert = 0;
 return hourToConvert;
}

• Instinct: Didn't realize I wasn't even changing pos in that function, must have just been copying the input types from getNum. Will make the function changes though I wasn't sure whether to keep them void+pass by reference or just return the values I wanted change, but it does seem more readable this way.

Answer 3

using namespace std;

I don't do much C++, but one thing I've learned from the many reviews I've upvoted on this site, I'm 100% positive, it isn't a good idea to import std in the global namespace; instead, refer to std::cout, std::string and std::istringstream.

See this Stack Overflow answer for more details about why it's best to avoid potential name clashes by not importing that namespace.

Also return 0 at the end of the main() procedure is redundant, the compiler should take care of it.

• Instinct: Never knew you didn't need to return 0, is it considered bad practice to leave the return out though? Only kept the namespace import because the site automatically put it in there, usually I leave it out.
• Jamal: @Instinct: No, it shouldn't be considered bad practice. It is already known that main() will always return 0 at the end, and any knowledgeable C++ programmer should know this. Leaving it in is still okay; it's just redundant if the compiler can already do this.

Answer 4

First, you should be aware that although you probably don't care about them in this specific case, there are lots of bits of ugliness to deal with if you try to deal with this issue in general. Time-keeping has so many strange little corner cases it's an absolute nightmare to get it really correct in general. Just for example, if a time crosses a daylight savings time (aka "summer time") boundary, the amount of time could be one hour longer or shorter than this will compute. For the moment, I'm going to assume you don't want to deal with any issues like that though.

Second, the standard library has functions that attempt to make tasks like this quite a bit easier. If you don't want to implement everything on your own, a combination of mktime and difftime will reduce the amount of code you need to write by a fairly substantial factor.

Third, for this particular case, I'd at least consider using scanf instead of using iostreams at all. Given the formatting of the input data, you can consolidate nearly all the data reading down to something on this order:

scanf("%d%:%dc%*c-%d:%d%c%*c", &hour1, &minute1, &ampm1, &hour2, &minute2, &ampm2);

Although scanf can be somewhat tricky to use well (especially to read strings safely), in this case the use is pretty safe--certainly a lot safer than the gets you used to read the input (in fact, you'd be better off if you never used gets again--for anything, under any circumstances).

From there, I'd write a small function that takes an hour and AM/PM indicator, and converts that to hours since midnight:

int to24hour(int hour, char ampm) { 
 if (hour == 12)
 hour -= 12;
 return ampm == 'a' ? hour : hour+12;
}

This gives clearly-defined boundaries between different parts of the code. In main, you read the input from the stream, and get a time as hour (in 12-hour format), minute, and AM/PM indicator.

The next step takes 12-hour format input and produces 24-hour format as output. As your code is written, nearly all the code has to work with the input string as a string, and cooperate in keeping track of a current position in the string. This (for one example) makes it much more difficult for any of the code from being put to other uses--nearly all the code deals directly with the input string, so using it with input in a different format is likely to be difficult.

Summary:

1. Try to isolate code that deals with details like specific input formats into one place.
2. Don't use gets. Ever. For any reason.
3. Outside very strict boundaries, computations involving time get ugly in a hurry. Avoid them if humanly possible.
4. In a few cases (possibly including this one) it's worth considering alternatives to using iostreams directly. They're great for some purposes, but can get ugly for others.

• Morwenn: I would even add that gets has been deprecated for a long time and has been removed from C/C++ in both the C11 and C++14 standards.
• Loki Astari: I think you have your format string wrong. Currently it will read 12am:30 I think it should read 12:30am. "%d:%d%c%*c-%d:%d%c%*c" (don't forget to move the parameters appropriately.
• Instinct: Oh that's awesome have actually never used scanf before, but definitely seems appropriate here, I'm assuming %*c means "skip this character"? So it reads in the a or p but disregards the m? Also very clean converting function using only one if, thanks for the help!
• Jerry Coffin: @Instinct: Yes, %c means read a character, and the * means "read but ignore" (so "%*d" means read and ignore a decimal integer, and so on). Note that printf also uses *, but entirely differently (one of the few places the two are entirely different).