Is my code well written? What should I do next?

[RealGiganitris]

I wrote a small program which contain two functions that can used to do calculations for Pascal's triangle. I've been told in the past that my code can be pretty sloppy, so I was wondering where I can make improvements. I'm not sure when it's appropriate to comment the code.

On another note, what should I add to the program? What should it do?

int factorial(int a)
	{
	int b = 1;
	
	if(a >= 0)
		{
		for(int C = a;C != 0;C--)
			{
			b = b *C;
			}
		return b;
		}
	else if(a <= 0)
		{
		for(int C = a;C != 0;C++)
			{
			b = b *C;
			}
		return b;
		}
	return 0;
	}

int choose(int row, int column)
	{
	return factorial(row) / ( factorial(column) *factorial(row -column) );
	}

int main(int argc, char *argv[]){}

[Mantorr22]

Is my code well written? What should I do next?

Of course it is not well written at first sight. Good programmers usually write like this :

int factorial(int a)
{
	int b = 1;
	
	if(a >= 0)
	{
		for(int C = a; C != 0; C--) {
			b = b *C;
		}
		return b;
	}
	else 
	{
		for(int C = a; C != 0; C++) {
			b = b *C;
		}
		return b;
	}

	return 0;
}

int choose(int row, int column) {
	return factorial(row) / (factorial(column) *factorial(row - column));
}

int main(int argc, char *argv[]) 
{
}

[TheIdeasMan]

Hi,

const correctness is a good thing, try making the function parameters const.

You could avoid having 2 almost identical loops by making a positive to start with, then make the result negative if necessary.

Instead of having a long expression in the return statement, store it in another variable first. The reason for this, is when you look at the code in a debugger, you will be able to see the value of the expression.

There is a std::tgamma function which will give the factorial.

http://en.cppreference.com/w/cpp/numeric/math/tgamma

[Thomas1965]

@TheIdeasMan,

const correctness is a good thing, try making the function parameters const.

what would be the benefit? You receive only a copy of the var so even if you modify it it shouldn't be a problem.

[TheIdeasMan]

@Thomas1965

The function choose looks as though it's parameters should not be modified inside the function, so enforce that with const

You receive only a copy of the var so even if you modify it it shouldn't be a problem.

Sometimes that very thing is a problem, so one avoids it by using const. Maybe it makes no difference here, but it is good practise to use const where ever possible. Think of the future maintenance programmer who wants to change the code in some way ......

Regards :+)

[JLBorges]

In the declaration of a function, avoid using const to qualify parameters passed by value.

Semantically:
The use of const in a parameter passed by value only means that the function would not be able to modify its own copy of the object. What a function does with the copy of the object it receives is an implementation detail, internal to the function, and should not appear in its interface.

Syntactically:
The const qualification of a parameter passed by value is ignored for determining the type of the function.


In the definition of the function (separate from its declaration, typically not visible to the user), const qualifiers may be added to parameters passed by value; it would still be implementing the function that was declared earlier.

[RealGiganitris]

I like my use of brackets better for myself. To me, it's a lot easier to read and understand. I'm not really sure when it's a good idea to use const and what the benefits are to it. I also don't know how to use the gamma function to return a factorial, I never learned it in my math class.

Is there any use for a program that does calculations for Pascal's triangle?

[Thomas1965]

I'm not really sure when it's a good idea to use const and what the benefits are to it.

I agree with TheIdeasMan now even in your app it wouldn't make a difference but won't harm either.

[Michael5]

In general, a key attribute of well written code isn't just that it's formatted correctly, but that it's commented adequately.

Admittedly, your program is short and pretty simple, so one could reasonably maintain that commenting isn't truly needed in the same way it would be were the program longer and its component parts less obvious.

Still, since you do ask for tips on improving code, one tip I would offer is that commenting is not (generally) optional. It's not a "frill," having little real importance. And it isn't something to be included at the last step, just to keep a professor happy. No! It should be an integral part of the process of writing code.

Heck, short and simple as your program is, a little commenting might improve it. At the very least, how about a line or two up at the top, indicating the program's name, what it's intended to do, when it was written, and the name of the person who wrote it?

Commenting code is a very good habit to acquire. Start acquiring it now.

[dhayden]

I'm not sure when it's appropriate to comment the code.

Put a brief comment before each function saying what it does, what it's parameters are, and what the return value is. This doesn't have to be long. For example, for choose, you might say:

// Returns the number of ways to choose "column" items from "row" possibilities

Regarding the indentation style, if you ask 10 programmers how they indent their code, you'll get 15 answers. But consider this: the purpose of the braces is to guide the compiler, not the human. Humans read the indentation from the block structure and add whitespace where they think it's appropriate. I base my indentation style on this observation, modified slightly to take advantage of my editor's abilities to auto-indent.

