Reputation: 101
Here is a code i wrote for pin verification, but i cannot make the system verify the new pin. In this program, input is taken from user and is verified using the pre-defined pin. If the user fails to verify the code, then a new code is presented which is to be the new pin.
#include<stdio.h>
#include<stdlib.h>
#include<strings.h>
#include<windows.h>
int pin();
int main()
{
int pin();
{
int pin,new_pin,old_pin;
{
old_pin=1111;
printf("Enter your pin: ");
scanf("%d", &pin);
if(pin==old_pin)
{
printf("PIN succefully verified \n");
}
else if(pin!=old_pin)
{
printf("PIN couldn't be verified. \n");
printf("Press 1 to generate a new pin, 2 to re-enter or 0 to exit\n");
scanf("%d", &new_pin);
if (new_pin==1)
{
printf("Your new pin is: \n");
//generating random 4numbered pin
int i,random;
for(i=0; i<3; i++)
random= (rand() );
printf("%d\n", random);
main();
random=new_pin;
new_pin=old_pin;
}
else if (new_pin==2) {
main();
}
else{
printf("Exiting the program....\n");
exit;
}
}
}
}
return(0);
}
Upvotes: 1
Views: 110
Reputation:
First of all, there is need to revisit the code. You reference to function called pin()
, that source code we don't know. Next, we run into tiny conflict of names (int pin;
, int pin();
). There are way too many code blocks. Variables are named poorly (new_pin
isn't new pin, old_pin
isn't old). Loop for generating pin is useless as you override variable without storing old randoms. Multiline strings without \
in printf
's (and others) are forbidden (I spotted them somewhere near Your original code). exit
is missing paranthesis. That's about syntax, code optimization and naming quality.
The code is recursively calling main, and that's certainly not a good idea (unless You know, what You are doing!)
I've formatted it and removed int pin();
declarations as they were redudant. Complexity of code blocks was reduced and unnecessary library imports were stripped. The for loop was simplified (as it was redudant). Code looks now like this:
#include <stdio.h>
#include <stdlib.h>
int main(void) {
int pin, new_pin, old_pin = 1111;
printf("Enter your pin: ");
scanf("%d", &pin);
if (pin == old_pin)
printf("PIN succefully verified.\n");
else if (pin != old_pin) {
printf("PIN couldn't be verified.\n");
printf("Press 1 to generate a new pin, 2 to re-enter or 0 to exit\n");
new_pin = getchar();
if (new_pin == '1') {
int i, random;
printf("Your new pin is: %4d\n" rand() % 9999);
main();
} else if (new_pin=='2')
main();
else {
printf("Exiting the program....\n");
return 0;
}
}
}
Now, let's implement the functionality was made incorrectly. New pin mechanism was broken so hard, there is no sense in explaining it. It's better to get brand new one. Values were staticized so they won't disappear after creating new local variable set is made.
#include <stdio.h>
#include <stdlib.h>
int main(void) {
static int pin, new_pin, validPin = 1111;
printf("Enter your pin: ");
scanf("%d", &pin);
if (pin == validPin)
printf("PIN succefully verified.\n");
else if (pin != validPin) {
printf("PIN couldn't be verified.\n");
printf("Press 1 to generate a new pin, 2 to fall back or any other key to exit.\n");
new_pin = getchar();
if (new_pin == '1') {
validPin = rand() % 9999;
printf("Your new pin is: %4d.\n", validPin);
main();
} else if (new_pin=='2')
main();
else {
printf("Exiting the program.\n");
return 0;
}
}
}
Now, let's revisit main recursion antipattern and divide the code into subprocedures.
#include <stdio.h>
#include <stdlib.h>
int proc() {
/* ... the code in partially unmodified form goes here. */
}
int main(void) {
while(!proc());
}
Finally, let's mess around with variable scope to reduce it to the absolute minimum and put exit message at the end of main. Code at the end looks like this:
#include <stdio.h>
#include <stdlib.h>
int proc() {
int pin;
static int validPin = 1111;
printf("Enter your pin: ");
scanf("%d", &pin);
if (pin == validPin)
printf("PIN succefully verified.\n");
else if (pin != validPin) {
int command;
printf("PIN couldn't be verified.\n");
printf("Press 1 to generate a new pin, 2 to fall back or any other key to exit.\n");
command = getchar();
if (command == '1') {
validPin = rand() % 9999;
printf("Your new pin is: %4d.\n", validPin);
return 0;
} else if (command == '2')
return 0;
else
return 1;
}
return 1;
}
int main(void) {
while(!proc());
printf("Exiting the program.\n");
}
As some online C compilers may incorrectly react to getchar()
, just use scanf()
, like You actually did:
int command;
printf("PIN couldn't be verified.\n");
printf("Press 1 to generate a new pin, 2 to fall back or 0 to exit:\n");
scanf("%d", &command);
if (command == 1) {
validPin = rand() % 9999;
printf("Your new pin is: %4d.\n", validPin);
return 0;
} else if (command == 0)
return 1;
else
return 0;
My final code could have been improved by exploding god-procedure proc()
into pin verifier and command handlers, but the application is simple enough to keep things concise. In your learning curve, this application example might be a nice piece of code to train refactoring skills.
I hope my review helped you.
Upvotes: 1