8
\$\begingroup\$

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

}
\$\endgroup\$
  • \$\begingroup\$ With nonZeroCoefficient(), what is the minimum acceptable return value? \$\endgroup\$ – chux - Reinstate Monica Jan 15 '18 at 18:18
5
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ (rand()%(level+2) - (level)) + 1; can generate 0. Incorrect for a function called nonZeroCoefficient() \$\endgroup\$ – chux - Reinstate Monica Jan 15 '18 at 17:13
  • \$\begingroup\$ @chux: I did say "(modify the first part of the equation to suit)". \$\endgroup\$ – AJD Jan 16 '18 at 19:12
  • \$\begingroup\$ OP's code does not return a 0 for nonZeroCoefficient() - a certain requirement for proper code usage. This answer's alternative suggestion breaks functionality (a bug) without warning - even if it does proved better statistical distribution. IMO, the "modify ... to suit)" outside of code is insufficient notice of the bug introduced. \$\endgroup\$ – chux - Reinstate Monica Jan 16 '18 at 19:22
  • \$\begingroup\$ @chux : sigh Missing the point. I provided a review and suggested a change. This does not always mean completely refactoring their code for them. I mentioned "modify" and implicitly left it as an exercise for them. Yes, I could have gone through and done some maths or perhaps even suggested putting it in a loop (while n=0, do random thing) which I thought of just now, but I didn't because I also have limited time in which to achieve many things throughout the day. I thought that my note would be enough of a prompt - the random bias was the important part from my perspective. \$\endgroup\$ – AJD Jan 17 '18 at 5:17
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for your comprehensive answer. I really learned a lot. Thanks As I said it's my first one. I would apply all the changes you suggested and keep them in mind in future works. Thanks again. 👍 \$\endgroup\$ – Ali Tahrei-Shalmani Jan 14 '18 at 22:24
  • \$\begingroup\$ Concerning const in function definition int nonZeroCoefficient(const int level) { - I have no problem with it there. Although not shown, I find const unneeded in a corresponding function declaration like int nonZeroCoefficient(const int level); versus int nonZeroCoefficient(int level);. The later is less clutter and follows the standard C functions declarations style. \$\endgroup\$ – chux - Reinstate Monica Jan 15 '18 at 19:15
  • \$\begingroup\$ Including "unneeded" standard headers files does have an advantage: It helps detect problems should code attempt to make a type, function or variable that collides. e.g. fma(). \$\endgroup\$ – chux - Reinstate Monica Jan 15 '18 at 19:20
  • \$\begingroup\$ OK, that's fair. It's a balancing act between those concerns and increasing build times due to increased number of includes. Not an issue here, but can become one in larger code bases. It's definitely a trade-off. \$\endgroup\$ – user1118321 Jan 15 '18 at 19:23
2
\$\begingroup\$
  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 && ....
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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