I was wondering where I can make improvements.

The biggest improvement isn't in the style, but the functionality. At line 24, the intermediate results of factorial(row) / (factorial(column) * factorial(row-column)) will quickly overflow a computer's representation of integers. A calculation as simple as choose(15,3) will fail, even though the final result is just 455.

For unsigned values, it's best to compute choose(n,k) by alternating multiplication and division:
n * (n-1) / 2 * (n-2) / 3 * (n-3) / 4 ... * (n-k+1) / k
In addition to avoiding overflow, if you do it in this order, the divisions will always give an integer result, as long as the multiplications don't overflow. To see this, notice that in n * (n-1), you multiply 2 consecutive numbers, which means that one of them must be even. So dividing by 2 in the next step results in an integer. Now when you multiple by n-2, you have 3 consecutive numbers in the numerator, so one must be divisible by 3. So the division by 3 in the next step also results in an integer.

[TheIdeasMan]

With commenting, apart from dhayden's excellent suggestion, code can be self documenting by choosing good and meaningful names for functions and variables. The code itself should tell a story of what is happening. Here is a comical example of what I mean:

http://www.cplusplus.com/forum/lounge/176684/#msg872881

dhayden has mentioned several times in the past this idea: Put comments to say why the code is doing something.

Other things that can be comments: pre and post conditions, invariants, expected ranges of values.

One thing that often happens, is that names are often abbreviated too much. The length of a variable name is supposed to match the scope. For example the counter of a loop can have a single char name - although I am not a fan of i or j or any thing else that looks too similar. Call things for what they are: as you have done, row and column not i and j But a function should be fairly descriptive, not abbreviated too much, nor overly long.

So with the function choose: Choose what? So we are trying to turn dhayden's

// Returns the number of ways to choose "column" items from "row" possibilities

into a function name. The "number of ways" is a permutation, mathematically that would be notated permutation(row,column) so how about PermutationRowCol as a function name?

[JLBorges]

Say, we write a function unsigned long long factorial( unsigned int number ) ;

Do not, repeat do not, "put a brief comment before the function saying what it does, what it's parameters are, and what the return value is."

Use comments only for what can't be expressed in code. In the declaration of the above function, what needs to be commented are: What are the range of acceptable values for number? In particular, what happens if the factorial of the number is outside the range of unsigned long long?

The typical naming conventions (and comments) for functions would vary depending on whether the function is part of an interface or whether it is an implementation detail, accessible only to the implementer. In particular, the intended audience for comments in a header file are very different from that for comments an the implementation file.

The situation is the same as the one indicated earlier regarding the const-qualification of parameters passed by value: http://www.cplusplus.com/forum/beginner/215704/#msg1001747

[dhayden]

Do not, repeat do not, "put a brief comment before the function saying what it does, what it's parameters are, and what the return value is."

In the declaration of the above function, what needs to be commented are: What are the range of acceptable values for number? In particular, what happens if the factorial of the number is outside the range of unsigned long long?

Your points are well taken, but what's the point in documenting the edge cases if you don't explain what the function does in the first place?

[chipp]

int factorial(int a)
	{
	int b = 1;
	
	if(a >= 0)
		{
		for(int C = a;C != 0;C--)
			{
			b = b *C;
			}
		return b;
		}
	else if(a <= 0)
		{
		for(int C = a;C != 0;C++)
			{
			b = b *C;
			}
		return b;
		}
	return 0;
	}

int choose(int row, int column)
	{
	return factorial(row) / ( factorial(column) *factorial(row -column) );
	}

int main(int argc, char *argv[]){}

by the time i saw it, it feels like you're not use your code for programming but for drawing :DDDD

[lastchance]

#include <iostream>
using namespace std;

typedef unsigned long long INT;        // some of these numbers get ... big

//====================================================================================//
// Calculate nCr, the number of ways of choosing r things from n, disregarding order. //
// See:                                                                               //
//    https://www.mathsisfun.com/combinatorics/combinations-permutations.html         //
// for the difference between permutations (nPr) and combinations (nCr)               //
//                                                                                    //
// Requires:   n >= 0;   0 <= r <= n                                                  //
//====================================================================================//
INT combination( int n, int r )
{                                 
   if ( n < 0 || r < 0 || r > n )
   {
      cout << "Invalid input to combination( n, r )\n";
      return 0;
   }

   INT result = 1;
   if ( r > n - r ) r = n - r ;                                           // lesser number of factors
   for ( int k = 1; k <= r; k++ ) result = result * ( n - k + 1 ) / k;    // see explanation by @dhayden
   return result;
}



