Solved

Urgent!  What's wrong in this code?

Posted on 2002-07-01
20
232 Views
Last Modified: 2010-04-01
I want to read user id and password from text file and stored in an array of object.

Account accountList[100];

e.g.
class Account{
userid
password
}

text file data
123
pass1
456
pass2

then, i need to loop through the object in array to compare user id and password key in by user.

this is how user key in id and password and compare with object in array.

int Bank::userLogin(void){
  char *tUserID=new char[10];
  char *tPassword=new char[10];
  char *tAccID=new char[10];
  char *tAccPassword=new char[10]
  int success=1;  // if = 1 = invalid
  Account aAccount;

  clrscr();
  cout<<"Login"<<endl;
  cout<<"====="<<endl;
  cout<<"Please key in your ID and Password"<<endl;
  tUserID=getUserInput("User ID : ");
  tPassword=getUserInput("Password : ");

  if( (strcmp(tUserID,"ADMIN")==0) && (strcmp(tPassword,"PASSWORD") ==0))
    success=0;
  else
  for(int loop=0;loop<getNoOfAcc();loop++){
    Account aAccount=getAccount(loop);
    strcpy(aAccount.getUserID(),tAccID);
    strcpy(aAccount.getPassword(),tAccPassword);
    if( (strcmp(tUserID,tAccID)==0) && (strcmp(tPassword,tAccPassword) ==0)){
      success=0;
      loop=getNoOfAcc();//exit
    };
    getch();
  };

  return success;
}


This is how i read from file into array.

void Bank::readAccountInfor(){
  FILE *fp;
  Account aAccount;
  char *temp=new char[256];
  if((fp=fopen("c:\\bank\\Account.txt","r+"))==NULL){
    clrscr();
    displayMessage("Error reading account.txt file !");
    exit(1);
  }

  fgets(temp,256,fp);
  while(!feof(fp)){
    Account tAccount;
    aAccount.setUserType(temp);

    temp=tAccount.readFile(fp);
    aAccount.setUserID(temp);

    temp=tAccount.readFile(fp);
    aAccount.setPassword(temp);

    if(strlen(aAccount.getUserType())>=7){
      cout<<"Add "<<aAccount.getUserID();getch();
      addAccount(aAccount,getNoOfAcc());
      setNoOfAcc(getNoOfAcc()+1);
    };
    fgets(temp,256,fp);

  };//while
};

Problem, cannot compare record.. always invalid user id or password.

please help me to solve this problem..provide me with complete coding.
If can, provide me coding for every function including read from text file and write to text file using fputs, fgets. Thanks.

I will give points to most complete answer.
0
Comment
Question by:yongyih
  • 13
  • 7
20 Comments
 
LVL 30

Expert Comment

by:Axter
ID: 7122227
char *tUserID=new char[10];
 char *tPassword=new char[10];
 char *tAccID=new char[10];
 char *tAccPassword=new char[10]

The above method for initializing these variables gives you all the disadvantage and non of the advantages of using pointers.

Since you're initializing the pointers with a FIXED compile time size, you should then initialize them in the following manner:
 char tUserID[10];
 char tPassword[10];
 char tAccID;
 char tAccPassword;

Using this method, you don't have to worry about deleteing the variables later.

I notice that you have not deleted any of these pointers, which will lead to memory leaks.


You should always avoid using NEW when possible.

You also have this line
tUserID=getUserInput("User ID : ");

That means you're basically loosing your previous pointer, and therefore, you can't even deleted the buffer you previously created.

continue............
0
 
LVL 30

Expert Comment

by:Axter
ID: 7122245
There's a lot in this code that is wrong.

What is the return type for getUserInput?

If the return type is a pointer to a string, that is garanteed to be valid outside of the function, then your pointers should be initialized to NULL.
Example:
char *tUserID=NULL;
char *tPassword=NULL;
char *tAccID=NULL;
char *tAccPassword=NULL;

And infact, a better method, is not to declare the pointers untill you need them.  Thereby, declaring them and initializeing them all in one.

Example:

char *tUserID=getUserInput("User ID : ");

You should avoid declaring all your variables in the top of the function, as C programmers do.
This style is needed in C, but it's not needed in C++, and is discourage by top experts.

continue......
0
 
LVL 30

Expert Comment

by:Axter
ID: 7122266
strcpy(aAccount.getUserID(),tAccID);

