Quiz on factoring quadratic expressions

Question

I am a beginner, and it is my first program in C. It works well, but I was wondering if there is a shorter way to do it, and how I could have done it better.

It is supposed to make a quadratic expression and ask the user to factor it. If the user factors it correctly (s)he proceeds to the next level, which is a slightly harder question. If (s)he cannot solve it correctly, (s)he stays on the same level, unless (s)he wants to exit.

# include <stdio.h>
# include <stdlib.h>
# include <ctype.h>
# include <string.h>
# include <math.h>
# include <time.h>

int main() {

  srand(time(0));
  int a, b, c, level=1, randMax=level*5;
  char ch;

  // generate the problem to ask
  int ask(){

    // generate non-zero random number for first coefficient
    // that is less than the level of the game, but not zero
    int nonZeroCoefficient(){
      int n = (rand()%(level+2) - (level));
      if ( n == 0 ){
      n++;
      }
      return n;
    }

    // generate non-zero random number

    int nonZeroRand(){
      int n=(rand()%randMax - (randMax));
      if (n == 0){
        n++;
      }
      return n;
    }

    int f = nonZeroCoefficient();
    int h = nonZeroCoefficient();
    int k = nonZeroRand();
    int g = nonZeroRand();

    a = f*h;
    b = (f*k + g*h);
    c = g*k;


    // shows the Level
    printf("\n\n\n\n --- Level %d ---", level);

    // generate the question
    printf("\nfactor the following expression\n%dx^2 %+dx %+d",a ,b, c);
    puts("\n\nEnter your answer in the form of (fX+g)(hX+k)");
    int inputF, inputG, inputH, inputK;
    printf("\nEnter f:");
    scanf("%d", &inputF);
    printf("Enter g:");
    scanf("%d", &inputG);
    printf("Enter h:");
    scanf("%d", &inputH);
    printf("Enter k:");
    scanf("%d", &inputK);

      if (inputF*inputH == a && ((inputF*inputK+inputG*inputH== b) && (inputG*inputK == c))){
        printf("\nRight on!\n");
        level++;

      }else{
        printf("\nSorry! your answer is not currect!\nthe currect answer is:\nf:%d\ng:%d\nh:%d\nk:%d\n \ntry again",f, g, h, k);
      }

    }


    do{
      ask();
      printf("\nDo you want to continue? (Y/N)");
      scanf (" %c", &ch);
    } while(ch == 'y'|| ch == 'Y');

}

Answer 1

Where you have your own bounds for rand, calculate them directly. Instead of

int nonZeroCoefficient(){
  int n = (rand()%(level+2) - (level));
  if ( n == 0 ){
  n++;
  }
  return n;
}

try

int nonZeroCoefficient(){
  int n = (rand()%(level+2) - (level)) + 1;
  return n;
    }

(modify the first part of the equation to suit). This also reduces a skew or bias in your randomness where anything <1 becomes one (i.e. the number 1 becomes over-represented in the random samples).

You also define your functions within your main(). This makes the code harder to read/understand and will make it harder to maintain. As a result, main() will only be:

int main() {
    do{
      ask();
      printf("\nDo you want to continue? (Y/N)");
      scanf (" %c", &ch);
    } while(ch == 'y'|| ch == 'Y');

}

As you don't return anything from main() or ask(), void will suffice. The following are the functions that should be defined separately from your main code

int nonZeroCoefficient() // noting the comment I made about about generating the random number
int nonZeroRand() // noting the comment I made about about generating the random number
int ask()

Both nonZeroCoefficient() and nonZeroRand() functions will be single line functions that could be replaced by a single function

int newRand(int f1, int f2){ //f1 could be level+2 or rand Max, F2 could be level or randMax
      int n = (rand()%(f1) - (f2)) + 1;
      return n;
}

Answer 2

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.

Answer 3

1. Standard C does not allow definition a function within another. Move definitions of functions like int ask() outside of main().

2. The if ( n == 0 ){ n++; } favors the return value of 1- a bias. Alternative:

   // return [-level...-1, 1...level]
   int nonZeroCoefficient(void) {
     int n = rand()%(2*level);
     if (n % 2) {
       return n/2 + 1;
     }
     return -(n/2 + 1);
   }
   

3. Likewise for nonZeroRand(). In this case, code could simply re-try to avoid the 1 bias.

   int nonZeroRand(void) {
     int n;
     do {
       int n=(rand()%randMax - randMax);
     } while (n == 0);
     return n;
   }
   

4. stdout may not be buffered as expected. Use fflush() to insure output is seen before asking for input.

   printf("\nEnter f:");
   fflush(stdoout); // add
   scanf("%d", &inputF);
   

5. Robust code checks the return value of user input.

   // scanf("%d", &inputF);
   if (scanf("%d", &inputF) != 1) Handle_Error();
   

6. Curious that code did not define f,g,h,k in alphabetically order.

   int f = nonZeroCoefficient();
   int h = nonZeroCoefficient();
   int k = nonZeroRand();
   int g = nonZeroRand();  // why last?
   

7. Formating to the width of the review presentation is appreciated.

   }else{
     printf("\nSorry! your answer is not currect!\nthe currect answer is:\nf:%d\ng:%d\nh:%d\nk:%d\n \ntry again",f, g, h, k);
   

8. ... versus (also check spelling)

   }else{
     printf("\nSorry! your answer is not correct!\n"
         "the correct answer is:\nf:%d\ng:%d\nh:%d\nk:%d\n \n"
         "try again",f, g, h, k);
   

9. Consider declaring/defining variables when needed.

   // int a, b, c, level=1, randMax=level*5;
   // .... many lines later 
   // a = f*h;
   // b = (f*k + g*h);
   // c = g*k;
   
   int a = f*h;
   int b = (f*k + g*h);
   int c = g*k;
   

10. Important tip for future coder efficiency (your efficiency). Do not manually format code as the below code implies. Use your coding environment's auto formatter (or get one that does that) that meets your group's coding style. Life is too short for such manual picayune tasks - like white-space concerns

    // OP's
    scanf("%d", &inputK);
    
      if (inputF*inputH == a && ...
    
    // Auto formated
    scanf("%d", &inputK);
    
    if (inputF * inputH == a && ....