Characters again

[catasturslykid]

Hi again guys,

I'm doing the second program for the university and I've got a problem with a comparison between characters.

The user has to choose a gamemode (E or F).

#include <stdio.h> 
#include "tipus.h"




int main() { 
int error;
int i = 0;
int nCorrecte = 0;
int nJugadors = 0;
char cMode;
stPlayer jugador[4];
 


allegro_init(); 


install_keyboard(); 


error=set_gfx_mode(GFX_SAFE, 800, 600, 0, 0); 
if (error==0){ 






    while(nCorrecte == 0){
        printf("Gamemode <E o F>: ");
        fflush(stdin);
        scanf("%c",&cMode);
        printf("%c",cMode); //That's for check if the cMode is good.
        if(cMode == 'E' || cMode == 'F' || cMode == 'e' || cMode == 'f'){ //This comparison works great.
             nCorrecte = 1;
        }
        else{
            printf("Error! Mode de joc incorrecte!\n");
        }
     }
tauler(cMode); //Here is where I'm going to do the comparison.

}

And tauler:

void tauler(char cMode){
     stCasella casella[96];
     
     printf("%c\n",cMode); //That's for check the cMode again, it's fine.
     
     assignaCasella(casella);
     assignaEspecial(cMode, casella); //That's the function!
     printaCasella(casella);
     
     
}

void assignaEspecial(char cMode, stCasella casella[96]){
     FILE *f;
     char strFitxer[50];
     stFitxer fitxer[96];
     int i = 0;
     int proba = 5;
     
     
     printf("%c\n",cMode); //Works!
     
     
     if(cMode == 'F' ||cMode == 'f'){ //DOESN'T WORK
              printf("File name?: "); 
              scanf("%s",strFitxer);
              f = fopen(strFitxer,"r");
              if( f == NULL){
                  printf("Error! Fitxer NULL\n");
              }
              else{
                  fscanf(f,"%c",fitxer[i].cTipus);
                  fscanf(f,"%d", fitxer[i].nCasella);
                  while(!feof){ //Llegim el fitxer
                      i++;
                      fscanf(f,"%c",fitxer[i].cTipus);
                      fscanf(f,"%d", fitxer[i].nCasella);
                      if(fitxer[i].cTipus == 'B'){
                          fscanf(f,"%d",fitxer[i].nTorns);
                      }             
                  }
              }
              fclose(f);
              assignaFitxer(fitxer, casella, i);
     }
     if(cMode == 'E' || cMode == 'e'){ //DOESN'T WORK TOO
          
         
          for(i = 0; i < 96; i++){
                if(i == 59 || i == 80){
                     casella[i].nEspecial = 2;
                     casella[i].nColor = 6;
                }
                if( i == 66 || i == 90){
                    casella[i].nEspecial = 3;
                    casella[i].nColor = 0;
                }
                if( i == 5 || i == 10 || i == 17 || i == 23 || i == 30 || i == 38 || i == 42 || i == 50 || i == 56 || i == 64 || i == 70 || i == 73 || i == 77 || i == 82 || i == 92){
                    casella[i].nEspecial = 1;
                    casella[i].nColor = 7;
                }
          }
     }
     
              
}

So, what's happening?

[anduril462]

For starters, once you read in the mode character from the user, use toupper/tolower to convert everything to one case, then you can simply your if checks:

scanf("%c", &cMode);
cMode = tolower(cMode);
if (cMode == 'e' || cMode == 'f')  // much simpler

Also, what are all those magic numbers in assignaEspecial? #define some constants, for the length of fitxer and strFitxer, and for all the arbitrary numbers in 'E' mode (59, 80, 66, 90, 5, 10, etc) and the values used for nEspecial and nColor. In fact, you should have no magic numbers anywhere in your code, ever.

So you're saying the printf at the copy of assignaEspecial prints the correct value for cMode? Are you sure it's that printf and not one of the others showing up (you don't have a \n in the printf in main, meaning the output may buffer and not show up as expected, throwing you off of which printf you think you're looking at)? You might want to put the name of the function or the line you're on, when printing, to make sure you're seeing the right output, and that cMode has the value you expect where you expect it.

printf("%s: %d: %s: cMode = %c\n", __FILE__, __LINE__, __func__, cMode);

__FILE__, __LINE__ and __func__ are macros that the compiler will fill in with the correct file name, line number and function name, for where they are used. Very handy for debugging/logging.

Your best bet, however, is to learn to use a debugger. Then you can step through the code and inspect the value of cMode after every instruction and see what changes it.

If the value seems to change out of the blue, my bet would be a buffer overflow, but hard to say without running the code with output that we know will cause a failure, and watching it in a debugger.

[catasturslykid]

Well, first of all, the "magic numbers" means the "spaces" of the game (I don't know the word in english... box? square?).

That's the board:



So, if the FILE says that there's a special "box" in the box number, for example 38, the color of the box will change and the nEspecial specificate what kind of special box it is.

In the other hand, I wanna show you what I see while writing what you told me:



I've put a...

printf("%c",cMode);

... just before the "basic debugging tool" and that's what I see.
There's nothing in the "debugging printf" but it is in the other printf.

[oogabooga]

Nothing seems wrong with what you've posted (except that it's non-standard to fflush(stdin)).
You need to post the entire program.

[anduril462]

EDIT: What oogabooga said is an understatement. fflush(stdin) is undefined behavior. This theoretically could be the cause of your problem, but it's unlikely.

Put the __FILE__, __LINE__ and __func__ in every debug printf, to be sure you're seeing the right values everywhere. I'm still not 100% sure where the first or second 'F' are being printed from.

Woah! I just saw this in your cMode == 'F' code:

while(!feof){

That is doubly broken. First, feof is a function. As is, you're simply looping while the feof function has an address of NULL. Since it's a real function that actually exists (not a function pointer pointing to NULL), it will never be NULL, thus your loop never runs. Second, don't use feof to control your file reading loops. Read this: FAQ > Why it's bad to use feof() to control a loop - Cprogramming.com. Use the return value of fscanf to control your loop, or a boolean variable called something like done or valid_read. Make sure you check the return values of all your file-related function calls, to prevent running your program with bad/incomplete data if there's a problem with your file. Especially for cMode == 'F' which seems to be a user-provided file (users are great at providing incorrect input/files to your programs).

Box, square or space would all be appropriate terms. I might avoid space, however, since it can easily be confused with the space character in programming context.

I suppose here the magic numbers are more acceptable, but some comments and #defines would be helpful:

#define NUM_CASELLAS 96  // use everywhere for clarity, and in case you ever change the number of boxes you have

#define TIPUS_CORE_DUMPED 2
#define COLOR_NEGRO 6
// similar for other types/colors
...
void tauler(char cMode){
    stCasella casella[NUM_CASELLAS];
    ....      
    printaCasella(casella);
}


void assignaEspecial(char cMode, stCasella casella[]){  // don't need the size, but if you want it, use NUM_CASELLAS
...
    for (i = 0; i < NUM_CASELLAS; i++) {
        if(i == 59 || i == 80){
            // box 59 and 80 are Core Dumped
            casella[i].nEspecial = TIPUS_CORE_DUMPED;
            casella[i].nColor = COLOR_NEGRO;
        }
        else if( i == 66 || i == 90){  // using else-if keeps the program from making unnecessary checks, if i is 59 or 80, it can't be 66 or 90, and allows for default case
        }
        else if(i == <5, 10, 17, ...>){
        }
        else{
            // default case for all other boxes -- depending on how you initialize, this might not be necessary
        }
    }
...

Alternatively, you can use a switch statement for your initialization, and take advantage of the fall-through property. I find this a bit easier to read personally, especially for long lists like your third if statement:

void assignaEspecial(char cMode, stCasella casella[]){
...
    for (i = 0; i < NUM_CASELLAS; i++) {
        switch (i) {
            case 59:
            case 80:
                // box 59 and 80 are Core Dumped
                casella[i].nEspecial = TIPUS_CORE_DUMPED;
                casella[i].nColor = COLOR_NEGRO;
                break;

            case 66:
            case 90:
                // boxes 66 and 90 are XXX
                break;

            case 5:
            case 10:
            case 17:
            ...
                // these boxes are YYY
                break;

            default:
                // default case for all other boxes -- depending on how you initialize, this might not be necessary
                break;
        }
    }
    ...
}

[catasturslykid]

oogabooga, here it is:

cBoard2.zip

It's not finish and it crashes after printing the board (that's something that I have to check out too, because I don't know where it crashes).

Ok andruil, let me have a look and I will tell you something...

[oogabooga]

EDIT: What oogabooga said is an understatement. fflush(stdin) is undefined behavior. This theoretically could be the cause of your problem, but it's unlikely.

I'd say I erred! If it's undefined then you should never do it.

It's apparently allowed in MS, though:
"If the stream is open for input, fflush clears the contents of the buffer." -- fflush at MSDN

[catasturslykid]

Ok, I write what you (aldruin) told me in every debug printf and this is what I see:



The printf before the last "debugg tool" is not printed.

And the while(!feof) is the only way that our teachers show us to know where the File ends... I could use another method, but I don't know if they will aprobe it, and I need to pass this subject.

[anduril462]

Note, something else happens between the printf in tauler and the assignaEspecial. Namely, assignaCasella is called. I'm guessing you have a buffer overflow in there. Please post that function. Also, put another printf, as below

void tauler(char cMode){
     stCasella casella[96];
      
     printf("%s: %d: %s: cMode = %c\n", __FILE__, __LINE__, __func__, cMode);


     assignaCasella(casella);
     printf("%s: %d: %s: cMode = %c\n", __FILE__, __LINE__, __func__, cMode);  // here, see if assignaCasella has corrupted cMode
     assignaEspecial(cMode, casella); //That's the function!
     printaCasella(casella);
      
      
}

EDIT:

And the while(!feof) is the only way that our teachers show us to know where the File ends... I could use another method, but I don't know if they will aprobe it, and I need to pass this subject

I'm almost positive that is not exactly how your teachers showed you, unless you use a non-standard feof in a special library just for your classes. More likely, your teacher is teaching you the wrong thing (quite common, sadly). Using feof to control the file reading loop causes you to read the last element twice. If you're not sure whether other methods are safe, show that link to your professor and ask if you can use a different method (be polite, like "Hey, I saw this link and was wondering if this problem applies to our file-reading loops. Should/May I use another method that avoids this problem?"). If your professor is wrong, and wont accept a more correct method, then you might consider complaining to a higher level, like the department head.

[catasturslykid]

Yes, something happens in assignaCasella because I write "printf("%s: %d: %s: cMode = %c\n", __FILE__, __LINE__, __func__, cMode);" as you said and this is what happens:



Here is the "assignaCasella" (LONG CODE, but repetitive)

void assignaCasella(stCasella casella[96]){
 int i;    
         //ASSIGNO CASELLA INICIAL
     
     casella[0].nX1 = 0;
     casella[0].nX2 = 120;
     casella[0].nY1 = 540;
     casella[0].nY2 = 600;
     casella[0].nTextx = 4;
     casella[0].nTexty = 544;
     casella[0].nColor = 3;
     casella[0].nColortext = 4;
     casella[0].nTorns = 0;
     
     for( i = 1; i <= 8; i++){ //Assignem primera fila
          
          casella[i].nX1 = casella[i-1].nX2;
          casella[i].nX2 = casella[i-1].nX2 + 60;
          if( i == 1){//La primera casella es mes gran.
              casella[i].nTextx = 124;
          }
          else{
               casella[i].nTextx = casella[i-1].nTextx + 60;
          }
          casella[i].nY1 = 540;
          casella[i].nY2 = 600;       
          casella[i].nTexty = 544;
          casella[i].nColor = 11;
          casella[i].nColortext = 4;
          casella[i].nTorns = 0;
     }
      //Assigem de manera manual la casella 9
      casella[9].nX1 = 540;
      casella[9].nX2 = 600;
      casella[9].nY1 = 480;
      casella[9].nY2 = 540;
      casella[9].nTextx = 540;
      casella[9].nTexty = 484;
      casella[9].nColor = 11;
      casella[9].nColortext = 4;
      casella[9].nTorns = 0;
      
      //Segona fila
     for ( i = 10; i <= 18; i++){
         
         casella[i].nX1 = casella[i-1].nX1 - 60; 
         casella[i].nX2 = casella[i-1].nX1;
         casella[i].nY1 = 480;
         casella[i].nY2 = 540;
         casella[i].nTextx = casella[i-1].nTextx - 60;
         casella[i].nTexty = 484;
         casella[i].nColor = 11;
         casella[i].nColortext = 4;
         casella[i].nTorns = 0;
         
     }
     //Assignem de manera manual la casella 19
      casella[19].nX1 = 0; 
      casella[19].nX2 = 60;
      casella[19].nY1 = 420;
      casella[19].nY2 = 480;
      casella[19].nTextx = 4;
      casella[19].nTexty = 424;
      casella[19].nColor = 11;
      casella[19].nColortext = 4;
      casella[19].nTorns = 0;
      
        for( i = 20; i <= 28; i++){ //Assignem tercera fila
          
          casella[i].nX1 = casella[i-1].nX2;
          casella[i].nX2 = casella[i-1].nX2 + 60;
          casella[i].nY1 = 420;
          casella[i].nY2 = 480;
          casella[i].nTextx = casella[i-1].nTextx + 60;
          casella[i].nTexty = 424;
          casella[i].nColor = 11;
          casella[i].nColortext = 4;
          casella[i].nTorns = 0;
        }  
        
         //Assignem de manera manual la casella 29
      casella[29].nX1 = 540; 
      casella[29].nX2 = 600;
      casella[29].nY1 = 360;
      casella[29].nY2 = 420;
      casella[29].nTextx = 540;
      casella[29].nTexty = 364;
      casella[29].nColor = 11;
      casella[29].nColortext = 4;
      casella[29].nTorns = 0;
      //Cuarta fila
      for ( i = 30; i <= 38; i++){
         
         casella[i].nX1 = casella[i-1].nX1 - 60; 
         casella[i].nX2 = casella[i-1].nX1;
         casella[i].nY1 = 360;
         casella[i].nY2 = 420;
         casella[i].nTextx = casella[i-1].nTextx - 60;
         casella[i].nTexty = 364;
         casella[i].nColor = 11;
         casella[i].nColortext = 4;
         casella[i].nTorns = 0;
         
     }
     
      casella[39].nX1 = 0; 
      casella[39].nX2 = 60;
      casella[39].nY1 = 300;
      casella[39].nY2 = 360;
      casella[39].nTextx = 4;
      casella[39].nTexty = 304;
      casella[39].nColor = 11;
      casella[39].nColortext = 4;
      casella[39].nTorns = 0;
      
      for( i = 40; i <= 48; i++){ //Assignem tercera fila
          
          casella[i].nX1 = casella[i-1].nX2;
          casella[i].nX2 = casella[i-1].nX2 + 60;
          casella[i].nY1 = 300;
          casella[i].nY2 = 360;
          casella[i].nTextx = casella[i-1].nTextx + 60;
          casella[i].nTexty = 304;
          casella[i].nColor = 11;
          casella[i].nColortext = 4;
          casella[i].nTorns = 0;
        } 
        
      casella[49].nX1 = 540; 
      casella[49].nX2 = 600;
      casella[49].nY1 = 240;
      casella[49].nY2 = 300;
      casella[49].nTextx = 540;
      casella[49].nTexty = 244;
      casella[49].nColor = 11;
      casella[49].nColortext = 4;
      casella[49].nTorns = 0;
            
      for ( i = 50; i <= 58; i++){
         
         casella[i].nX1 = casella[i-1].nX1 - 60; 
         casella[i].nX2 = casella[i-1].nX1;
         casella[i].nY1 = 240;
         casella[i].nY2 = 300;
         casella[i].nTextx = casella[i-1].nTextx - 60;
         casella[i].nTexty = 244;
         casella[i].nColor = 11;
         casella[i].nColortext = 4;
         casella[i].nTorns = 0;
         
      }
      casella[59].nX1 = 0; 
      casella[59].nX2 = 60;
      casella[59].nY1 = 180;
      casella[59].nY2 = 240;
      casella[59].nTextx = 4;
      casella[59].nTexty = 184;
      casella[59].nColor = 11;
      casella[59].nColortext = 4; 
      casella[59].nTorns = 0;    
      
      for( i = 60; i <= 68; i++){ //Assignem tercera fila
          
          casella[i].nX1 = casella[i-1].nX2;
          casella[i].nX2 = casella[i-1].nX2 + 60;
          casella[i].nY1 = 180;
          casella[i].nY2 = 240;
          casella[i].nTextx = casella[i-1].nTextx + 60;
          casella[i].nTexty = 184;
          casella[i].nColor = 11;
          casella[i].nColortext = 4;
          casella[i].nTorns = 0;
        } 
        
      casella[69].nX1 = 540; 
      casella[69].nX2 = 600;
      casella[69].nY1 = 120;
      casella[69].nY2 = 180;
      casella[69].nTextx = 540;
      casella[69].nTexty = 124;
      casella[69].nColor = 11;
      casella[69].nColortext = 4; 
      casella[69].nTorns = 0; 
      
      for ( i = 70; i <= 78; i++){
         
         casella[i].nX1 = casella[i-1].nX1 - 60;
         casella[i].nX2 = casella[i-1].nX1;
         casella[i].nY1 = 120;
         casella[i].nY2 = 180;
         casella[i].nTextx = casella[i-1].nTextx - 60;
         casella[i].nTexty = 124;
         casella[i].nColor = 11;
         casella[i].nColortext = 4;
         casella[i].nTorns = 0;
         
      }
      
      casella[79].nX1 = 0; 
      casella[79].nX2 = 60;
      casella[79].nY1 = 60;
      casella[79].nY2 = 120;
      casella[79].nTextx = 4;
      casella[79].nTexty = 64;
      casella[79].nColor = 11;
      casella[79].nColortext = 4; 
      casella[79].nTorns = 0;
      
      for( i = 80; i <= 88; i++){ //Assignem tercera fila
          
          casella[i].nX1 = casella[i-1].nX2;
          casella[i].nX2 = casella[i-1].nX2 + 60;
          casella[i].nY1 = 60;
          casella[i].nY2 = 120;
          casella[i].nTextx = casella[i-1].nTextx + 60;
          casella[i].nTexty = 64;
          casella[i].nColor = 11;
          casella[i].nColortext = 4;
          casella[i].nTorns = 0;
        } 
        
      casella[89].nX1 = 540; 
      casella[89].nX2 = 600;
      casella[89].nY1 = 0;
      casella[89].nY2 = 60;
      casella[89].nTextx = 540;
      casella[89].nTexty = 4;
      casella[89].nColor = 11;
      casella[89].nColortext = 4; 
      casella[89].nTorns = 0;
      
       for ( i = 90; i <= 95; i++){
         
         casella[i].nX1 = casella[i-1].nX1 - 60; 
         casella[i].nX2 = casella[i-1].nX1;
         casella[i].nY1 = 0;
         casella[i].nY2 = 60;
         casella[i].nTextx = casella[i-1].nTextx - 60;
         casella[i].nTexty = 4;
         casella[i].nColor = 11;
         casella[i].nColortext = 4;
         casella[i].nTorns = 0;
         
      }
      
      casella[96].nX1 = 240; 
      casella[96].nX2 = 0;
      casella[96].nY1 = 0;
      casella[96].nY2 = 60;
      casella[96].nTextx = 4;
      casella[96].nTexty = 4;
      casella[96].nColor = 10;
      casella[96].nColortext = 4; 
      casella[96].nTorns = 0;
      
      }

EDIT: They showed us that that using a feof is not the only way to read the finish of the file but, this year, is the only method we are going to learn. All the time we use feof it worked...

I'm almost positive that is not exactly how your teachers showed you, unless you use a non-standard feof in a special library just for your classes. More likely, your teacher is teaching you the wrong thing (quite common, sadly). Using feof to control the file reading loop causes you to read the last element twice. If you're not sure whether other methods are safe, show that link to your professor and ask if you can use a different method (be polite, like "Hey, I saw this link and was wondering if this problem applies to our file-reading loops. Should/May I use another method that avoids this problem?"). If your professor is wrong, and wont accept a more correct method, then you might consider complaining to a higher level, like the department head.

[ZuK]

      casella[96].nX1 = 240;
      casella[96].nX2 = 0;
      casella[96].nY1 = 0;
      casella[96].nY2 = 60;
      casella[96].nTextx = 4;
      casella[96].nTexty = 4;
      casella[96].nColor = 10;
      casella[96].nColortext = 4;
      casella[96].nTorns = 0;

Out of bounds

Kurt

[anduril462]

I told you you had a buffer overflow. Kurt is right. If you have an array of 96 elements, the only valid indexes are 0-95. 96 is out of bounds and corrupting other variables, namely cMode.

Whenever you have a 2-d structure, you should think loops. Nested loops, or a single loop that has a modulo (remainder -- % operator) or counter to track when it should start processing a new row.

There is a systematic way to convert casella index into row/column. Notice you have repeating groups of 20: 9-28 is basically the same as 29-48, 49-68 and 69-88. You have "special" cases for 1-8 and 89-94, but not really. They follow the same mapping, but are incomplete, i.e. they don't have 20 elements in their group. It would take me a few minutes to sort out the mapping, but it would also be a good exercise for you:

int calc_x1(int i)
{
    // return the appropriate x1 coordinate for this index
}
...
for (i = 0; i < NUM_CASELLAS; i++) {
    // some trickery with modulo (%) and divide (/) operators
    // and some basic multiplication and addition/subtraction
    // should give you a nice easy way to say
    casella[i].nX1 = calc_x1(i);
    casella[i].nY1 = calc_y1(i);
    // same for x2, y2 and text locations
}

[catasturslykid]

Ok, thank you so much, guys!

That was the problem.

Special thanks to anduril462 for all your help and ideas!

I have to finish the program in 2 days so, when I deliver the program, I will study all your ideas and do a "remake" of the program with your ideas.

Thank you again!