expected ')' before string constant

[skillz21]

Hi!
I am an absolute Arduino noob and I want to get my coding skills up. I'm doing a project at the moment of a 'Maths Game' where I need to get two random numbers and print them like this to my display, like this lcd.print(val1 "+" val2). (i want to randomise the operator later on too, if you have any suggestions on how to do that plz help) I have to type the answer in with a 4x4 numerical keypad. The code I mentioned earlier doesn't work. The error pops up: "expected ')' before string constant" what did I do wrong?

Thx

Full code: (Bear in mind, it's not complete yet, this is what I have made so far by stitching together pieces of code from different library examples, yes I know I shouldn't do that, but I am hoping I can learn enough so that I can write it on my own some day)

#include <Keypad.h>
#include <Wire.h>
#include <LiquidCrystal_I2C.h>

const byte ROWS = 4;
const byte COLS = 4;



int val1;
int val2;
int result;

LiquidCrystal_I2C lcd(0x27, 16, 2);

char keys[ROWS][COLS] = {
  {'1','2','3','A'},
  {'4','5','6','B'},
  {'7','8','9','C'},
  {'*','0','#','D'}
};
byte rowPins[ROWS] = {5, 4, 3, 2};
byte colPins[COLS] = {8, 7, 6, 9};

Keypad keypad = Keypad( makeKeymap(keys), rowPins, colPins, ROWS, COLS );

void setup(){
  Serial.begin(9600);
    lcd.begin();
    lcd.clear();
    lcd.backlight();
}
 
void loop(){

char key = keypad.getKey();
 
lcd.setCursor(0, 0);
  lcd.print("Maths Game 1.0");
  lcd.setCursor(0, 1);
  lcd.print("Loading...");

  delay(3000);
 
val1 = random(0, 10);
val2 = random(0, 10);
result = val1+val2;

lcd.setCursor(0, 0);
lcd.print(val1 "+" val2);


 

  }

[samnmax]

The last line is not correct:

Code: [Select]

lcd.print(val1 "+" val2);
You cannot concatenate strings this way. Maybe look into the sprintf function.

(Edit) I forgot Arduino has string concatenation using the + operator. You should do somehing like val1 + "+" + val2. "+" is a string constant, not an operator.

[Ian.M]

C++ cant implicitly concatinate variables with a string literal, and lcd.print() doesn't work that way.  See https://www.arduino.cc/en/Reference/LiquidCrystalPrint

The easiest fix, (as the Arduino libraries make it difficult to use printf() except as sprintf(), and the printf() family functions use a lot of memory anyway) would be to do:

Code: [Select]

lcd.print(val1), lcd.print("+"), lcd.print(val2);
Edit: but see below re: converting the variables to strings and explicitly concatenating them with the + operator./i]

[Mechatrommer]

you cannot do that its not visual basic... search how to use itoa (convert integer value to string) and then store all of them in temporary string using strcat (concantenate strings), its a long story, google how to use them...

edit:

(Edit) I forgot Arduino has string concatenation using the + operator. You should do somehing like val1 + "+" + val2. "+" is a string constant, not an operator.

right, they have it, no need itoa and strcat... https://www.arduino.cc/en/Tutorial/StringAdditionOperator

[skillz21]

i did this instead :
lcd.setCursor(0, 0);
lcd.print(val1);
lcd.print("+");
lcd.print(val2);

[skillz21]

Also, small problem, this is my first time using the random function, on the LCD the numerals are continually being randomised, how can I make it only randomise once, and make it stay the same, until the next round?


edit: i used a state variable, so don't worry about this one

[skarecrow]

Keypad keypad = Keypad??? Is that how it's supposed to be? Also, shouldn't all your code be inside the setup and loop blocks?

Sent from my XT1565 using Tapatalk

[Brumby]

Also, shouldn't all your code be inside the setup and loop blocks?

Not if they are Functions.

[WaveyDipole]

Or global definitions of variables, constants or devices.

[joshtyler]

i did this instead :
lcd.setCursor(0, 0);
lcd.print(val1);
lcd.print("+");
lcd.print(val2);

If you want to do this in one statement, you can use the Arduino String library to convert integers to strings, then combine them etc. See the language reference here.

 For example you could do this:

Code: [Select]

int val1 = random(0,10);
int val2 = random(0,10);
lcd.print(String(val1)+"+"+String(val2));