// Test routine for the above
int main()
{
   int n, r;
   cout << "Calculates nCr. Enter a negative value of n to end.\n\n";
   while ( true )
   {
      cout << "Enter n r: ";
      cin >> n >> r;
      if ( n < 0 ) return 0;
      cout << n << "C" << r << " = " << combination( n, r ) << endl;
   }
}

Calculates nCr. Enter a negative value of n to end.

Enter n r: 15 3
15C3 = 455
Enter n r: 12 7
12C7 = 792
Enter n r: -1 0

Recursive variant (a la Pascal's triangle):

INT combination( int n, int r )
{                                 
   if ( n < 0 || r < 0 || r > n )
   {
      cout << "Invalid input to combination( n, r )\n";
      return 0;
   }

   if ( n <= 1 || r == 0 || r == n ) return 1;
   else                              return combination( n - 1, r - 1 ) + combination( n - 1, r );
}

Using doubles to allow larger n and r:

INT combination( int n, int r )
{                                 
   if ( n < 0 || r < 0 || r > n )
   {
      cout << "Invalid input to combination( n, r )\n";
      return 0;
   }

   double value = 1.0;
   for ( int i = 0; i < r; i++ ) value = value * ( n - i ) / ( r - i );
   INT result = value + 0.5;           // rounds toward 0 to give integer value
   return result;
}

[JLBorges]

> but what's the point in documenting the edge cases

What you call 'the edge cases' are the invariants for the function: they are part of the interface, required to understand how the function is to be used. In particular:

a. Are there any constraints on the input to the function.

b. If so, what does the function do if these constraints are violated?


> if you don't explain what the function does in the first place?

unsigned long long factorial( unsigned int number ) ; does that quite clearly in the first place.

In this case, "put a brief comment before the function saying what it does, what it's parameters are, and what the return value is", just adds needless verbiage.

// This function computes the factorial of a number (surprise!) - unsigned int passed by value 
// (read carefully! without the above comment how could anyone have figured that out?)
// and returns the result as a prvalue of type unsigned long long (more surprise!, bet you couldn't have known that)
unsigned long long factorial( unsigned int number ) ;

[TheIdeasMan]

RealGiganitris wrote:

I also don't know how to use the gamma function to return a factorial, I never learned it in my math class.

https://en.wikipedia.org/wiki/Gamma_function

The example from cppreference I linked earlier:

#include <iostream>
#include <cmath>
#include <cerrno>
#include <cstring>
#include <cfenv>
#pragma STDC FENV_ACCESS ON
int main()
{
    std::cout << "tgamma(10) = " << std::tgamma(10)
              << ", 9! = " << 2*3*4*5*6*7*8*9 << '\n'
              << "tgamma(0.5) = " << std::tgamma(0.5)
              << ", sqrt(pi) = " << std::sqrt(std::acos(-1)) << '\n';
    // special values
    std::cout << "tgamma(1) = " << std::tgamma(1) << '\n'
              << "tgamma(+Inf) = " << std::tgamma(INFINITY) << '\n';
    // error handling 
    errno=0; std::feclearexcept(FE_ALL_EXCEPT);
    std::cout << "tgamma(-1) = " << std::tgamma(-1) << '\n';
    if(errno == EDOM)
        std::cout << "    errno == EDOM: " << std::strerror(errno) << '\n';
    if(std::fetestexcept(FE_INVALID))
        std::cout << "    FE_INVALID raised\n";
}

RealGiganitris wrote:

I'm not really sure when it's a good idea to use const and what the benefits are to it.

https://isocpp.org/wiki/faq/const-correctness

[JLBorges]

There is no guarantee that the std::tgamma would yield an accurate integer result,
though

Many implementations calculate the exact integer-domain factorial if the argument is a sufficiently small integer. http://en.cppreference.com/w/cpp/numeric/math/tgamma

Strongly favour the integer calculation of factorials for this program.

[lastchance]

If the OP's ultimate objective is his function choose( row, column ) - and I suspect it is - then he shouldn't be using factorials at all.

Writing the combination
ⁿC_r = n! / r!(n-r)!
is a nice tidy mathematical formula ... but it doesn't mean you should actually calculate it like this; (a) there is a massive amount of factor cancellation; (b) the individual factorials rapidly overflow.

On a separate point, it's irrelevant to function choose(), but in the OP's original code the factorial function isn't defined for negative integers (the corresponding gamma function goes to infinity).

[dhayden]

in the OP's original code the factorial function isn't defined for negative integers

It's defined at lines 13-20.