Time calculator for a given speed and file size

Question

I wrote this little code in c++. It calculates the needed time for a given speed of a medium (for example the speed is 1024 B/s, the file size is 1MB, it'll take 17 minutes and 4 seconds to finish). The code works, but I'm not sure if it's proper.

Can you tell me if it's okay or not?

#include <iostream>

int main()
{
    int transmission_speed; //speed of the transmission in bytes per seconds
    int file_size_mb; //file's size in MBs

    std::cout << "Enter the speed of the transmission in bytes per seconds: ";
    std::cin >> transmission_speed;
    std::cout << "Enter the file's size in megabytes: ";
    std::cin >> file_size_mb;

    int file_size_b = file_size_mb *1024*1024;

    int seconds_needed = file_size_b / transmission_speed;

    int days_needed = (seconds_needed / 3600) / 24;
    seconds_needed -= days_needed*86400;

    int hours_needed = seconds_needed / 3600;
    seconds_needed -= hours_needed*3600;

    std::cout << "Days needed: " << days_needed << std::endl;
    std::cout << "Hours needed: " << hours_needed << std::endl;
    std::cout << "Seconds needed: " << seconds_needed << std::endl;
    return(0);
}

Answer 1

It's a very simple little program, however, there's a few comments to make:

• All of the code is in main. Of course, for a program of this size, that doesn't really matter too much, however, you should prefer to break things up into self-contained functions where possible. For example, there should be a separate function here to actually do the calculations.
• Use the correct data type for what you need. Can time ever be negative here? The answer is no, so I'd prefer to use unsigned over int.
• You should be somewhat careful about overflow. Any filesize over 2048MB (2GB) will overflow the size of an int (assuming a 4 byte int - in actuality, an int is only technically required to be at least 2 bytes). Using an unsigned will change this to 4GB, however, if you expect any filesizes larger than that, you should look at another datatype (std::uint64_t perhaps).
• Magic numbers. It's not so bad when dealing with time, because it's generally fairly obvious here, but numbers like 86400 shouldn't be shown as-is. They should be a named constant, such as const unsigned seconds_in_day = 86400.

I'd suggest breaking this up into a main function and a required_time(unsigned transmission_rate, unsigned file_size) function.

Answer 2

I see very few things to change, your code is easy to read.

• You're not validating the input. What if someone enters something that's not a number?

  I'd suggest you create a function that takes the prompt string and returns the entered integer. That function can do the validation, which isn't completely trivial.

  Two good examples of how to do the validation:

  • What is the best way to do input validation in C++ with cin?
  • Good input validation loop using cin

• (Very minor) You put a comment on the first two variable declarations, but not on the following ones. In my opinion, you chose sufficiently descriptive variable names, the comments aren't necessary. But if you do put a comment on some (or if your local coding style guidelines enforce that), do it consistently.

• This "looks" strange:

  int days_needed = (seconds_needed / 3600) / 24;
  seconds_needed -= days_needed*86400;
  

  You should be dividing with the same constant(s) that you use in the multiplication (or vice-versa). (These constants are easy to identify, I don't think giving them names is worth the effort. Just be systematic in how you use them.)

• return(0); is unusual, it looks like a function call. return is not a function call, it's a statement. I would prefer return 0; instead.

• Your code appears to have forgotten about minutes :-)

Answer 3

The code looks good though, did you miss on the minutes needed?

All the calculations you're doing are on int. You may consider using float at some places.

e.g.:

file-size-in-MB may be 3.4MB

transmission-speed-in-bytes may be 5.9B/s

Answer 4

In addition to all the helpful suggestions made by others...

Using a console interactively to get input is so old style!! It's more convenient to have the input data in a file and just read them directly without the lines for the prompt. Just use:

std::cin >> transmission_speed;
std::cin >> file_size_mb;

Then, you can use:

cat input.txt | ./program

This is very small oversight but the number of seconds computed in the line

int seconds_needed = file_size_b / transmission_speed;

will be off by 1 second if file_size_b % transmission_speed != 0. You should add

if ( file_size_b % transmission_speed != 0 )
   ++seconds_needed;