Readability for cs50. There valuable changes for no reason at "if" line

Question

So I started programming 5 days ago. I'm going through course cs50. There is a task (see https://cs50.harvard.edu/x/2020/psets/2/readability/) to make a program which evaluates grade of text. I did it.

#include <stdio.h>
#include <cs50.h>
#include <string.h>
#include <ctype.h>
#include <math.h>

int main(void) {
  int ln = 0;
  int wn = 1;
  int sn = 0;
  string text = get_string("write your text:\n");
  int l = strlen(text);
  for (int i = 0; i < l; i++) {
    if (isalpha(text[i])) {
      ln++;
    }
    if ((char) (text[i]) == (char) (' ')) {
      wn++;
    }

    if ((char) (text[i]) == (char) ('.') | (char) (text[i]) == ('!')
        | (char) (text[i]) == ('?')) {
      sn++;
    }
  }
  float grade = ((float) ((ln / wn * 100) * 0.0588)
      - ((float) ((sn / wn * 100) * 0.296)) - 15.8);

  if (grade > 1 && grade < 16) {
    printf("Grade %f\n", grade);
  } else if (grade < 1) {
    printf("Before grade 1\n");
  } else if (grade > 16) {
    printf("grade 16+\n");
  }

  printf("%i, %i, %i", ln, wn, sn);
}

And when I uses debugger, I can see that at that long line, where I do all the math, float grade is equal just to the number I need, everything is fine. But right after it, where "if" starts, it becomes 1.8 for no reason. I tried to change different parameters, and the math is still right till the if line. What do I do wrong?

Aside: consider `(char)(text[i])==(char)('.') | (char)(text[i])==('!') | (char)(text[i])==('?')` --> `text[i] == '.' || text[i] == '!' || text[i] == '?'` — chux - Reinstate Monica, Oct 21 '20 at 12:23
Sometimes optimisations get in the way. What happens if you ouput `grade` a few times? Like every second line in your program. If you know how to, try building without optimisations if you intend to debug. — Yunnosch, Oct 21 '20 at 12:23
Sorry, didn't get about optimisation. I don't think that i should output it a few times. In a debugger i can see that at the line of math, grade ==the number i need. Right at the next line it jumps to very strange number. And remains to be this number to the end — VVS232, Oct 21 '20 at 12:26
I did not mean to really permanently change your code to do unneeded outputs. But for getting an insight into how the compiler "sees" the variable it is the way to temporarily find out. It is OK if you do not see the influence of optimisations here, do not worry. Since I have had support by others please trust my proposal as "hidden magic" try it, see the values it shows you. If THEY still are weird, then things become more interesting. I predict that they show the value you expect. Outputting them forces the compiler to be more true to your code. — Yunnosch, Oct 21 '20 at 12:33
Ok. I'll just trust you, though didn't get it quite well. Thank you. I still have a lot to get used to. I'll see how to works — VVS232, Oct 21 '20 at 12:40
Try. If I was wrong then just ignore me (though I hope for an insightful answer by others in that case). If I am right and you really want to know I can make an answer with some explanation. — Yunnosch, Oct 21 '20 at 12:43
Just a friendly warning, the CS50 library is an attempt to paint a friendly face on standard C, and in the process misrepresents how the language really works. The `string` type doesn't really represent a *string*, and the `get_string` function doesn't behave like most C I/O routines. They are very useful abstractions, but they're not part of the language itself and if you start writing C outside of the CS50 environment you may be confused about some things. Not saying it's bad, just that you need to be aware of it. — John Bode, Oct 21 '20 at 18:39
OT: when compiling, always enable the warnings, then fix those warnings. (for `gcc`, at a minimum use:`-Wall -Wextra -Wconversion -pedantic -std=gnu11` ) Note: other compilers use different options to produce the same results. — user3629249, Oct 22 '20 at 00:16
OT: regarding the indenting of the code: a 2 space indent can be lost when using a variable width font. Strongly suggest each indent level be 4 spaces — user3629249, Oct 22 '20 at 00:17
regarding `if (grade > 1 && grade < 16) {` and following lines of code: this block of code ignores a `grade` of 1 or 16. Perhaps you meant: `if (grade >= 1 && grade <= 16) {` Note the `>=` and `<=` operators — user3629249, Oct 22 '20 at 00:20
OT: regarding: `if ((char) (text[i]) == (char) (' ')) {` and similar statements. There is (rarely) a need to cast a character using `(char) — user3629249, Oct 22 '20 at 00:31
OT: regarding statements like: `float grade = ((float) ((ln / wn * 100) * 0.0588) - ((float) ((sn / wn * 100) * 0.296)) - 15.8);` This forces the compiler to produce lots of code to convert between `int` (100) and `double` (0.588, 0.296, 15.8) That 100 would be better written as a `float` (100.0f) Note the decimal point and the `f` Those `double` values would be better written as `float` 0.588f, 0.296f and 15.8f. Any time the compiler is performing an implicit conversion, there is always the possibility of an error occurring — user3629249, Oct 22 '20 at 00:42
OT: regarding: `#include ` it is a very poor programming practice to include header files those contents are not used. Suggest removing that statement. — user3629249, Oct 22 '20 at 00:47
Please clarify what the 'correct value was, and exactly which line the value changed to 1.8. Please post exactly what input is being given to the program — user3629249, Oct 22 '20 at 00:56

