using namespace std;
Why is “using namespace std;” considered bad practice?
Personally, I find code more readable if it says what namespace is being used. E.g.
unsigned test_cases, low_range, high_range, count = 0;
Three of these variables are only used inside the loop, so you can simplify by just saying
As a general rule, you should declare variables at the smallest scope where they are needed.
while(test_cases != 0)
cin >> low_range >> high_range;
Some people prefer
for loops for this kind of situation.
for (; test_cases > 0; --test_cases)
uint32_t count = 0;
uint32_t low_range, high_range;
std::cin >> low_range >> high_range;
Also added declarations inside the loop for variables that are only used inside the loop. Note how this also takes care of resetting the
count on each iteration. This saves clearing the
count after it's no longer used, which your original code does.
unsigned is not guaranteed to be a 32-bit integer, although it almost always will be. Using
uint32_t guarantees 32 bits and is found in
for(unsigned i = low_range; i <= high_range; ++i)
if(sqrt(i) == floor(sqrt(i)))
count += 1;
First, why not say
That's simpler than
count += 1;.
I am unsure of how I can simplify my code for this HackerRank activity:
But you are experiencing a timeout. You don't need to make it simpler. You need to make it faster.
Rather than calculating the square root on every iteration, calculate them only once.
// +1 because both ends are inclusive
// floor to keep within the high end (inclusive)
// ceil to keep within the low end (inclusive)
int32_t count = (unsigned)std::floor(std::sqrt(high_range)) - (unsigned)std::ceil(std::sqrt(low_range)) + 1;
if (count < 0)
count = 0;
I was originally going to do a
for loop where I only calculated the squares, but I realized that I didn't have to do so. This will get the right answer consistently. It counts the number of integers from the square root of
A to the square root of
B. That will correspond to the number of squares between
B, since we're only counting the squares of integers.
for(unsigned i = 0; i < sherlock_squares.size() - 1; ++i)
cout << sherlock_squares.at(i) << endl;
cout << sherlock_squares.at(sherlock_squares.size() - 1);
Rather than subtracting 1 from the end, add 1 to the beginning. That saves math operations.
std::cout << sherlock_squares.at(0);
for (unsigned i = 1; i < sherlock_squares.size(); ++i)
std::cout << '\n' << sherlock_squares.at(i);
'\n' is faster, as
std::endl triggers a flush, which you don't need here. Probably not the performance bottleneck in this case, but it can be in some cases.
Always using the block form of control structures is more reliable and simplifies source control if you add statements in the future.
But I actually think that what the challenge wanted was
for (auto sherlock_square : sherlock_squares)
std::cout << sherlock_square << '\n';
with new lines after every line rather than only between them.
It may even have wanted you to interleave input and output. But that's uncertain.