Some logical error always executes if block?

Question

#include<stdio.h>
#include<string.h>
char Sys_Pass[]="3699";
char verify_Pass(char *P,char *Q)
{
    char ch;
    ch=strcmp(P,Q);
    printf("String Cmp %s,%s is %c %d\n",P,Q,ch,ch);
    if(ch==0)
        return 1;
    else
        return 0;
}
void main()
{
    char Pass[10],New[10];
    char fp=0,sp=0;
    printf("Enter Password : ");
    scanf("%s",Pass);
    if(fp=verify_Pass(Pass,Sys_Pass))
    {
Change:     printf("Enter a New Password : ");
        scanf("%s",Pass);
        printf("Re-Type New Password : ");
        scanf("%s",New);
        if(sp=verify_Pass(Pass,New))
        {
            strcpy(Sys_Pass,New);
            printf("Password Successfully changed\n");
            printf("New Password : %s\n",Sys_Pass);

        }
        else
            printf("Passwords Mismatch\n");
    }
    else
    {
        if(strcmp(Pass,"111999")==0);
            goto Change;
        printf("Wrong Password !!!\n");
    }
    printf("Fp : %c %d\nSp : %c %d\n",fp,fp,sp,sp);
}

This code is always executing the if block and provides the password change even though wrong current password is typed I want to know if there is a logical error if any ?

I am working on a 8051 project which i want to establish a secret password change i.e if i forgot a password and i type password as 111999 it should direct me to the password change menu but here it is always directing to change password menu. This was actually embedded C code but I tried to rectify that using C code but produces same output in both the cases.

It is not wrong (that would mean the program code does not work under all valid circumstances). But it is very bad. As a beginner you should refrain from using `goto` at all. As an expert, you will know when to use it. Note that most coding standards completely disallow `goto`. It is not _necessary_. Learn about loops in C and structured programming. Best is to get a good book to learn. You cannot learn C from tutorial and by asking particular questions. You need to get the whole picture. — too honest for this site, Dec 14 '16 at 11:48

dbush · Accepted Answer · 2016-12-13T14:50:03.790

5

You have a stray semicolon:

         // here ---------------v
    if(strcmp(Pass,"111999")==0);
        goto Change;

As an aside, this is not a good use of goto. The better thing to do is check for either the current password or 111999 in the if condition.

printf("Enter Password : ");
scanf("%s",Pass);
if ((strcmp(Pass,"111999")==0) || (strcmp(Pass,Sys_Pass)==0))
{
    printf("Enter a New Password : ");
    ...
}
else
{
    printf("Wrong Password !!!\n");
}

edited Dec 13 '16 at 14:50

answered Dec 13 '16 at 14:36

dbush

205,898
23
218
273

3

You forgot to mention that there is also a `goto` after the semicolon... ;) – LPs Dec 13 '16 at 14:39
that does not make any difference – Anjaneyulu Dec 13 '16 at 14:39
@Anjaneyulu Running your code with this change, it only enters the password change code if you enter either 3699 or 111999. Are you seeing something different? If so be *specific*. – dbush Dec 13 '16 at 14:47
@Anjaneyulu if you are referring to the `goto`, it is not true. It causes me a rush each time a see it outside Linux Kernel context... ;) – LPs Dec 13 '16 at 14:51

score 0 · Answer 2 · edited May 23 '17 at 12:13

0

There are several things in this code that have margin for improvement, but I will start by asking if you have used a debugger to debug the issue. If you haven't, you can check gdb: video here

I think also you should consider changing your main so it returns an int, except if you have a very good reason not to do it. Then you could have return codes. Same applies for the input arguments to main. If it receives none, I would strongly encourage you to specify it by typingint main(void). See here for more info

Personally I always add braces around every if/else block, even when it is one line. It helps avoid mistakes.

I dont have anything against the goto statement if it is used properly, but I believe in this case it is not. The Clean Code book by Robert Martin contains some good explanations on how to use it. I think you can try to rewrite this and avoid goto usage.

edited May 23 '17 at 12:13

Community

1
1

answered Dec 13 '16 at 14:54

strontivm

59
9

This does not provide an answer to the question. Once you have sufficient [reputation](http://stackoverflow.com/help/whats-reputation) you will be able to [comment on any post](http://stackoverflow.com/help/privileges/comment); instead, [provide answers that don't require clarification from the asker](http://meta.stackexchange.com/questions/214173/why-do-i-need-50-reputation-to-comment-what-can-i-do-instead). - [From Review](/review/low-quality-posts/14583378) – GilZ Dec 13 '16 at 16:51
@GilZ Yes, this does not provide an answer to the question asked. But it shows the OP how she/he can find the answer her/himself by debugging using GDB. It provides resources that show how to do this. Additionally it provides resources on how to improve the code. I quote from here http://stackoverflow.com/help/how-to-answer "Any answer that gets the asker going in the right direction is helpful". Is somehow the answer not doing this? – strontivm Dec 14 '16 at 08:49
If possible provide the answer your opinion – Anjaneyulu Dec 14 '16 at 11:08
@strontivm, most of your answer focused on code style and best practices. Although I agree the OP can improve their code style and avoid some bad practices, this isn't an answer to the question. As the title of the section you've quoted says, "Answer the question". I appreciate your participation, but IMHO, I think you should try to answer well-asked questions (or comment, edit and try to improve less than perfect ones). – GilZ Dec 14 '16 at 15:00
@strontivm I mentioned that this was an embedded C code developed using a Keil IDE so no such options for GDB – Anjaneyulu Dec 19 '16 at 13:27

Some logical error always executes if block?

2 Answers2