chux - Reinstate Monica · Accepted Answer · 2020-10-21T12:47:43.790

5

Code at least has this problem: Nothing printed when grade is 1 or 16

  // Nothing printed when grade is 1 or 16
  if (grade > 1 && grade < 16) {
    printf("Grade %f\n", grade);
  } else if (grade < 1) {
    printf("Before grade 1\n");
  } else if (grade > 16) {
    printf("grade 16+\n");
  }

Suggest

  //        >=            <=
  if (grade >= 1 && grade <= 16) {
    printf("Grade %f\n", grade);
  } else if (grade < 1) {
    printf("Before grade 1\n");
  } else {
    printf("grade 16+\n");
  }

Integer division likely wrong. Use FP division.

//                          vvvvvvv                               vvvvvvv 
// float grade = ((float) ((ln / wn * 100) * 0.0588) - ((float) ((sn / wn * 100) * 0.296)) - 15.8);
float grade = 0.0588 * ln / wn * 100 - 0.296 * sn / wn * 100 - 15.8;

Improvements

// if ((char) (text[i]) == (char) (' ')) {
if (text[i] == ' ') {

// if ((char) (text[i]) == (char) ('.') | (char) (text[i]) == ('!') | (char) (text[i]) == ('?')) {
if (text[i] == '.' || text[i] == '!' || text[i] == '?') {

edited Oct 21 '20 at 12:47

answered Oct 21 '20 at 12:34

chux - Reinstate Monica

143,097
13
135
256

Yeah, definitely a problem i didn't see. Thanks. It doesn't help my question, but this also has to be improved. Thank you a lot – VVS232 Oct 21 '20 at 12:41
2

@VVS232 Code has other issues too. – chux - Reinstate Monica Oct 21 '20 at 12:46
@ Reinstate Monica Solved! Your FP division solved everything. Works just fine. Amazing. But... one more question. fp - float point, i googled it. But.. what did you change? How to use fp divison instead of integer one? I thought i used it when i wrote "(float)" before division – VVS232 Oct 21 '20 at 12:46
1

@VVS232 `0.0588 * ln / wn * 100` is like `((0.0588 * ln) / wn) * 100`. FP (floating-point) product occurs before the FP division. `(sn / wn * 100)` is an `int` division followed by an `int` multiply. Your `(float)` applies _after_ `(sn / wn * 100)` is done - too late to affect the math. You could have done `(float) ln / wn * 100 * 0.0588`, but better to avoid casts. – chux - Reinstate Monica Oct 21 '20 at 12:50
@VVS232 Aside: Use `double` as the default type FP type. Little reason to use `float grade` here. – chux - Reinstate Monica Oct 21 '20 at 12:57

Readability for cs50. There valuable changes for no reason at "if" line

1 Answers1