5
\$\begingroup\$

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);
}
\$\endgroup\$
  • \$\begingroup\$ This code is incredibly clean and easy to read, however, you might want to eliminate the output for days_needed when days_needed is 0. Think about that. It will make the program output cleaner if the output is not specified like in a contest or homework. \$\endgroup\$ – Shane Hsu Aug 30 '13 at 12:08
11
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ or unsigned const seconds_in_day = 24*60*60;. \$\endgroup\$ – Martin York Aug 30 '13 at 20:02
5
\$\begingroup\$

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:

  • (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 :-)

\$\endgroup\$
3
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ thanks :D after I posted the question I noticed the "missing minutes", but I thought it's not important :) \$\endgroup\$ – mitya221 Aug 30 '13 at 11:42
  • \$\begingroup\$ Why would floats help here? \$\endgroup\$ – Mat Aug 30 '13 at 11:45
  • \$\begingroup\$ @Mat, imagine a user input of float number going to an integer variable. It may change the figures and hence the calculation output. \$\endgroup\$ – Vivek Jain Aug 30 '13 at 11:56
0
\$\begingroup\$

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;
\$\endgroup\$

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.