Code Questions

I have 2 different versions of code for a simple program. One version works and with minor changes after comparing line for line the two different version only have about 4 different lines. I have many questions about this as far as my style and optimization memory usage etc... But Im not sure if its cool to post the entire program (its about 100 lines maybe minus space for readability). My main question is one of them the bad one exits with a memory error I think and I cant figure out why these few changes make the program run fine (minus warnings which I could never figure out anyway. I dont need any help with the actual program as its for a class and its already completed. The questions are more for me than anything else.

take a chance and post your code - the bad one.. I'm sure someone around here would be able to help you out. Just make sure you list all the problems your having.

This is the bad version. Im not sure (or else I wouldnt be asking) but I think it has something to do with scope or the way I am passing the structures. I messed up last night and accidentally started correcting this one so it doesnt exit with the exact same error but its close enough to baffle and concern me. Feel free to comment on any portion of it..its not an error its a learning opportunity.

Code:

#include <stdio.h> #include <stdlib.h> #include <string.h> //Structure Definitions struct Employee{ char FName[20]; char LName[10]; int HrsWorked; float PayRate; int StpFlag; float RPay; float OPay; float GPay; float FITAmount; float NPay; }; struct Totals{ float TRPay; float TOPay; float TGPay; float TFITAmount; float TNPay; }; //Function Prototypes void OpenFiles(void); void CloseFiles(void); void PrintHeads(void); void PrintDetails(struct Employee); void PrintFooter(struct Totals); struct Totals ProcessLoop(struct Totals); struct Employee InputName(struct Employee); struct Employee InputDetails(struct Employee); struct Employee CalcPay(struct Employee); struct Employee CalcGPay(struct Employee); struct Employee CalcNPay(struct Employee); struct Employee CalcFIT(struct Employee); //Global Variables FILE *PFile; //Main Line int main() { struct Totals NTot = {0,0,0,0,0}; OpenFiles(); PrintHeads(); NTot = ProcessLoop(NTot); PrintFooter(NTot); CloseFiles(); return 0; } //Open Files void OpenFiles(void) { PFile = fopen("c:\datadump.txt","w"); return; } //Module to print report headers void PrintHeads(void) { char HL1[] = " Weekly Payroll Report Page 01\n"; char HL2[] = " 2/15/05"; char HL3[] = "\n\n Employee Hours Regular Overtime Gross"; char HL4[] = "\n Name Worked Pay Pay Pay FIT Net Pay\n\n"; fprintf(PFile,HL1); fprintf(PFile,HL2); fprintf(PFile,HL3); fprintf(PFile,HL4); return; } //Main module for processing all data struct Totals ProcessLoop(struct Totals NTot) { struct Employee NEmp = {"","",0,0,0,0,0,0,0,0}; NEmp = InputName(NEmp); while(NEmp.StpFlag != 1) { NEmp = InputDetails(NEmp); NEmp = CalcPay(NEmp); NTot.TRPay = NTot.TRPay + NEmp.RPay; NTot.TGPay = NTot.TGPay + NEmp.GPay; NTot.TNPay = NTot.TNPay + NEmp.NPay; NTot.TOPay = NTot.TOPay + NEmp.OPay; NTot.TFITAmount = NTot.TFITAmount + NEmp.FITAmount; PrintDetails(NEmp); system("cls"); NEmp = InputName(NEmp); } return NTot; } //Get the Employee Names struct Employee InputName(struct Employee NEmp) { char TFName[4] = ""; printf("Enter Employee Name\n"); printf("\nFirst Name: "); scanf("%20s",NEmp.FName); fflush(stdin); for(int count = 0;count <= 4;count++) TFName[count] = tolower(NEmp.FName[count]); if(strcmp(TFName,"stop") != 0) { printf("Enter Last Name: "); scanf("%10s",NEmp.LName); fflush(stdin); strcat(NEmp.FName," "); strcat(NEmp.FName,NEmp.LName); system("cls"); } else { NEmp.StpFlag = 1; } return NEmp; } //Input Employee Details struct Employee InputDetails(struct Employee NEmp) { printf("Enter Employee Details\n"); printf("\nEnter Hours Worked: "); scanf("%d",&NEmp.HrsWorked); fflush(stdin); printf("Enter Hourly Pay: "); scanf("%f",&NEmp.PayRate); fflush(stdin); return NEmp; } //Main Module to calculate pay and tax struct Employee CalcPay(struct Employee NEmp) { if(NEmp.HrsWorked > 40) { NEmp.RPay = 40 * NEmp.PayRate; NEmp.OPay = (NEmp.HrsWorked - 40) * (NEmp.PayRate * 1.5); } else { NEmp.RPay = NEmp.HrsWorked * NEmp.PayRate; NEmp.OPay = 0; } NEmp = CalcGPay(NEmp); NEmp = CalcFIT(NEmp); NEmp = CalcNPay(NEmp); return NEmp; } struct Employee CalcFIT(struct Employee NEmp) { const float FITRate = .15; NEmp.FITAmount = NEmp.GPay * FITRate; return NEmp; } struct Employee CalcGPay(struct Employee NEmp) { NEmp.GPay = NEmp.RPay + NEmp.OPay; return NEmp; } struct Employee CalcNPay(struct Employee NEmp) { NEmp.NPay = NEmp.GPay - NEmp.FITAmount; return NEmp; } void PrintFooter(struct Totals NTot) { char FL1[] = "\n\n Totals: "; fprintf(PFile,FL1); fprintf(PFile,"$ %8.2f",NTot.TRPay); fprintf(PFile," $ %8.2f",NTot.TOPay); fprintf(PFile," $ %8.2f",NTot.TGPay); fprintf(PFile," $ %8.2f",NTot.TFITAmount); fprintf(PFile," $ %8.2f",NTot.TNPay); return; } void CloseFiles(void) { fclose(PFile); return; } void PrintDetails(struct Employee NEmp) { fprintf(PFile," %-20s",NEmp.FName); fprintf(PFile," %2d",NEmp.HrsWorked); fprintf(PFile," $ %8.2f",NEmp.RPay); fprintf(PFile," $ %8.2f",NEmp.OPay); fprintf(PFile," $ %8.2f",NEmp.GPay); fprintf(PFile," $ %8.2f",NEmp.FITAmount); fprintf(PFile," $ %8.2f\n",NEmp.NPay); return; }

what exactly are your errors that you are having problems with. Your code compiled fine with devC++ on my box - with an exception.
line 102

Code:

for(int count = 0;count <= 4;count++)

this line makes your code less portable. you can do this in c99 i believe but not before. I simply declared int count oustside of the loop.

The other problems i see that others will surely comment on is using fflush(stdin) bad - as this is undefined.

You have a whole bunch of functions return type struct, which is just a waste of memory, cause your creating a copy of the struct with each call to the functions. See if you can't put a pointer to struct in their and do what needs to be done to the struct - its much more efficient and easiear on memory.

system("cls") isn't portable either. and calling system is bad as well. Search the faq and the board for why, they are constantly covered.

and as well you don't check to maker sure you can actually open a file, or that the file is actually closed properly. this line

Code:

PFile = fopen("c:\datadump.txt","w");

should be

Code:

PFile = fopen("c:\\datadump.txt","w");

otherwise you get an unknown escape sequence and you might get the creation of your file, but not where you expect.

also

Code:

char HL1[] = " Weekly Payroll Report Page 01\n"; char HL2[] = " 2/15/05"; char HL3[] = "\n\n Employee Hours Regular Overtime Gross"; char HL4[] = "\n Name Worked Pay Pay Pay FIT Net Pay\n\n";

I'd use some field width to display it correctly in the file. As it stands the file output is kinda hodepdge and all over the place. Have you tested this? check it out.

Since you want to know about your style, I would recommend using good formatting. Tabs make everything so much easier to read, especially when dealing with nested loops.

I just realized that I didnt post the changes and as far as the output I lined it up in notepad in windows. The first line throws it off because part of the project reqs are that the authors name goes in the first line of the report on the top left. I dont post any identifiable information over any internet medium Im sure you understand my logic there. Changes I will post in a second with the edit in this post.

here are the changes only

Code:

//Structure Definitions struct Employee{ char FName[20]; char LName[10]; char TFName[4]; int HrsWorked; float PayRate; float RPay; float OPay; float GPay; float FITAmount; float NPay; int StpFlag; }; //Main module for processing all data struct Totals ProcessLoop(struct Totals NTot) { struct Employee NEmp = {"","","",0,0,0,0,0,0,0,0}; NEmp = InputName(NEmp); while(NEmp.StpFlag != 1) { NEmp = InputDetails(NEmp); NEmp = CalcPay(NEmp); NTot.TRPay = NTot.TRPay + NEmp.RPay; NTot.TGPay = NTot.TGPay + NEmp.GPay; NTot.TNPay = NTot.TNPay + NEmp.NPay; NTot.TOPay = NTot.TOPay + NEmp.OPay; NTot.TFITAmount = NTot.TFITAmount + NEmp.FITAmount; PrintDetails(NEmp); system("cls"); NEmp = InputName(NEmp); } return NTot; } //Get the Employee Names struct Employee InputName(struct Employee NEmp) { printf("Enter Employee Name\n"); printf("\nFirst Name: "); scanf("%20s",NEmp.FName); fflush(stdin); for(int count = 0;count <= 4;count++) NEmp.TFName[count] = tolower(NEmp.FName[count]); if(strcmp(NEmp.TFName,"stop") != 0) { printf("Enter Last Name: "); scanf("%10s",NEmp.LName); fflush(stdin); strcat(NEmp.FName," "); strcat(NEmp.FName,NEmp.LName); system("cls"); } else { NEmp.StpFlag = 1; } return NEmp; }

These are all from the good code. One of the warnings was an unknown escape at the c:\datadump.txt which is just the file I use on my machine to capture the output without having to print it that gets changed to the network printer when its all done.

conversion from double to float right here

Code:

struct Employee CalcPay(struct Employee NEmp) { if(NEmp.HrsWorked > 40) { NEmp.RPay = 40 * NEmp.PayRate; NEmp.OPay = (NEmp.HrsWorked - 40) * (NEmp.PayRate * 1.5); /here for the warning } else { NEmp.RPay = NEmp.HrsWorked * NEmp.PayRate; NEmp.OPay = 0; } NEmp = CalcGPay(NEmp); NEmp = CalcFIT(NEmp); NEmp = CalcNPay(NEmp); return NEmp; }

and this line gives me c4305 initializing truncation const double to const float

Code:

const float FITRate = .15;

the first error Ive seen many times and havent really looked at since my test data always came out fine second pops up less often but same effect.

when I run the first version without and changes it will take all the data fine until I signal stop which is when it throws a windows error and asks me to debug which i do and the first thing it shows is

unhandled exception in payroll.exe:0xc0000005 Access Violation

then it throws me to the processloop return line where it returns NTot. NTot is the structure holding my totals right now and feeds them to the module that prints footers and final lines.

As far as all the passing. I was considering using three different structures but that made even less sense. Would it be better to not use a structure here or create smaller local variables and use only them or create the two strucutres but use the smaller cariables in the functions and use those as return values to fill the structure? Im just really not used to only being able to return one value at a time. I knew it looked strange and I was going to post it a few days ago I figured I would finish it with a few changes and then post the results with questions for comments

Quote:

Originally Posted by Taikon

when I run the first version without and changes it will take all the data fine until I signal stop which is when it throws a windows error and asks me to debug which i do and the first thing it shows is

Well, since the error is triggered by "stop", look at the code around "stop"

Code:

struct Employee InputName(struct Employee NEmp) { char TFName[4] = ""; printf("Enter Employee Name\n"); printf("\nFirst Name: "); scanf("%20s",NEmp.FName); fflush(stdin); for(int count = 0;count <= 4;count++) TFName[count] = tolower(NEmp.FName[count]); if(strcmp(TFName,"stop") != 0)

You have TFName[4], so the array elements have index 0, 1, 2, 3. Then the loop goes from 0 to 4. Furthermore there is no guarantee that the string you are giving to strcmp is teriminated with a zero byte anywhere within your legal address space.

Maybe you could try something like this;

Code:

char TFName[5] = {0}; /* initialize to all zeros */ for (count = 0; count < 4; count++) { TFName[count] = tolower(......)

Now, you have four characters followed by a '\0', so that strcmp won't throw a hissy fit (access violation).

By the way: I am shocked that none of the fflush(stdin) commandos has excoriated you about this despicable practice yet. My advice: don't use fflush(stdin). Even if it appears to work on your system, you shouldn't depend on it, since it isn't standard. There are other, standard, ways of emptying the input buffer when you really need to.

You really should get into the habit of always (always) checking return values from fopen(), scanf(), etc.

Regards,

Dave

A good tip..... when you have a problem and you are unsure where it is coming from, put some debug lines in the code.

The best way (I find personaly) to do this is to get the program to print "Debug 1", followed by a pause, then "debug 2" etc at different points in the program (if you know a certain part is executing fine then don't bother putting them there). When you run the program you will then know which parts have executed without a problem.

This serves to be very usefull as you can narrow the problem right down to a line sometimes and then i is A LOT easier to fix.

Sorry about bumping this but the question is along the same line and the program code already posted is a good example.

The question is about the structures and the variables Ive stored in them. I really can't explain why I have been doing it like this lately aside from experimenting with what you can and can't do with a structure. I understand why you wouldn't want to do this on almost any program thats used in a business enviornment or code it this way for a job as far as how I have passed the entire structure each time.

I guess I will phrase the actual question like this:
By convention or good habit how WOULD you want to store these variables?

It seems like these might be the best way to make updating the program for someone coming in behind me fairly easy. Im also thinking that I might have gotten a different response if I had passed only one variable or passed by pointer as well, although I am not exactly sure you can pass just one variable and I think I remember from experimenting that it caused some problems I had trouble pinpointing and fixing.

If this comes off as a confused ramble my apologies its still fairly early for me, and again sorry for the bump.