Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have been reading Clean Code and decided to start working problems on Codechef.com attempting to apply some of what I have learned.

Do I seem to be on the right track or am I way off?

The challenge is to find the number of trailing zeroes in the decimal form of N!, where 1 ≤ N ≤ 109.

I am more concerned with the coding style than the way I solved the problem but any comments are appreciated.

#include <iostream>
#include <vector>

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);

int main() {    
    std::ios::sync_with_stdio(false);
    std::vector<int>* numbers;
    numbers = createVector(requestNumInts());       
    loadVector(numbers);
    printNumTrailingZeros(numbers);
    delete numbers;
    return 0;
}

int requestNumInts() {
    int totalNumInts = 0;
    while(!validateInputSize(totalNumInts))
    {
        std::cin >> totalNumInts;   
    }   
    return totalNumInts;
}

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;
    }
}

std::vector<int>* createVector(int totalNumInts) {              
    std::vector<int>* numbers = new std::vector<int>(totalNumInts);     
    return numbers;
}

void loadVector(std::vector<int>* numbersEntered) {
    for(unsigned int count = 0; count < numbersEntered->size(); ++count) {
        std::cin >> (*numbersEntered)[count];   
    }
}

void printNumTrailingZeros(const std::vector<int>* numbersEntered) {
    for(unsigned int count = 0; count < numbersEntered->size(); ++count) {
        std::cout << findTrailingZeros((*numbersEntered)[count]) << std::endl;
    }
}

int findTrailingZeros(int numberToCalculateZeros) {
    int totalTrailingZeros = 0; 
    const int FACTOR = 5;
    while(numberToCalculateZeros >= FACTOR) {
        numberToCalculateZeros /= FACTOR;
        totalTrailingZeros += numberToCalculateZeros;
    }
    return totalTrailingZeros;
}
share|improve this question

2 Answers 2

up vote 10 down vote accepted

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";
     }
 }
share|improve this answer
    
Thank you for taking the time to answer in such detail although it was pretty disheartening at first having made so many mistakes. I have applied a few of them so far and it has made the code much simpler. –  M K Aug 12 at 19:49
    
@MK: I would not call them mistakes (apart from using pointers). –  Loki Astari Aug 12 at 20:13
3  
@godlygeek: Well normally you go. int main() { int result = 0; /* Do Stuff that may alter ->result<- */ return result; } If you know its not going to fail. Then indicate this by no return value. –  Loki Astari Aug 12 at 20:43
1  
I'd find that weird if I ever saw it in production code. Still don't think I agree with it. But all the rest of the points are great, though - +1 –  godlygeek Aug 12 at 20:43
2  
I like the suggested convention for including/omitting return 0;. –  200_success Aug 12 at 21:08

My first thought is that it doesn't look like a (modern) C++ program. Rather, it looks like code that started as C and then was translated to C++.

C++11 isn't mentioned, thus I won't consider it (even though you should).


  1. You almost never need a pointer to a container: pass by (const) reference whenever possible.

  2. Uppercase names should be reserved to macros, and names should be neither too long nor too short:

    const int MAX_NUMBER_OF_INPUTS = 1000000000;
    const int MIN_NUMBER_OF_INPUTS = 1;
    

    could be simply named max_inputs and min_inputs.

  3. I personally don't like if-else statements for returns. Just use

    return totalNumInts >= min_inputs && totalNumInts <= max_inputs ? true : false
    

    or even

    return totalNumInts >= min_inputs && totalNumInts <= max_inputs
    
  4. As a successor of #1, vector, and all containers in general, need not be allocated dynamically at all. I can imagine at least these drawbacks:

    • Performance: Allocating on the heap is slower than on the stack.
    • Exception safety: new might throw std::bad_alloc and in that case your vector would be leaked.
    • General idioms: RAII is vastly used among C++ programmers and vector itself does follow that idiom.
  5. loadVector may be simplified using one of the generic <algorithm>s such as std::for_each.

Other notes:

  1. Instead of requesting the number of elements and then input them, you might ask the user to enter EOF (CTRL + D on Unix) this way:

    int temp; 
    while (cin >> temp)
        numbers.push_back(temp);
    
  2. Your task could be wrapped around a class.

share|improve this answer
    
Thank you for you response. I do have a question and it's based on your initial impression of it not looking like (modern) c++. Could you please elaborate on that or possibly point me to a resource so I could research it on my own? –  M K Aug 12 at 19:54
1  
You caught a few things I missed. Not sure I agree on 4 (Performance) or 4 (Exception Safety) but not a big deal close enough. +1 –  Loki Astari Aug 12 at 20:16
1  
@MK You're welcome. Basically I got that impression because it's closer to C-style than the C++ one. Check out this and this for further information of what I'm talking about. –  black Aug 15 at 16:03
    
Thank you @LokiAstari and all the editors. –  black Aug 15 at 16:04
    
@black Wow that presentation was eye-opening, I need to watch it a few more times to fully grasp everything mentioned. Thank you for that link. –  M K Aug 15 at 18:14

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.