In the above line, you attemp to copy tAccID before it is initialized with any meaning full data.
Furthermore, you're copying it to whatever aAccount.getUserID() returns.
I think what you really meant to do is copy the return of aAccount.getUserID() to tAccID.
The first argument in strcpy is the destination, and the second argument is the source.
So maybe this is really what you want:
strcpy(tAccID,aAccount.getUserID());

But that means you also want to make sure that tAccID is initialized with enough space.
char tAccID[32];
strcpy(tAccID,aAccount.getUserID());

continue.......
0
 
LVL 30

Expert Comment

by:Axter
ID: 7122281
I suggest you post the code for getUserInput, or it's declaration, so we can see the complete picture.

If getUserInput returns a local variable, this will cause problems.
0
 
LVL 30

Expert Comment

by:Axter
ID: 7122296
yongyih,
Is this homework?

If so, I'm glad that you at least making an effort to do the work.
But I don't think you should rely on other experts giving you complete code.
You will learn more if you let the experts help you work through your current code, so you know what exactly is wrong with it, and how to prevent making the same mistakes in the future.
0
 
LVL 2

Author Comment

by:yongyih
ID: 7123469
getUserInput method
char* Bank::getUserInput(char *message){
  char *userInput = new char[256];
  cout<<message;
  cin.getline(userInput,255);//get user input, max=256
  return userInput;//return what user key in
};

actually i help my friend to ask this question and she did most of the coding already. The problem is, don't know where is the problem come from.

when get user input, i think she have to declare char*userInput=new char[256];
otherwise, will have some error.

if declare char name[10];, do we need to always assign '\0' to the end of string?

How about u tell me how to declare a new instance of object inside a loop?

E.g.
Bank bank1;
assign data to bank1
bankList[0]=bank1;
assign new data to bank1
bankList[1]=bank1;

do u think the first object data will change??

is there any problem with the getUserInput method?

0
 
LVL 2

Author Comment

by:yongyih
ID: 7123481
at first, i didn't declare string in that way.  but because always has error, then i try to change the code until no error...

You said:
"You should avoid declaring all your variables in the top of the function, as C programmers do.
This style is needed in C, but it's not needed in C++, and is discourage by top experts."

I thought all programmers will declare all variables at top because easier to maintain and debug.  If your variables declare everyway in coding, i think you will have big problem when program has error.

Anyway, please tell me the most important part to solve my problem.  Like correct way to declare string so that no problem when get input from user, read from file, write to file, pass between method..
because if didn't declare properly, the string will contain rubbish data at the end of string..

Thanks for reply.
0
 
LVL 2

Author Comment

by:yongyih
ID: 7123483
besides, we cannot return a string if we didn't declare it as pointer, right?

i try to return char[] but cannot. how about return char[10]?

this will has error or not?
char [] getName(void){
..
}

0
 
LVL 30

Expert Comment

by:Axter
ID: 7123553
>>I thought all programmers will declare all variables at
>>top because easier to maintain and debug.
Actually, it's easier to maintain when you declare when you need it.
When you declare all the variables on the top, it's easy to end up with extra variables that don't get used.  Or it's easy to start using a variable for one intension, and then further down the function, you start using it for something totally different that could confict with your original purpose.

When you declare the variable when you need it, if you decide to remove the code that uses the variable, it's easier to detect, that you no longer need the variable.

For example, if you have the following in the middle of your code:
char *tPassword=getUserInput("Password : ");

And then you decide you no longer need getUserInput function, when your remove the line of code using getUserInput, it's easy to dectect that you no longer need
tPassword variable.

Whether or not putting all variables in the top is easier to maintain, can be argued, and can be considered a personal view point.
However, putting all arguements on the top is diffinitly less efficient, and there's no argueing that, because it's a fact, and not a view point.

It's less efficient because of two reasons.  Double initialization and initialization of un-used variables.

For example:
void Myfunction()
{
std::string MyString;
//....more code here
MyString = "Hello World";
}

In the above code, MyString is first initialized and given the value of an empty string "".
Then when the code finally uses MyString, it gets assigned a value of "Hello World".
The code has to do double work, when you could complete dispense with the trouble of assigning an empty string, by waiting to you need the variable, and then assigning the value.

Putting all variables in the top also is less efficient because you end up initializeing variables, that you never get to use.
Example:
void Myfunction()
{
int x, y, z;

x = GetSomeValue();
if (x == 0) return 1;
//.... more code
 y = x*2;
 z = y + x;
 return z;
}

In the above function, when x equals zero, y and z are initialized even though they don't get a chance to be used.
0
 
