Binary file Handling - Add,modify and display records

Question

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<fstream>
#include<process>
#include<string>
#include<stdio>
#include<conio>
using namespace std;
int opt;
class housing
{
 int hno,income;
 char name[20],type[20];
 public:
 void assign()
 {
 if(income<15000)
 strcpy(type,"LIG");
 else if(income>=15000)
 strcpy(type,"MIG");
 else if(income>=25000)
 strcpy(type,"HIG");
 }
 void input()
 {
 cout<<"\n Enter House Number: ";
 cin>>hno;
 cout<<"\n House Name: ";
 gets(name);
 cout<<"\n Annual Income: ";
 cin>>income;
 assign();
 }
 void output()
 {
 cout<<"House Number: "<<hno<<"\n"<<"House Name: "<<name<<"\n"<<"Annual Income: "<<income<<"\n"<<"Type: "<<type;
 }
 int retno()
 {
 return hno;
 }
};
void main()
{
 housing h,h1;
 fstream f;
 int hono;
 menu:
 cout<<"\n 1:Add a Record"<<'\t'<<"\n 2:Modify"<<'\t'<<"\n 3:Display All Records"<<'\t'<<"\n 4:Exit"<<endl;
 cin>>opt;
 if(opt==1)
 {
 char ch='y';
 f.open("hous.dat",ios::out|ios::binary|ios::app);
 while(ch=='y')
 { 
 cout<<"\n Enter Details: ";
 h.input();
 f.write((char*)&h,sizeof(h));
 cout<<"\n Want to Enter More? y/n: "<<endl;
 cin>>ch;
 }
 f.close();
 }
 if(opt==2)
 {
 cout<<"\n Enter House No of Record to be modified: ";
 cin>>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;
}

• Pole_Star: What is the use of ate in the modify function

Answer 1

Elephant

Well let us address the Elephant first.

menu:
<STUFF>
goto menu;

Can simply replace by an infinite loop.

while(true)
{
 <STUFF>
}

C Vs C++ String

You are using C-Strings rather than C++ std::string. I would switch to using C++ std::string as it uses is much more natural. As it is an well defined class its usage semantics mirror those of the built in types.

std::string x;
x = "Plop";

So rather than using obscure functions (whose actual meaning I now need to look up).

strcpy(type,"LIG");
// becomes
type = "LIG";

There are lots of other advantages that happen when you start dynamically sizing your strings which happens in any reasonable code base. When you use C you have the start the whole processes of memory management and making sure the string does not leak memory. In C++ this is all handled for you inside the std::string class.

Serialization Vs Binary protocol

Binary protocols (such as you use) are by nature very brittle. You should consider this. Serialization is definitely more expensive and can take more code but it is more flexible and thus robust and is thus usually preferred over a binary format.

Additionally your binary format is fine for very simple objects. But once your object starts to use any members that use resources outside the object this will start to break down.

You make several assumptions here without even realizing it.

1. Your integer is a specific size.
   This is not true. The integer is a platform specific size.
2. The integer has a specific representation in terms of layout.
   This is not true. You need to check the difference between big and little endian systems (or even a few more obscure than that).
3. You spacing between members is consistent. This is not necessarily true. The spacing between the members a compiler thing. It can very by compiler or even on the same compiler it can very just by using different flags for optimization.

Admittedly serialization does have certain drawbacks. Because it is not necessarily a fixed size overwriting a value in a file becomes harder. But you can get around that with a tiny amount of work. Personally I think the extra work is worth it in the long run.

Normally when you write serializers for an object your overload the >> and << operators.

std::ostream& operator<<(std::ostream& st, housing const& data)
{
 st << hno << " " 
 << income << " " 
 << name << '0円' << std::string(19 - strlen(name), ' ') << " " 
 << type << '0円' << std::string(19 - strlen(type), ' ') << "\n"; 
 return st;
}
std::istream& operator>>(std::istream& st, housing& data)
{
 int hno;
 int income;
 char name[20];
 char type[20];
 char sep1;
 char sep2;
 char eormark;
 bool readWorded = (st >> hno >> income).read(&sep, 1)
 .read(name, 20).read(&sep2, 1)
 .read(type, 20).read(&eormark, 1);
 readWorked = readWorked && sep1 == ' ' && sep2 == ' ' && eormark == '\n';
 if (readWorked)
 {
 // All data read and verified. Place in object.
 data.hno = hno;
 data.income = income;
 strcpy(data.name, name);
 strcpy(data.type, type);
 }
 return st;
}

Now you can read and write object like this.

house x1;
std::cin >> x1;
std::cout << x1;

• Loki Astari: @Ajmal RS I think you should wait a couple of days before accepting an answer. Give everybody else a chance to see and contribute.

Answer 2

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 <cstring>. It also uses std::cout which means it should #include <iostream>. 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 <conio> and the getch() function within that, and also #include <process> 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<housing> 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, "LIG");
 else 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.