Solved

Urgent!  What's wrong in this code?

Posted on 2002-07-01
20
231 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
Comment Utility
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
Comment Utility
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
Comment Utility
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
Comment Utility
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
Comment Utility
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
Comment Utility
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
Comment Utility
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
Comment Utility
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
Comment Utility
>>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
Comment Utility
>>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
IT, Stop Being Called Into Every Meeting

Highfive is so simple that setting up every meeting room takes just minutes and every employee will be able to start or join a call from any room with ease. Never be called into a meeting just to get it started again. This is how video conferencing should work!

 
LVL 2

Author Comment

by:yongyih
Comment Utility
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
Comment Utility
>>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
Comment Utility
Can you post your Bank class code, and your Account class code?
0
 
LVL 2

Author Comment

by:yongyih
Comment Utility
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
Comment Utility
>>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
Comment Utility
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
Comment Utility
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
Comment Utility
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
Comment Utility
If you have any questions feel free to ask.
0
 
LVL 2

Author Comment

by:yongyih
Comment Utility
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

Enabling OSINT in Activity Based Intelligence

Activity based intelligence (ABI) requires access to all available sources of data. Recorded Future allows analysts to observe structured data on the open, deep, and dark web.

Join & Write a Comment

This article will show you some of the more useful Standard Template Library (STL) algorithms through the use of working examples.  You will learn about how these algorithms fit into the STL architecture, how they work with STL containers, and why t…
IntroductionThis article is the second in a three part article series on the Visual Studio 2008 Debugger.  It provides tips in setting and using breakpoints. If not familiar with this debugger, you can find a basic introduction in the EE article loc…
The goal of the tutorial is to teach the user how to use functions in C++. The video will cover how to define functions, how to call functions and how to create functions prototypes. Microsoft Visual C++ 2010 Express will be used as a text editor an…
The viewer will learn how to clear a vector as well as how to detect empty vectors in C++.

762 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

9 Experts available now in Live!

Get 1:1 Help Now