LVL 30

Expert Comment

by:Axter
ID: 7123557
>>when get user input, i think she have to declare
>>char*userInput=new char[256];
>>otherwise, will have some error.
For this function, you do need to use new char[] in order to return a buffer.

>>How about u tell me how to declare a new instance of object inside a loop?

I don't understand the question.
0
Is Your Active Directory as Secure as You Think?

More than 75% of all records are compromised because of the loss or theft of a privileged credential. Experts have been exploring Active Directory infrastructure to identify key threats and establish best practices for keeping data safe. Attend this month’s webinar to learn more.

 
LVL 2

Author Comment

by:yongyih
ID: 7123559
This is my another question that ask about how to read/write from text file.  I posted my code in that question.  My problem is the program will read extra string file text file.. tell me a better way to check eof.

http://www.experts-exchange.com/jsp/qManageQuestion.jsp;jsessionid=0052619FEE4D5CB3089D235487FF2045?qid=20316765

For this question, please tell me how to add an object to a loop and read it back..
i think is just assign directly, right?

I can't change the code.  I have to use char *name=new char[256]; because without this, my program will have a lot of problems.  Whole program doesn't work at all.  Anyway, i will set it to NULL.

0
 
LVL 30

Expert Comment

by:Axter
ID: 7123563
>>is there any problem with the getUserInput method?
The method can work as long as you remmeber to delete the returned buffer, after the calling function is done with it.


>>besides, we cannot return a string if we didn't declare
>>it as pointer, right?

If you use std::string, you don't have to declare the code as a pointer, and you don't have to use new char[].

I'll post an example shortly.
0
 
LVL 30

Expert Comment

by:Axter
ID: 7123569
Can you post your Bank class code, and your Account class code?
0
 
LVL 2

Author Comment

by:yongyih
ID: 7123571
for this question, help me to solve this problem.
thanks.

int Bank::userLogin(void){
  char *tUserID=new char[256];
  char *tPassword=new char[256];
  char *tAccID=new char[256];
  char *tAccPassword=new char[256];

  int success=1;  // if = 1 = invalid
  Account aAccount;
  clrscr();
  cout<<"Login"<<endl;
  cout<<"====="<<endl;
  cout<<"Please key in your ID and Password"<<endl;

  tUserID=getUserInput("User ID : ");
  tPassword=getUserInput("Password : ");

  if( (strcmp(tUserID,"ADMIN")==0) && (strcmp(tPassword,"PASSWORD") ==0))
    success=0;
  else
  for(int loop=0;loop<getNoOfAcc();loop++){
    aAccount=getAccount(loop);
    strcpy(tAccID,aAccount.getUserID());
    strcpy(tAccPassword,aAccount.getPassword());
    cout<<"ID read from array "<<tAccID<<endl;getch();
    if( (strcmp(tUserID,tAccID)==0) && (strcmp(tPassword,tAccPassword) ==0)){
      success=0;
      loop=getNoOfAcc();//exit
    };
  };

  return success;
}
0
 
LVL 30

Expert Comment

by:Axter
ID: 7123573
>>for this question, help me to solve this problem.
>>thanks.
It's harder to solve that specific problem without know the class declaration for Account
0
 
LVL 30

Expert Comment

by:Axter
ID: 7123596
Here's an example code using std::string instead of pointers.
int Bank::userLogin(void)
{
     int success=1;  // if = 1 = invalid
     
     clrscr();
     cout<<"Login"<<endl;
     cout<<"====="<<endl;
     cout<<"Please key in your ID and Password"<<endl;
     std::string tUserID = getUserInput("User ID : ");
     std::string tPassword=getUserInput("Password : ");
     
     if( tUserID == "ADMIN" && tPassword == "PASSWORD")
     {
          success=0;
     }
     else
     {
          for(int loop=0;loop< getNoOfAcc();++loop)
          {
               Account aAccount = getAccount(loop);
               std::string tAccID = aAccount.getUserID();
               std::string tAccPassword = aAccount.getPassword();
               if(tUserID == tAccID && tPassword == tAccPassword)
               {
                    success=0;
                    break; //exit
               }
               getch();
          };
     }
     
     return success;
}
0
 
LVL 30

Accepted Solution

by:
Axter earned 300 total points
ID: 7123600
Since I don't know what your Account class looks like, here what I used.

class Account
{
public:
     Account(const std::string &Src)
     {
          Id = Src;
     }
     std::string getUserID(void){return "foo";};
     std::string getPassword(void){return "foofoo";};
     std::string Id;
};

