Binary file Handling - Add,modify and display records

I have written a basic binary file handling program and have used goto, so I want to know how to improve this program by replacing goto with another function

#include
#include
#include
#include
#include

using namespace std;

int opt;
class housing
{
  int hno,income;
  char name[20],type[20];
  public:
  void assign()
  {
   if(income=15000)
   strcpy(type,"MIG");
   else if(income>=25000)
   strcpy(type,"HIG");
  }
  void input()
  {
   cout>hno;
   cout>income;
   assign();
  }
   void output()
  {
   cout>opt;
 if(opt==1)
 {
  char ch='y';
  f.open("hous.dat",ios::out|ios::binary|ios::app);
  while(ch=='y')
  { 
   cout>ch;
  }
 f.close();
 }
 if(opt==2)
 {
  cout>hono;
  f.open("hous.dat",ios::in|ios::out|ios::binary|ios::ate);
  f.seekg(0);
  while(f.read((char*)&h,sizeof(h)))
  {
   if(h.retno()==hono)
   {
    cout<<"\n New Value: ";
    h1.input();
    f.seekp(-sizeof(h),ios::cur);
    f.write((char*)&h1,sizeof(h1));
   }
  }
 f.close();
 }
 if(opt==3)
 {
  f.open("hous.dat",ios::in|ios::binary);
  f.seekg(0);
  while(f.read((char*)&h,sizeof(h)))
  h.output();
  f.close();
 }
 if(opt==4)
 exit(0);
 cout<<"\nPress ... ";
 getch();
 goto menu;

}

Here are some things that may help you improve your code.

Use the required #includes

The code uses strcpy which means that it should #include . It also uses std::cout which means it should #include . Always use the required include files per the standards specification and not just the ones that seem sufficient to make it compile on your machine.

Isolate platform-specific code

In this code, there are several things that are DOS/Windows only including #include and the getch() function within that, and also #include which I can only guess is a C++ version of the standard process.h. Your code runs successfully on Linux if I supply those missing functions, but it would be nice if there were an #ifdef WINDOWS already in the code so that one could recompile without having to alter the source code.

Don't use gets

Using gets is not good practice because it can lead to buffer overruns. It has been removed from the C11 standard and marked "obsolete" in POSIX 2008. Similarly, it should not be used in C++ program. Instead use fgets or better, declare name to be a std::string and use std::cin >> name;

Follow standard declaration of main

Your compiler may allow you to declare void main() but that construction is technically only valid for what are called "non-hosted" environments, e.g. one in which you are not running Windows. For that reason, you should use the standard int main() instead.

Whitespace improves readability

Allofyourwordsarecrammedtogether. Inserting some spaces in your lines will make them much easier to read. For example, instead of this:

while(f.read((char*)&h,sizeof(h)))

Use this:

while (f.read((char *)&h, sizeof(h)))

Fix your formatting

The formatting of this code is absolutely diabolical. The code executed within above-mentioned while loop, for example, is at the same indent level as the while itself. Choose a standard format and use it consistently. Being consistent helps others read and understand your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. For this program, I'd advocate removing it and simply using the std:: prefix where needed.

Make better use of C++ objects

The code is already using a housing class, but doesn't make very good use of that class. For instance, all of the actual binary input and output is done inside main while it could instead be relegated to the housing class. Also, the file is simply a collection of housing objects. Why not represent that in a std::vector or similar?

Create a function rather than repeating code

In several places in the code, the file is opened, read, a housing record read and then the file is closed again. It would make much more sense and make the code easier to read, understand and maintain, if that functionality were instead encapsulated in a function; probably most appropriately a member function of a house collection object.

Use a switch instead of multiple if or if ...else chains

The logic is much easier to see if a switch statement is used instead of a long if...else chain. The default case can then be used solely for error cases.

Use longer, more meaningful names

Names like h and f are not very descriptive. Instead, you might use house and datafile.

Avoid the use of global variables

I see that opt is declared as global variables rather than as local variables. It's generally better to explicitly pass variables your function will need rather than using the vague implicit linkage of a global variable. In this case, there is really no reason that opt couldn't be local to main.

Eliminate the goto

Instead of the goto, use while (!done) and set done to true when the user indicates a desire to exit the program.

Fix the bug

The assign() member function is written like this:

void assign {
    if (income = 15000)
        strcpy(type, "MIG");
    else if (income >= 25000)
        strcpy(type, "HIG");
}

However, I think you'll find that HIG will never be assigned no matter how high the reported income. If the user enters a value of 999999, it will be assigned "MIG".

Eliminate "magic numbers"

In a number of cases, the code uses "magic numbers" such as 20 and 15000 that have no obvious meaning. These would be better as named constants. There is a similar argument to be made about the file name "hous.dat" -- this should be made a named constant instead of repeating it three times within the code.

StackExchange Code Review Q#139540, answer score: 6