As for wanting the random numbers to be the same each time. Simply set the random seed to a fixed value. I.e. put this in your setup() loop:

Code: [Select]

randomSeed(0); //This will make the random number function produce the same pseudo random number sequence each time
// Replace 0 with a different number for a different pseudo random sequence.
Library reference here.

[skillz21]

Hi
I changed the code around a bit (update at the end) and for some reason, the random function started returning the equation 7+49..... it is always the same, why is this happening?? Isn't it supposed to be random? also, how to I make it only type in numbers in the second line, and not letters and other characters? also, I need the "C" to clear the second line, at the moment it just types the letter in, how do I do this? I also want to apply this to an "enter" button.


(once again, it's not complete)

#include <Keypad.h>
#include <Wire.h>
#include <LiquidCrystal_I2C.h>

const byte ROWS = 4;
const byte COLS = 4;



int val1;
int val2;
int result;
int Random;
int Random2;
bool state;

LiquidCrystal_I2C lcd(0x27, 16, 2);

char keys[ROWS][COLS] = {
  {'1','2','3','A'},
  {'4','5','6','B'},
  {'7','8','9','C'},
  {'*','0','#','D'}
};
byte rowPins[ROWS] = {5, 4, 3, 2};
byte colPins[COLS] = {8, 7, 6, 9};

Keypad keypad = Keypad( makeKeymap(keys), rowPins, colPins, ROWS, COLS );

void setup(){
  Serial.begin(9600);
state = true;
    lcd.begin();
    lcd.clear();
    lcd.backlight();
   
    lcd.setCursor(0, 0);
  lcd.print("Maths Game 1.0");
  lcd.setCursor(0, 1);
  lcd.print("Loading...");

  delay(3000);
  lcd.clear();
 
}
 
void loop(){
  {
Random = random(0, 100);
Random2 = random(0, 100);
  }
char key = keypad.getKey();
 
if(state == true){
val1 = random(0, 100);
delay(10);
val2 = random(0, 100);
result = val1+val2;
state = false;


}

lcd.setCursor(0, 0);
lcd.print(val1);
lcd.print("+");
lcd.print(val2);

  lcd.setCursor(0, 1);
if(key)  {

  lcd.print(key);
 
  }

if (key == "C")  {
  lcd.setCursor(0, 1);
  lcd.print("                ");
  lcd.setCursor(0, 1);
  } 
 
if (key == "A")  {

  } 
 
  }

[skillz21]

Keypad keypad = Keypad??? Is that how it's supposed to be?

Yes probably, that's how it was in an example from the keypad library that I used.

[skarecrow]

Hi
I changed the code around a bit (update at the end) and for some reason, the random function started returning the equation 7+49..... it is always the same, why is this happening?? Isn't it supposed to be random? also, how to I make it only type in numbers in the second line, and not letters and other characters? also, I need the "C" to clear the second line, at the moment it just types the letter in, how do I do this? I also want to apply this to an "enter" button.


(once again, it's not complete)

#include <Keypad.h>
#include <Wire.h>
#include <LiquidCrystal_I2C.h>

const byte ROWS = 4;
const byte COLS = 4;



int val1;
int val2;
int result;
int Random;
int Random2;
bool state;

LiquidCrystal_I2C lcd(0x27, 16, 2);

char keys[ROWS][COLS] = {
  {'1','2','3','A'},
  {'4','5','6','B'},
  {'7','8','9','C'},
  {'*','0','#','D'}
};
byte rowPins[ROWS] = {5, 4, 3, 2};
byte colPins[COLS] = {8, 7, 6, 9};

Keypad keypad = Keypad( makeKeymap(keys), rowPins, colPins, ROWS, COLS );

void setup(){
  Serial.begin(9600);
state = true;
    lcd.begin();
    lcd.clear();
    lcd.backlight();
   
    lcd.setCursor(0, 0);
  lcd.print("Maths Game 1.0");
  lcd.setCursor(0, 1);
  lcd.print("Loading...");

  delay(3000);
  lcd.clear();
 
}
 
void loop(){
  {
Random = random(0, 100);
Random2 = random(0, 100);
  }
char key = keypad.getKey();
 
if(state == true){
val1 = random(0, 100);
delay(10);
val2 = random(0, 100);
result = val1+val2;
state = false;


}

lcd.setCursor(0, 0);
lcd.print(val1);
lcd.print("+");
lcd.print(val2);

  lcd.setCursor(0, 1);
if(key)  {

  lcd.print(key);
 
  }

if (key == "C")  {
  lcd.setCursor(0, 1);
  lcd.print("                ");
  lcd.setCursor(0, 1);
  } 
 
if (key == "A")  {

  } 
 
  }

No computer (that I've seen yet) can create a true random number. They can just generate pseudorandom numbers that will be the same every time. For example if you want a 5 number string it might come back with 29573, and every time you will get 29573. To get a different number you need to set a different random seed each time. Many languages let you randomize with a timer as a seed. Not sure if Arduino has that capability. Even if it does, there's a good chance your first set of random numbers will be the same each time if the boot process takes the same amount of time. Perhaps make it start after hitting a key so there will be very little chance of getting the exact same timing every time it starts up.

Sent from my XT1565 using Tapatalk

[TK]

C function rand() needs to be seeded using srand().  It must be done once, so in Arduino it is good to do it in setup{}.  srand() expects an argument that is the seed, seed can be something that is different everytime you run the code, i.e. time().

[joshtyler]

Hi
I changed the code around a bit (update at the end) and for some reason, the random function started returning the equation 7+49..... it is always the same, why is this happening?? Isn't it supposed to be random? also, how to I make it only type in numbers in the second line, and not letters and other characters? also, I need the "C" to clear the second line, at the moment it just types the letter in, how do I do this? I also want to apply this to an "enter" button.

C function rand() needs to be seeded using srand().  It must be done once, so in Arduino it is good to do it in setup{}.  srand() expects an argument that is the seed, seed can be something that is different everytime you run the code, i.e. time().

Arduino doesn't use srand(), it has it's own seed function called randomSeed(). It also has no concept of absolute time, so you can't seed with that (the closest you get is millis(), but this is time since the program elapsed, so would give the same seed every time!). A quick 'n' dirty way of seeding with Arduino is performing analogRead() on an unused pin.

The screen clear is not working because you are comparing a char array ("C") with a char ('C') (note the double vs single quotes). I'm surprised this doesn't generate a compiler warning.

skillz21, what you need to do is this:

In setup(), add this line:

Code: [Select]

randomSeed(analogRead(0)); //Replace 0 with an analog pin that you are not using
This will make the random numbers different each time.

And change

Code: [Select]

if (key == "C")
to:

Code: [Select]

if (key == 'C')
This should make your if statement work.

[brucehoult]

i did this instead :
lcd.setCursor(0, 0);
lcd.print(val1);
lcd.print("+");
lcd.print(val2);

If you want to do this in one statement, you can use the Arduino String library to convert integers to strings, then combine them etc. See the language reference here.

 For example you could do this:

Code: [Select]

int val1 = random(0,10);
int val2 = random(0,10);
lcd.print(String(val1)+"+"+String(val2));


Why on earth would you want to do that? The code with three calls to lcd.print() is going to be a lot faster, and result in a smaller program as well -- less generated code at that part of the program, less memory use, plus possibly not pulling things from the String library into the program at all.

[forrestc]

I changed the code around a bit (update at the end) and for some reason, the random function started returning the equation 7+49..... it is always the same, why is this happening?? Isn't it supposed to be random?

As others have mentioned, random numbers in most computers are actually generated by using an algorithm which means it's really a sequence, not a true set of random numbers.    Because the sequence is mathematically generated, the sequence of numbers can be very very long, but it really is just a sequence.

Many times, whenever a program starts, the sequence will be set to to same spot each time.   In the arduino library reference it includes the following note:

If it is important for a sequence of values generated by random() to differ, on subsequent executions of a sketch, use randomSeed() to initialize the random number generator with a fairly random input, such as analogRead() on an unconnected pin.

Personally I'd do something like the following:

Code: [Select]

void setup()
{
  //Use the somewhat random value of an unconnected pin
  //to seed the random number generator
  randomSeed(analogRead(0));
}

void loop()
{
  //Every pass through the loop, grab another random number.
  //This helps with randomness since we won't know how many
  //random numbers are grabbed between keypresses.
  random(0,10);

  //Put the rest of your code below.
}

This will seed the random number from the unconnected pin.   PLUS, it will ensure even improved randomness by continuing to run the random number generator sequence no matter what.   Since it sounds like your sketch will be driven by button presses, the exact timing of presses will ensure that the random number is truly random.

I would recommend waiting to pull the initial random number until you get the first button press.  You should find this is definitely sufficient for your needs (maybe a bit of overkill).

[joshtyler]

i did this instead :
lcd.setCursor(0, 0);
lcd.print(val1);
lcd.print("+");
lcd.print(val2);

If you want to do this in one statement, you can use the Arduino String library to convert integers to strings, then combine them etc. See the language reference here.

 For example you could do this:

Code: [Select]

int val1 = random(0,10);
int val2 = random(0,10);
lcd.print(String(val1)+"+"+String(val2));


Why on earth would you want to do that? The code with three calls to lcd.print() is going to be a lot faster, and result in a smaller program as well -- less generated code at that part of the program, less memory use, plus possibly not pulling things from the String library into the program at all.

The OP specifically asked in the original post how it could be done in a manner similar to:

Code: [Select]

lcd.print(val1 "+" val2)
I thought it would be useful to show how it could be done very similarly to this, and arguably produce more readable code.

In this situation it is trivial to use three calls to lcd.print(), but I think it is worth mentioning the existence of the string library, because it may be an asset if tackling a more complex problem.

The Arduino libraries are never going to be built for speed, and it can be hard to estimate off hand which is faster. Either approach is pretty horrific.
Using your method, the call stack will look as follows:

• print(int n, int base) will be called from the Arduino print library, which calls:
• print(long n, int base), which does runs through some if-else statements and calls:
• printNumber(unsigned long n, uint8_t base) which runs through a do while loop, including two divisions per iteration (horribly slow on embedded!, but divisions are necessary here), and sone logic, which invokes the inline function:
• write(uint8_t) from the base class, which calls several other functions including three separate I2C transactions (I can't be bothered to trace this, but a decent amount is involved)

This will be repeated for the two numbers, and the string will directly invoke write()

On the other hand, converting to a string involves:

• String(int value, unsigned char base) will be called from the String library, which calls init, and the itoa library function etc.
• Concatenation functions will then be called (I can't be bothered to figure out the full call stack, but there is a decent amount involved in this)
• A single call to write() will then occur

As you can see, it is not immediately obvious which approach uses less code/memory/is faster. I would argue however that using the string libraries give you the advantages of:

• Using the (potentially) more optimised compiler library instead of the Arduino integer to string function
• Vastly fewer I2C transactions

Given that blocking mode I2C transactions will take a fairly long amount of time, this could be significant.

This is besides the point however. I would like to think that my comment helped make a new person aware of functionality they hadn't previously known, whilst providing a good solution to a problem. In my mind is kind of the point of Arduino.

[skillz21]

(code update at the bottom)
I changed it, and the values are now random because it's reading a value of an unconnected analog pin. Now, the randomization works properly (the values are pretty large at the moment, but I'll make them smaller later. So I have two questions. One, when the screen says "press any button to start", it actually says"0+0ss any button to start"... I have no idea why it's doing this because the equation happens in the loop section, the "press any button to start" is in the setup. Secondly, how do I make it type the characters in one after another?(at the moment, when I can type in numbers(and letters) it replaces the number I previously typed, so I can't get more than one character typed...) I think this is happening because I make the cursor go to that spot beforehand, but I need to do this because the cursor is somewhere else beforehand. how do I fix this? also, I got the 'C' to clear the second line when I'm typing, but how do I make it so I can only enter in numbers and not letters and other characters? I tried to do this: "if (key == '1'|'2'|'3'|'4'|'5'|'6'|'7'|'8'|'9'|'0'){lcd.print(key);}"  It didn't work, the place where the number is supposed to go turns into some sort of glitchy square kinda thing.

code:
#include <Keypad.h>
#include <Wire.h>
#include <LiquidCrystal_I2C.h>

const byte ROWS = 4;
const byte COLS = 4;



int val1;
int val2;
int result;
int Random;
int Random2;
bool state;

LiquidCrystal_I2C lcd(0x27, 16, 2);

char keys[ROWS][COLS] = {
  {'1','2','3','A'},
  {'4','5','6','B'},
  {'7','8','9','C'},
  {'*','0','#','D'}
};
byte rowPins[ROWS] = {5, 4, 3, 2};
byte colPins[COLS] = {8, 7, 6, 9};

Keypad keypad = Keypad( makeKeymap(keys), rowPins, colPins, ROWS, COLS );

void setup(){

  Serial.begin(9600);

    lcd.begin();
    lcd.clear();
    lcd.backlight();
   
    lcd.setCursor(0, 0);
  lcd.print("Maths Game 1.0");
  lcd.setCursor(0, 1);
  lcd.print("Loading...");

  delay(3000);
  lcd.clear();
 
  lcd.setCursor(0, 0);
lcd.print("Press any Button");
lcd.setCursor(0, 1);
lcd.print("to Start");


 
}

void loop(){

char key = keypad.getKey();

if(key){
  lcd.clear();
  state = true;
}
  {
Random = random(0, 100);
Random2 = random(0, 100);
  }

 
if(state == true){
val1 = analogRead(A0);
val2 = analogRead(A1);
result = val1+val2;
state = false;


}



 
  lcd.setCursor(0, 0);
lcd.print(val1);
lcd.print("+");
lcd.print(val2);
   
 




  lcd.setCursor(0, 1);
if(key )  {
  lcd.print(key);
  }

if (key == 'C')  {
  lcd.setCursor(0, 1);
lcd.print("                ");
  } 
 
if (key == 'A')  {

  } 

  }

[TK]

getkey() seems to return even when no key is pressed, so 0+0 is being printed over "Pre" that was sent to the LCD during setup.  I think you need to use waitForKey() to loop until a key is pressed.

[joshtyler]

To allow only numerical keys, you have the syntax slightly wrong.
You tried:

Code: [Select]

if (key == '1'|'2'|'3'|'4'|'5'|'6'|'7'|'8'|'9'|'0'){lcd.print(key);}
The problems are:

• You want a logical or ("||"), instead of a bitwise or ("|")
• You need to put "key ==" for each letter. C evaluates each of the conditions separately

You want something like:

Code: [Select]

if(key == '1' || key == '2' || ....

As for entering the characters one after another, there are a couple of approaches.
Either you can have a state variable to remember when the first number has been entered, and set the cursor to the position of the second number on the second loop.
Or you can save the number a user enters into a variable, and print that to the screen.
Something like this maybe:

Code: [Select]

int userNum; //Store number given by user
...
void loop()
{
...
if(userNum == 0) //This will be true if the user has not entered a number yet
{
userNum = (key - '0')*10; //Add numerical value of key to userNum. The -'0' converts from ASCII to a normal number. Multiply by ten because this is the tens digit
//Perform what you want to do for the first number entered by the user
} else {
userNum += (key - '0'); //Add the units digit
//Perform what you want to do for the second number entered by the user
userNum = 0; //Reset
}

}

void loop()

The second approach is probably better, because it means that you could check the user's answer as well.

[TK]

keypad.getkey() is non blocking and returns always.  If no key was pressed, it returns NO_KEY.

[Ian.M]

You want something like:

Code: [Select]

if(key == '1' || key == '2' || ....


Why in %DEITY%'s name would you want to do it that way?

Code: [Select]

if(isdigit(key)){...would be the simplest, or if you don't want to use the character classification functions declared in ctype.h, and you know the platform you are using has a character set with the digits contiguous and in order (true of any system that uses ASCII or EDBIC, and GCC on Arduino uses ASCII), you could do:

Code: [Select]

if(key>='0' && key<='9'){...which uses two comparisons and a logical AND to simply check key is in the range of character codes used for the digits '0' to '9'.

[MarkS]

You want something like:

Code: [Select]

if(key == '1' || key == '2' || ....


Why in %DEITY%'s name would you want to do it that way

It illustrated the error and the correct way to do it. The fact that a function exists that simplifies this is immaterial. He clearly needs guidance on proper C coding.

[Ian.M]

It illustrated the error and the correct way to do it. The fact that a function exists that simplifies this is immaterial. He clearly needs guidance on proper C coding.

No.  It illustrated the error and showed a technically correct way to do it  However, to become a well rounded and competent C coder, in addition to technical competence with the language, the O.P. needs experience in detecting and resolving 'code smells'.  If you don't point out the fuglyness of the proposed technically correct method, you are failing to help the O.P. develop a nose for bad code.

The repetition of testing the value against each member of the set of digits is undesirable in a C flow control statement that takes a boolean control parameter, as its possible to test if the value is within the limits of the set, or simply to hand off the task to the standard libraries, either of which significantly improve readability.   If however the O.P. was using a switch() ... case statement, as standard ANSI/ISO C/C++ doesn't support ranges in case labels, there's no alternative to explicitly testing each possible digit, though that may indicate that using switch() was a poor implementation choice.

[brucehoult]

You want something like:

Code: [Select]

if(key == '1' || key == '2' || ....


Why in %DEITY%'s name would you want to do it that way?

Because it will work fine and is easily derivable from the basic rules of the C language? Something which the original poster is clearly having trouble with.

Sure, it's a bit more typing than some other solutions, but if it takes 3 us on an AVR instead of 0.5 us, of what concern is that when the human is hitting a couple of buttons a second?

And it saves you from having to know that the digits have consecutive codes in ASCII, or that there is a library with an isdigit() function. Those can come later.

Incidentally, I tried compiling:

Code: [Select]

int isdig(char c){
    return
        (c == '0') || (c == '1') || (c == '2') || (c == '3') || (c == '4') ||
        (c == '5') || (c == '6') || (c == '7') || (c == '8') || (c == '9');
}

gcc -O for ARM gave:

Code: [Select]

00000000 <isdig>:
   0: 3830      subs r0, #48 ; 0x30
   2: b2c0      uxtb r0, r0
   4: 2809      cmp r0, #9
   6: bf8c      ite hi
   8: 2000      movhi r0, #0
   a: 2001      movls r0, #1
   c: 4770      bx lr

gcc -O for RISC-V gave:

Code: [Select]

0000000000000000 <isdig>:
   0: fd05051b          addiw a0,a0,-48
   4: 0ff57513          andi a0,a0,255
   8: 00a53513          sltiu a0,a0,10
   c: 8082                ret

gcc -O for AVR gave:

Code: [Select]

00000000 <isdig>:
   0: 90 ed        ldi r25, 0xD0 ; 208
   2: 98 0f        add r25, r24
   4: 81 e0        ldi r24, 0x01 ; 1
   6: 9a 30        cpi r25, 0x0A ; 10
   8: 08 f0        brcs .+2      ; 0xc <__zero_reg__+0xb>
   a: 80 e0        ldi r24, 0x00 ; 0
   c: 08 95        ret

Even gcc -O for x86_64 gave:

Code: [Select]

0000000000000000 <isdig>:
   0: 83 ef 30              sub    $0x30,%edi
   3: 40 80 ff 09          cmp    $0x9,%dil
   7: 0f 96 c0              setbe  %al
   a: 0f b6 c0              movzbl %al,%eax
   d: c3                    retq   

So, there's no need to actually know the values are dense in ASCII and optimise it yourself, because the compiler is perfectly capable of doing it for you, even at only -O1.

[brucehoult]

It illustrated the error and the correct way to do it. The fact that a function exists that simplifies this is immaterial. He clearly needs guidance on proper C coding.

No.  It illustrated the error and showed a technically correct way to do it  However, to become a well rounded and competent C coder, in addition to technical competence with the language, the O.P. needs experience in detecting and resolving 'code smells'.  If you don't point out the fuglyness of the proposed technically correct method, you are failing to help the O.P. develop a nose for bad code.

The repetition of testing the value against each member of the set of digits is undesirable in a C flow control statement that takes a boolean control parameter, as its possible to test if the value is within the limits of the set, or simply to hand off the task to the standard libraries, either of which significantly improve readability.   If however the O.P. was using a switch() ... case statement, as standard ANSI/ISO C/C++ doesn't support ranges in case labels, there's no alternative to explicitly testing each possible digit, though that may indicate that using switch() was a poor implementation choice.

A fine example of the code smell "premature optimisation". See my previous post. :-) :-)

[MarkS]

No.  It illustrated the error and showed a technically correct way to do it  However, to become a well rounded and competent C coder, in addition to technical competence with the language, the O.P. needs experience in detecting and resolving 'code smells'.  If you don't point out the fuglyness of the proposed technically correct method, you are failing to help the O.P. develop a nose for bad code.

No, the technically correct method is the one shown by joshtyler. This is basic C syntax. There are many ways to do things correctly in C, and that is why I love the language, but it is important to show the mistake and how to resolve it. You showed a different way, and in this instance the better way, but there will not always be a function to help him out and he'll need to know how to do proper testing of variables. To be pedantic, your way wouldn't help him if the '*', "#', 'A', 'B', 'C' or 'D' keys were pressed, so the method that Josh showed would have had to come into play at some point.

A fine example of the code smell "premature optimisation". See my previous post. :-) :-)

Very true.