Skip to main content
Commonmark migration
Source Link

Naming

#Naming GenerallyGenerally, variable names should be related to what they represent in the real world. In the case of a, b, and c, they represent the coefficients to the square, linear, and constant terms of the quadratic. You could name them as such, but since most algebra books just call them a, b, and c, I wouldn't worry too much about that. Most people reading this will know what they mean.

Variable Locality

#Variable Locality YouYou define a, b, c, level, randMax, and ch at the top of main, but none of them are used until much further down in the function. Because of the function nesting you've used, these are essentially global variables (which is not a good thing). The nonZeroCoefficient() function could, for example, change level to something else, and it would be difficult to figure out where it changed if you were debugging a problem related to the level variable having the wrong value.

Whitespace

#Whitespace II recommend leaving a little more whitespace in your code. It's a bit hard for me to mentally parse:

Headers

#Headers SpeakingSpeaking of headers, do you need every one of those headers you include? I removed the inclusion of both <math.h> and <ctype.h> and everything compiled the same. It may depend on compiler settings, though. I'm not positive.

#Naming Generally, variable names should be related to what they represent in the real world. In the case of a, b, and c, they represent the coefficients to the square, linear, and constant terms of the quadratic. You could name them as such, but since most algebra books just call them a, b, and c, I wouldn't worry too much about that. Most people reading this will know what they mean.

#Variable Locality You define a, b, c, level, randMax, and ch at the top of main, but none of them are used until much further down in the function. Because of the function nesting you've used, these are essentially global variables (which is not a good thing). The nonZeroCoefficient() function could, for example, change level to something else, and it would be difficult to figure out where it changed if you were debugging a problem related to the level variable having the wrong value.

#Whitespace I recommend leaving a little more whitespace in your code. It's a bit hard for me to mentally parse:

#Headers Speaking of headers, do you need every one of those headers you include? I removed the inclusion of both <math.h> and <ctype.h> and everything compiled the same. It may depend on compiler settings, though. I'm not positive.

Naming

Generally, variable names should be related to what they represent in the real world. In the case of a, b, and c, they represent the coefficients to the square, linear, and constant terms of the quadratic. You could name them as such, but since most algebra books just call them a, b, and c, I wouldn't worry too much about that. Most people reading this will know what they mean.

Variable Locality

You define a, b, c, level, randMax, and ch at the top of main, but none of them are used until much further down in the function. Because of the function nesting you've used, these are essentially global variables (which is not a good thing). The nonZeroCoefficient() function could, for example, change level to something else, and it would be difficult to figure out where it changed if you were debugging a problem related to the level variable having the wrong value.

Whitespace

I recommend leaving a little more whitespace in your code. It's a bit hard for me to mentally parse:

Headers

Speaking of headers, do you need every one of those headers you include? I removed the inclusion of both <math.h> and <ctype.h> and everything compiled the same. It may depend on compiler settings, though. I'm not positive.

Source Link
user1118321
  • 11.8k
  • 1
  • 19
  • 46

I'm able to follow this code pretty easily. It's readable and comprehensible, which is great. In addition to the things others have said, I see a few things that could be improved.

#Naming Generally, variable names should be related to what they represent in the real world. In the case of a, b, and c, they represent the coefficients to the square, linear, and constant terms of the quadratic. You could name them as such, but since most algebra books just call them a, b, and c, I wouldn't worry too much about that. Most people reading this will know what they mean.

I would however change f, g, h and k, as those are not so common. You could name them something like factor1Coefficient and factor1Constant for f and g, respectively, and similarly for h and k.

I would also change the inputF through inputK variable names to what they represent. Something like userFactor1Coefficient and userFactor1Constant, etc.

The name ch tells a reader of the code nothing. It's a char, but we can already see that from the declaration. What is its purpose? In this case, it's to hold the user's response to whether they want to continue, so it could be named userResponse, or keepGoing, or something like that.

#Variable Locality You define a, b, c, level, randMax, and ch at the top of main, but none of them are used until much further down in the function. Because of the function nesting you've used, these are essentially global variables (which is not a good thing). The nonZeroCoefficient() function could, for example, change level to something else, and it would be difficult to figure out where it changed if you were debugging a problem related to the level variable having the wrong value.

I recommend that you do what you did with f through k and declare the variables where you use them. So in this case, a, b, and c would be declared after f, g, h, and k.

You may object at this point and notice that level is used in nonZeroCoefficient(), but it can't be declared there because it needs to already be set by the time that function is called. And you are correct. Instead what you should do is pass the value into the function like this:

int nonZeroCoefficient(const int level) {
    int n = (rand() % (level + 2) - level);
    // ...etc.
    return n;
}

Some may object to marking an int as const because even if you did change it in the function, the value would not be returned to the caller. However, it allows someone reading the code to understand that level will not change in the function at all, which makes understanding the whole function easier. It's a stylistic choice that's up to your judgement. I prefer it.

#Whitespace I recommend leaving a little more whitespace in your code. It's a bit hard for me to mentally parse:

inputF*inputK+inputG*inputH== b

I would rewrite it as:

inputF * inputK + inputG * inputH == b

As a general rule, I always put a space before and after any operator. There's a section of the book Code Complete by Steve McConnell that describes the way he formats his code and the reasoning behind each part of that decision. I found that very helpful early in my career for writing code that was easier to understand. You might find it helpful, too.

I would not, however, leave a space between the # and the word include when including headers. I've never seen that done anywhere. It's apparently valid, but looks very odd.

#Headers Speaking of headers, do you need every one of those headers you include? I removed the inclusion of both <math.h> and <ctype.h> and everything compiled the same. It may depend on compiler settings, though. I'm not positive.