Making a row of bricks

We want to make a row of bricks that is goal inches long. We have a number of small bricks (1 inch each) and big bricks (5 inches each). Return true if it is possible to make the goal by choosing from the given bricks.

bool make_bricks(int small,int big,int goal)
{
    if(big > 0)
    {
        double test = (double)goal/((double)big*5.0);//The above if statement eliminates the possibility of division by zero
        if(test > 1.0)//This means we cannot make the row with big bricks alone
        {
            if(small >= (goal-big*5))//If we have enough small bricks after using all our big bricks
            {
                return true;
            }
            else//If we don't have enough small bricks to finish the row
            {
                return false;
            }
        }
        else if(test == 1)//If we have exactly enough big bricks to make the row
        {
            return true;
        }
        else//We have enough big bricks to make the row, but perhaps not enough small bricks
        {
            if(small >= (goal%5))//If we have enough small bricks after using as many big bricks as possible
            {
                return true;
            }
            else//We do not have enough small bricks after using as many big bricks as possible
            {
                return false;
            }
        }
  }
  else//If the big is not greater than 0, we depend solely on small bricks to make the row.
  {
      if(small >= goal)//If we have enough small bricks to make the row or more than enough
      {
          return true;
      }
      else//We do not have enough small bricks to make the row
      {
          return false;
      }
  }
}

Your function is very much more complicated than it needs to be.

As I understand it, the function should check if it is possible to build a row of exactly the goal size, that goal is not a "minimum" requirement, but the exact requirement.

Futhermore, I assume it would be OK to make a row that is of size 14, for example, completely out of 14 small bricks, and 0 large bricks, as long as the row has the size of goal.

We test for the small bricks first:

// check minimum number of small bricks required.
if (goal % 5 > small) return false; // not enough small bricks.

If the target is something like 14, then no matter what, we will absolutely need at least 4 small bricks, or the goal is impossible.

Then we test to see if there is enough length, in total, to make the goal:

if (small+big*5 < goal) return false; // not enough
else return true; // passed both tests

And if we pass both of these checks, then we know it is possible to reach the goal with the bricks that were supplied.

Here is how you could write the entire funcion:

bool make_bricks(int small,int big,int goal)
{
    if (goal % 5 > small) return false;
    if (small+big*5 < goal) return false;
    else return true;
}

As suggested by user "200_success", you could further simplify this to:

bool make_bricks(int small,int big,int goal)
{
    return (goal % 5 = goal);
}

This version does cut down on the number of code lines, if that matters to you. Performance wise, there is no reason to think it will actually make a difference which one you use. The first, longer version, is perhaps clearer and easier to comment.

Note that, of course, none of these versions take into account what would happen if the function was called with nonsensical arguments, such as a negative number of small bricks.

Perhaps as a side-note, note that modulus % calculation is costly, though, so don't use it when you can avoid it. If you're writing x % 10, for example, then you might save performance by first checking if x < 10, and possibly avoid a few modulus calculations. That is, if you expect ´x´ to often be less than 10. If you expect the opposite, then don't make such a check.

When it comes to the design/interface choices, I would criticize the choice of the name of the function. The name seems to imply that the function "makes bricks". Good practice suggests that you name your functions in a way that makes it possible to deduce what the function does. And as Scott Meyers explained in a talk, it's about what people who see the function/interface expect about it.

Even if you are the only one who will ever use this function, I'm talking about good practice, and I hope and assume you'll appreciate the input/feedback, since you posted it here.

Common practice for functions whose primary role is to return a bool is to have the function name be a statement about the thing in question, such as is_ready() or was_released(), or something of that style, since this fits with the idea of true or false.

I suggest you consider something like bool can_make_row(int numSmall, int numBig, int rowSize).

StackExchange Code Review Q#125590, answer score: 6