Program I made!

it is still the first version, for those who want to help me learn c++, u can check it out and maby give me some tips or hints and Ideas for an upgrade

tnx!

this program is sort of an database, which stores records ( classes ) on a file and , reads writes or deletes them

( i wanted too upload a rar pack containing it all but thats not allowed )

-> in the future i want it too be some sort of client, where u can login using your password etc, and if you are an "admin" u have MORE powers then an normal member etc

You use goto a lot. This produces very hard to read code, and is a bad habit.

Goto = Bad, haha

In C++ we try to make Object-Oriented Programs, goto is used in linear programs.

If you wanna try something new try writing the same program without using goto

Have fun!

LOL? i used goto 2 times as in GOTO MAINDRIVER and GOTO skip.

whats so bad about goto? lol why should i make the same function calls over and over on the end of an task u picked if i can just use one goto to the beginning of the program where the function is just called once?.

if i replaced all Goto MAINDRIVER ( which are 4 calles ) with this :

PrintMenu ();
choise = IntegerInputHandler ( lengths,index );


, it will just make my application size bigger for no need. as for goto skip, i do not know how i could deal with that in an other way:

Code:

void ReadSingleRecord ( unsigned int id, int times )
{
  unsigned int LOCATION = 0;
  member empty (" "," "," ", 0 , 0 );
  std::ifstream ReadOut ("C:\\Datacab.dat", ios::in | ios::binary );
  if (!ReadOut)
  {
    CheckFile ();
  }
  else
  {
    ReadOut.seekg ( 0 , ios::beg );
    while (!ReadOut.eof() )
    {
      ReadOut.read ( ( reinterpret_cast < char * > (&empty) ),sizeof (member) );
      LOCATION++;
      if (empty.getaccnum ()  == id )
      {
        LOCATION--;
        break;
      }
    }
    ReadOut.seekg ( ( LOCATION* sizeof (member) ), ios::beg );
    if ( empty.getaccnum ()  == 0 )
    {
      std::cout <<"Record does not exist \n\n" << std::endl;
      goto skip;
    }
    for (int x = 0; x < times; x++)
    {
      ReadOut.read ( ( reinterpret_cast < char * > (&empty) ), sizeof (member) );

      std::cout <<"Record Number " << LOCATION << std::endl;
      std::cout <<"_______________\n"
                <<"Username: " << empty.getusername () <<"\n"
                <<"Email: " << empty.getemail () <<"\n"
                <<"Password: " << empty.getpassword () <<"\n"
                <<"Age: " << empty.getage () <<"\n"
                <<"Accountnumber: " << empty.getaccnum () <<"\n\n"<< std::endl;
    }
    skip:

  }
}

ok lol i could implement that if ( empty.getaccnum () == 0 ) into my loop, but that just waisting proccessing time.
(in my eyes)

I wrote different functions for my game depending on if people wanted to play again, then had the function call itself. It's just something different from goto.

Code:

#include <allegro.h>

void EasyMode(){
    while(stuff){
        //stuff, whatever can go here

        textout(screen, font, "play again?", 0, 0, makecol(255,0,0),makecol(0,0,0));
        if(key[KEY_Y]) EasyMode();
        if(key[KEY_N]) break;
    }
}

Originally Posted by epidemic

, it will just make my application size bigger for no need. as for goto skip, i do not know how i could deal with that in an other way:

Code:

 // snip
    if ( empty.getaccnum ()  == 0 )
    {
      std::cout <<"Record does not exist \n\n" << std::endl;
      goto skip;
    }
    for (int x = 0; x < times; x++)
    {
      ReadOut.read ( ( reinterpret_cast < char * > (&empty) ), sizeof (member) );

      std::cout <<"Record Number " << LOCATION << std::endl;
      std::cout <<"_______________\n"
                <<"Username: " << empty.getusername () <<"\n"
                <<"Email: " << empty.getemail () <<"\n"
                <<"Password: " << empty.getpassword () <<"\n"
                <<"Age: " << empty.getage () <<"\n"
                <<"Accountnumber: " << empty.getaccnum () <<"\n\n"<< std::endl;
    }
    skip:

  }

ok lol i could implement that if ( empty.getaccnum () == 0 ) into my loop, but that just waisting proccessing time.
(in my eyes)

Like this?

Code:

    if ( empty.getaccnum ()  == 0 )
    {
      std::cout <<"Record does not exist \n\n" << std::endl;
    }
    else
    {
        for (int x = 0; x < times; x++)
        {
          ReadOut.read ( ( reinterpret_cast < char * > (&empty) ), sizeof (member) );

          std::cout <<"Record Number " << LOCATION << std::endl;
          std::cout <<"_______________\n"
                    <<"Username: " << empty.getusername () <<"\n"
                    <<"Email: " << empty.getemail () <<"\n"
                    <<"Password: " << empty.getpassword () <<"\n"
                    <<"Age: " << empty.getage () <<"\n"
                    <<"Accountnumber: " << empty.getaccnum () <<"\n\n"<< std::endl;
        }
    }

A lot easier to follow if you ask me.

You seem to have the wrong attitude epidemic. It's not about the size of the program, it's about the program's readability and flow. Think about it. Keep in mind, others have to read it.

Originally Posted by Sentral

You seem to have the wrong attitude epidemic. It's not about the size of the program, it's about the program's readability and flow. Think about it. Keep in mind, others have to read it.

It is readable. very readable if u ask me. once u know that goto MAINDRIVER just asks for input again for the main menu... whats so hard to read it? it just "restarts" the program..

oysterman, k lol ty

