HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Opening files using fopen only if user enters correct password

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
openinguserfopenpasswordfilesusingcorrectentersonly

Problem

I wrote this program to just get some practice in C, but I have a question:

  • Do you see anything that I am doing that will create bad habits in my future programming?



```
#include

char password[]="ps"; // set the password that user must match
char input[20]; // user input buffer
char filename[32];
FILE *fp;
int c,i,count=3;

int main()
{

printf("Please enter the password:"); //ask for password
fgets(input,20,stdin); //store user entry into input buffer
sscanf(input,"%s",input); //format

if(strcmp(password,input)==0) //if the input is equal to the password then open the file and print contents
{
if((fp=fopen("text","r")) != NULL)
{
while((c=getc(fp)) != EOF)
{
putchar(c);
}
fclose(fp); //close the file once done
}
else
{
perror("Error:");
return(-1);
}
}
else if(strcmp(password,input) != 0) //if the input is wrong then give the user 3 more tries
{
for(i=0;i<3;i++)
{
printf("\nThe password you entered is incorrect!");
printf("\n%d tries left, password:",count);
fgets(input,20,stdin);
sscanf(input,"%s",input);

--count;

if(strcmp(password,input)==0) //if out of those 3 tries entry is correct--open file/print contents
{
if((fp=fopen("text","r")) != NULL)
{
while((c=getc(fp)) !=EOF)
{
putchar(c);

Solution

Don't repeat yourself. This block of code appears twice. It would be better to extract it to a separate function and reuse it:

if((fp=fopen("text","r")) != NULL)
        {
                while((c=getc(fp)) != EOF)
                {
                        putchar(c);
                }
                fclose(fp);                     //close the file once done
        }
        else
        {
                perror("Error:");
                return(-1);
        }


Btw, is there a reason to read and print the file contents character by character? It would be more efficient to do it buffer by buffer.

Also, the comment is pointless, that line is self-explanatory.

In this code:

if(strcmp(password,input)==0)
{
        // ...
}
else if(strcmp(password,input) != 0)


The else if condition can be replaced with a simple else. That's less typing, and again, don't repeat yourself!

Code Snippets

if((fp=fopen("text","r")) != NULL)
        {
                while((c=getc(fp)) != EOF)
                {
                        putchar(c);
                }
                fclose(fp);                     //close the file once done
        }
        else
        {
                perror("Error:");
                return(-1);
        }
if(strcmp(password,input)==0)
{
        // ...
}
else if(strcmp(password,input) != 0)

Context

StackExchange Code Review Q#71047, answer score: 7

Revisions (0)

No revisions yet.