Code Review
int requestNumInts();
bool validateInputSize(const int totalNumInts);
std::vector<int>* createVector(int totalNumInputs);
void loadVector(std::vector<int>* numbersEntered);
void printNumTrailingZeros(const std::vector<int>* numbersEntered);
int findTrailingZeros(int numberToCalculateZeros);
Personally I like to align the function names (this makes it easier to read). This is purely personal. Some like it some don't.
int requestNumInts();
bool validateInputSize(const int totalNumInts);
std::vector<int>* createVector(int totalNumInputs);
void loadVector(std::vector<int>* numbersEntered);
void printNumTrailingZeros(const std::vector<int>* numbersEntered);
int findTrailingZeros(int numberToCalculateZeros);
Now that I have lined it up two things sprint to mind.
- You are returning a vector by pointer (that's not good as there is no ownership semantics (who deletes it)). You should probably return by value. The optimizer will remove any copying and it prevents memory leaks. You can then pass the vector by reference to prevent copying in other situations.
- You seem to be writing C code. If you implements this inside an object then a lot of you parameters don't need to be passed they are part of the object that is being manipulated.
Nice:
std::ios::sync_with_stdio(false);
Pointer. Boo. Bad.
std::vector<int>* numbers;
It is rare to see RAW pointers in C++ code. Pointers are usually wrapped inside smart pointers. But in this case you don't even need a pointer just use a normal std::vector as an object in place.
std::vector<int> numbers = createVector(requestNumInts());
Using a delete is risky.
delete numbers;
It is hard to tell if numbers was dynamically allocated! You actually have to go and look that up in the function createVector(). So if you change createVector() you also need to go through your code and find every place that calls createVector() to make sure they also use it correctly. Also its not exception safe. If an exception propagates through your code then you leak memory.
The main() function is special. If you don't specify a return then the compiler generates a return 0; for you. If your code can do nothing else apart from exit successfully then leave the return 0; out to indicate that there are no failure states. If there are error exit states then return 0; is an indication that the reader of the code should look for exit failures attempts.
Here:
int requestNumInts() {
int totalNumInts = 0;
while(!validateInputSize(totalNumInts))
{
std::cin >> totalNumInts;
}
return totalNumInts;
}
The first attempt will always fail. So why not use a do {} while() loop. This is designed for this situation. You always execute the code before doing the test.
Avoid if conditions that return true/false.
bool validateInputSize(const int totalNumInts) {
const int MAX_NUMBER_OF_INPUTS = 1000000000;
const int MIN_NUMBER_OF_INPUTS = 1;
if(totalNumInts >= MIN_NUMBER_OF_INPUTS && totalNumInts <= MAX_NUMBER_OF_INPUTS) {
return true;
}
else {
return false;
}
}
The above can be written as:
Much more readable.
return (totalNumInts >= MIN_NUMBER_OF_INPUTS)
&& (totalNumInts <= MAX_NUMBER_OF_INPUTS);
Don't create the vector with new.
std::vector<int>* createVector(int totalNumInts) {
std::vector<int>* numbers = new std::vector<int>(totalNumInts);
return numbers;
}
RVO and NRVO will remove the copy that happens when you return by value. Also with C++11 and move semantics this makes this even more efficient. So never do this.
Also this whole function can be replaced with just a simple declaration in main.
Much simpler and more readable.
std::vector<int> numbers(requestNumInts());
Good try:
void loadVector(std::vector<int>* numbersEntered) {
for(unsigned int count = 0; count < numbersEntered->size(); ++count) {
std::cin >> (*numbersEntered)[count];
}
}
Couple of different ways to do this:
// Use the new foreach keyword
for(auto& val: numbers)
{
std::cin >> val;
}
// Using iterators.
for(auto loop = numbers.begin(); loop != numbers.end(); ++loop)
{
std::cint >> (*loop);
}
// Or we can use the old classic C++03 std::transform
std::transform(std::begin(numbers), std::end(numbers),
std::istream_iterator<int>(std::cin),
std::begin(numbers),
[](int& /*val1*/, int& val2){ return val2;});
Again nice effort with the printing.
void printNumTrailingZeros(const std::vector<int>* numbersEntered) {
for(unsigned int count = 0; count < numbersEntered->size(); ++count) {
std::cout << findTrailingZeros((*numbersEntered)[count]) << std::endl;
}
}
Again some other options:
// Use the new foreach keyword
for(auto& val: numbers)
{
std::cout << findTrailingZeros(val) << "\n";
}
// Using iterators
for(auto loop = numbers.begin(); loop != numbers.end(); ++loop)
{
std::cout << findTrailingZeros(*loop) << "\n";
}
// Or we can use the old classic C++03 std::for_each
std::for_each(std::begin(numbers), std::end(numbers),
[](int val){ std::cout << findTrailingZeros(val) << "\n";});
Also note the use of "\n" rather than std::endl. The std::endl adds a "\n" to the stream but then also calls flush. This is hardly ever what you actually want to do. Let the stream flush itself it makes it much more efficient to do so.
In your loops get used to use iterators to loop over containers. They are much more versatile and apply to all containers. Also you can pass them to functions very easily and they allow you to specify sub-ranges very trivially (event the foreach uses iterators underneath).
How I would do it.
First note that the output does not depend on previous values. You could cache them for speedy look-up but that seems overkill for such a simple algorithm. So there is no need to store the data in a vector.
#include <iostream>
int main()
{
std::ios::sync_with_stdio(false);
int count;
std::cin >> count;
for(int loop=0; loop < count; ++count)
{
int value;
std::cin >> value;
std::cout << LeadingZero(value) << "\n";
}
}