class Bank
{
     int userLogin(void);
     void readAccountInfor();
     std::string getUserInput(const std::string &message);
     Account getAccount(int Index)
     {
          return MyAccounts[Index];
     }
     int getNoOfAcc() {return 32;}
     Account MyAccounts[32];
};



std::string Bank::getUserInput(const std::string &message)
{
     cout << message;    
     std::string userInput;
     getline(cin, userInput);
     return userInput;//return what user key in
};

You can see that getUserInput function does not use a pointer, and doesn't use new char[].

Using std::string is much easier and safer.  Many of the problems this code had could be easily avoided if std::string was used instead of char* = new char[] is used.

0
 
LVL 2

Author Comment

by:yongyih
ID: 7124265
I already solved my problem myself.  anyway, thanks for your help.

i also will read through your code so that can learn something from your code.  

0
 
LVL 30

Expert Comment

by:Axter
ID: 7124938
If you have any questions feel free to ask.
0
 
LVL 2

Author Comment

by:yongyih
ID: 7125706
I don't know why my program ok last night, now it always read extra record which is rubbish data.  I think i have to rewrite my code for read and write text file.

Therefore, i accept your answer first.  If can, tell me what's wrong with this code. thanks.

MY PROBLEM IS:  program always read extra record.  if file has 3 lines (1 record), it will read six times (which is 2 record with 1 empty record)

Please help me to solve this as soon as possible. thanks.

Account.txt contain.

ACCOUNT TYPE
USER ID
USER PASSWORD


class Bank{
private:
  char *userType;
  char *userID;
  char *userPassword;
public:
  char* readFile(FILE *fp);
  void writeFile(FILE *fp);
  //getters and setters here...
}


in bank class
char* Bank::readFile(FILE *fp){
  char *tData=new char[256];
  fgets(tData,256,fp);
  tData[strlen(tData)-1]='\0';
  return tData;
};

void Bank::writeFile(FILE *fp){
  fputs(getUserType(),fp);
  fputs("\n",fp);
  fputs(getUserID(),fp);
  fputs("\n",fp);
  fputs(getPassword(),fp);
  fputs("\n",fp);
};

void BankAccount::readAccountInfor(){
  FILE *fp;
  Account aAccount;
  char *temp=new char[256];

  if((fp=fopen("c:\\Bank\\Account.txt","r+"))==NULL){
    clrscr();
    displayMessage("Error reading account.txt file !");
    exit(1);
  }

  while(!feof(fp)){
    Account tAccount;

    temp=tAccount.readFile(fp);
    aAccount.setUserType(temp);

    temp=tAccount.readFile(fp);
    aAccount.setUserID(temp);

    temp=tAccount.readFile(fp);
    aAccount.setPassword(temp);

    if(strlen(aAccount.getUserType())>=7){
      addAccount(aAccount,getNoOfAcc());
      setNoOfAcc(getNoOfAcc()+1);
    };
  };//while
};

void BankAccount::writeAccountInfor(){
  FILE *fp;
  Account aAccount;

  if((fp=fopen("c:\\alice\\Account.txt","w+t"))==NULL){
    clrscr();
    displayMessage("Error reading account.txt file !");
    exit(1);
  }
  for(int loop=0;loop<getNoOfAcc();loop++){
    aAccount=getAccount(loop);
    aAccount.writeFile(fp);
  };
};
0

Featured Post

Is Your Active Directory as Secure as You Think?

More than 75% of all records are compromised because of the loss or theft of a privileged credential. Experts have been exploring Active Directory infrastructure to identify key threats and establish best practices for keeping data safe. Attend this month’s webinar to learn more.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Written by John Humphreys C++ Threading and the POSIX Library This article will cover the basic information that you need to know in order to make use of the POSIX threading library available for C and C++ on UNIX and most Linux systems.   [s…
Container Orchestration platforms empower organizations to scale their apps at an exceptional rate. This is the reason numerous innovation-driven companies are moving apps to an appropriated datacenter wide platform that empowers them to scale at a …
The viewer will learn how to pass data into a function in C++. This is one step further in using functions. Instead of only printing text onto the console, the function will be able to perform calculations with argumentents given by the user.
The viewer will learn how to user default arguments when defining functions. This method of defining functions will be contrasted with the non-default-argument of defining functions.

920 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

16 Experts available now in Live!

Get 1:1 Help Now