6
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ Hi Alex. In comments to multiple answers, you mentioned constraints on the implementation; namely that your instructor has disallowed the use of stringstream, string, and STL. This is good information for answerers to know. Would you mind editing those constraints into the question? \$\endgroup\$ – scottbb Jun 20 '16 at 1:44
7
\$\begingroup\$
  • 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.

\$\endgroup\$
  • \$\begingroup\$ Thank you, I fixed the bug. Also, I didn't know about setw too, but we aren't allowed to use stringstreams nor strings (so far). \$\endgroup\$ – Alex Herbert Jun 18 '16 at 6:21
2
\$\begingroup\$

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";
}
\$\endgroup\$
  • \$\begingroup\$ We are not allowed to use much of STL, teacher says I should not use vector because "we did not learn to use it" how should I argue it? I'm familiar with vector, I use vector on my projects outside school, but I don't really mind using C arrays too, because I never had enormous input and problems in such category. I only need to argue why should we not use old C arrays, obviously not in agressive way or anything. Thanks. \$\endgroup\$ – Alex Herbert Jun 19 '16 at 21:20
  • \$\begingroup\$ @AlexHerbert: Listen to your teacher. He probably wants to teach you the basics first and get to vector later. Thats fine. \$\endgroup\$ – Martin York Jun 20 '16 at 5:43

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.