I don't get the ReadSingleRecord function. First you search for a record that has a matching id. If you find it then - instead of printing it right away and returning - you go through some sphagetti to print a whole bunch of records?

The skip is not needed, because you could have done whatever you are trying to skip later, already (if (empty.getaccnum () == id) -> do it here).

If the indentations go too deep, break it up into smaller functions.

(If you say it is very readable, why do you want comments on it? The only function I have seen is not readable at all: starting from the name of the function which seems off.)

Edit: Took a look at the rest. Not too bad, except for a few magic numbers. In main you probably don't seem to need the goto either. Wouldn't you get to the exact same place (top of while) if you just breaked each case of the switch like people are used to?

Originally Posted by scwizzo

I wrote different functions for my game depending on if people wanted to play again, then had the function call itself. It's just something different from goto.

Code:

#include <allegro.h>

void EasyMode(){
    while(stuff){
        //stuff, whatever can go here

        textout(screen, font, "play again?", 0, 0, makecol(255,0,0),makecol(0,0,0));
        if(key[KEY_Y]) EasyMode();
        if(key[KEY_N]) break;
    }
}

This is not a really good idea. You do realize that each time you go deeper into recursion, the stack space is getting smaller and smaller.

Code:

void EasyMode(){
    while(true){
        PlayOneGame();

        textout(screen, font, "play again?", 0, 0, makecol(255,0,0),makecol(0,0,0));
        if(!key[KEY_Y]) break;
    }
}

Originally Posted by anon

I don't get the ReadSingleRecord function. First you search for a record that has a matching id. If you find it then - instead of printing it right away and returning - you go through some sphagetti to print a whole bunch of records?

The skip is not needed, because you could have done whatever you are trying to skip later, already (if (empty.getaccnum () == id) -> do it here).

If the indentations go too deep, break it up into smaller functions.

(If you say it is very readable, why do you want comments on it? The only function I have seen is not readable at all: starting from the name of the function which seems off.)

u can also read out blocks of 10 records ( times ) with the function, u give in the first record number, it finds the record, and then reads 10 records, because I wanted it to be easy to update, I gave a variable int times too the function so in the future, u can choose howmany records u can read out at once.


ill change the function's name...

Code:

void ReadRecord ( unsigned int id, int times ) 
{
  unsigned int LOCATION = 0;  // (location in file)/ sizeof(member)
  member empty (" "," "," ", 0 , 0 ); // blank member
  std::ifstream ReadOut ("C:\\Datacab.dat", ios::in | ios::binary );
  if (!ReadOut)
  {
    CheckFile ();
  }
  else
  {
    ReadOut.seekg ( 0 , ios::beg ); // position file pointer into the begining
    while (!ReadOut.eof() ) // while end of file is not reached
    {
      ReadOut.read ( ( reinterpret_cast < char * > (&empty) ),sizeof (member) );
      LOCATION++; // add one too location ( 1 * sizeof (member) )
      if (empty.getaccnum ()  == id )
      {
        LOCATION--;
        break;
      }
    }
    ReadOut.seekg ( ( LOCATION* sizeof (member) ), ios::beg ); // find actual location 
    for (int x = 0; x < times; x++)
    {
      if ( empty.getaccnum ()  == 0 ) // if accnum equals 0, record does not exist
      {
        std::cout <<"Record Number " << LOCATION << " does not exist" << std::endl;
      }

      ReadOut.read ( ( reinterpret_cast < char * > (&empty) ), sizeof (member) );
  
      std::cout <<"Record Number " << LOCATION << std::endl;
      std::cout <<"_______________\n"
                <<"Username: " << empty.getusername () <<"\n"
                <<"Email: " << empty.getemail () <<"\n"
                <<"Password: " << empty.getpassword () <<"\n"
                <<"Age: " << empty.getage () <<"\n"
                <<"Accountnumber: " << empty.getaccnum () <<"\n\n"<< std::endl;
    }
    LOCATION += 1;  // location is not the same annymore (LOOP)
  }
}

ReadRecord still implies one record.

I think the user of this function would be a lot happier, if the function just returned a member object with the matching ID. This way they can decide what to do with it: how they want to output it, may-be they want to edit it and write it back to the file etc.

So I suggest something like that

Code:

member GetRecordById ( unsigned int id)
{
  member temp; //provide a default constructor

  //ios::in is probably redundant for ifstream
  std::ifstream ReadOut ("C:\\Datacab.dat", ios::in | ios::binary );
  if (!ReadOut)
  {
    CheckFile ();
  }
  else
  {
    //it is not wise to control file input with eof()!
    while (ReadOut.read( reinterpret_cast<char*>(&temp), sizeof(member))
    {
      if (temp.getaccnum ()  == id )
      {
        return temp;
      }
    }
  }
  return member(); //return default constructed empty record
}

By the way, your code doesn't compile. You are trying to pass something like (var +1 ) to a function that expects int&. The reference means that the function may modify the original value, but alas - (var + 1) is a constant value. If you didn't mean to change the referenced value, don't pass it as a reference: references are more expensive for built-in data-types than copies.

It compiles with my compiler...
but tnx for your post... gave me some good ideas

Looking back, i am ashamed i posted this messy program ... it did work, and it did do what i wanted, but it still needs a lot of work. ( deleted added files )

Don't be ashamed epidemic. Everyone starts somewhere and if you are just learning, well you were given some advice on how to improve. It is kind of like learning to draw. The first few are not going to be perfect, but you get to review and refine your methods. If you learn a way to improve your program, you can do it again taking advantage of the new methods.