Formatting seconds as hours:minutes:seconds

Question

This is basically my school homework and I want to know what to improve, and what not to do. The homework basically is this: You will get a input in seconds, which is supposed to be shown as H:MM:SS where H is hours M is minutes and S is seconds. The formatting is supposed to look like in my example. ( note: In output, 0s from the left will be ignored and replaced with spaces )

Input:
3000
730
308
3967
15
1

Output:
The count of entires: 6
  50:00
  12:10
   5:08
1:06:07
     15
      1

So basically, I did a code based on the example. I haven't got more informations, just that but it is enough and fun. Here goes my code:

#include <iostream>
#include <fstream>

inline int CountSpaces ( int hod, int min, int sec ){
    int spaces = -1;
    // 1:06:07
    if ( hod )
        return 0;
    if ( min >= 10 )
        return 2;
    if ( min < 10 && min > 0 )
        return 3;
    if ( sec >= 10 )
        return 5;
    if ( sec < 10 )
        return 6;
    // in the case something goes wrong..
    return spaces;
}

// convert time to hour,min,seconds
void CalcTime ( int value, int& hour, int& min, int& sec ){
    hour = value / 3600;
    min = value % 3600 / 60;
    sec = value % 60;
}

// output spaces 
void Space ( int* hour, int* min, int* sec ){
    int spaces = CountSpaces ( *hour , *min , *sec );
    if ( !spaces ) return;
    for ( int i = 0 ; i < spaces ; i++ ){
        std::cout << ' ';
    }
}

void Write ( int time ){
    if ( time <= 0 )
        return;

    int hour;
    int min;
    int sec;

    CalcTime ( time , hour , min , sec );
    Space ( &hour , &min , &sec );

    bool bHour = false;
    bool bMin = false;
    if ( hour > 0 ){
        bHour = true;
        std::cout << hour << ':';
    }
    if ( min < 60 && min > 0 ){
        bMin = true;
        if ( bHour ){
            if ( min < 10 ){
                std::cout << '0';

            }
        }
        if ( min > 0 )
            std::cout << min << ':';

    }
    if ( sec < 60 && sec >= 0 ){
        if ( sec < 10 && bMin ){
            std::cout << '0';
        }
        std::cout << sec << std::endl;
    }
}


int main ( ){
    std::ifstream fin ( "in.txt" );

    if ( !fin ) return 1;

    int num;
    int count = 0;
    int arr[100];
    while ( fin >> num ){
        arr [ count++ ] = num;
    }
    std::cout << "The count of entires: " << count << std::endl;
    for ( int i = 0 ; i < count ; i++ ){

        Write ( arr [ i ] );
    }

    return 0;
}

Answer 1

• Bug

  Handling a case of 0 minutes is a bit more involved. For example, giving 3601 as an input produces 1:1 instead of expected 1:00:01.

• Trust your math

  min and sec are guaranteed to be less than 60. There is no point to test for it.

• Computing spaces

  I recommend not to compute them explicitly. You know that the output should be right-aligned into the 7-character-wide field. An std::setw(7) manipulator is specifically designed to do the job. Write your output to the string (or a stringstream), and

        std::cout << std::setw(7) << s << '\n';
  

  will achieve the goal.

Answer 2

Stop using built in arrays

int arr[100];
while ( fin >> num ){
    arr [ count++ ] = num;
}

If there are more than 100 numbers you are skrewed. USe the standard containers.

std::vector<int> arr;
while ( fin >> num ){
    arr.push_back(num);
}

// If you want to show off you can use:
// This does the same thing as the above loop.
std::vector<int> arr(std::istream_iterator<int>(fin), std::istream_iterator<int>());

Don't pass by pointer

You do a good job here (and use references):

void CalcTime ( int value, int& hour, int& min, int& sec ){

So why do you use pointers here?

void Space ( int* hour, int* min, int* sec ){

Pointers have no concept of ownership semantics (which means who owns the pointer. The owner of the pointer is the person responsible for calling delete). So you have no idea if you should be deleting the passed pointers or leaving them alone without digging into the code to find out. This is a very error prone proccess in anything larger than 100 line program. Luckily for you the use case is obvious but its something you should not be doing.

Error Message?

if ( time <= 0 )
    return;

You have already said the number of results you expect to print. But if one of your inputs is negative then you print nothing. This could be seen as an error.

Prefer '\n' to std::endl

You only need to use std::endl if you want to force a flush. This is rarely true. The streams will flush themselves when they need to at the most optimum time (so you forcing them to flush is usually sub optimal and can cause performance issues).

Write logic can be simplified a lot:

void Write ( int time )
{
    if ( time <= 0 )
    {
        std::cout << "Invalid Input (negative): " << time << "\n";
        return;
    }

    int hour;
    int min;
    int sec;

    CalcTime ( time , hour , min , sec );

    if ( hour > 0 )
    {
        std::cout << std::setw(4) << std:: setfill(' ') << hour << ':';
    }
    else
    {
        std::cout << std::string(5, ' ');
    }
    if (hour > 0 || min > 0)
    {
        char fillchar = (hour > 0) ? '0' : ' ';
        std::cout << std::setw(2) << std::setfill(fillchar) << min << ":";
    }
    else
    {
        std::cout << std::string(3, ' ');
    }
    char fillchar = (hour > 0 || min > 0) ? '0' : ' ';
    std::cout << std::setw(2) << std::setfill(fillchar) << sec << "\n";
}