Code:
record.h
#define size 40
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
typedef struct{
char name[size];
char address[size];
char citystate[size];
char zipcode[size];
}CITYRECORDS;
typedef CITYRECORDS *CRP;
CRP p;
int readin(CITYRECORDS *city[]);
void writot(CITYRECORDS *city[], int num);
1) It sometimes causes problems to hide a pointer type by typedefing it and obscuring its presence.
2) Don't put variables in your header files like what you have for CRP p. This file is included in two separate source files which means each one, when compiled, gets its own version of the variable p. When the linker tries to merge the two object files record.obj and progb6b.obj it should be spitting out a linker error about variable already being defined somewhere else. The CRP type variable p is only used in the record source file so it should only be declared there. This error should really have prevented you from building your program at all.
3) Might be a matter of personal taste, but those #includes don't really belong there. The header should be minimalist in that it should only contain #include statements that are required for that header to compile. Since the header itself doesn't contain anything other than some prototypes and struct definitions and typedefs, no #include statements should be necessary at this point. These should be in the individual source files themselves.
4) Your two source files define the values CITIZENS/MAXCITYREC as 50. To avoid confusion you should simply create a single define and place it in the header and reference that identifier in both of your source files.
5) Consider using header inclusion guards in any header as well.
Considering all of that I would have this as my record.h header file:
Code:
#ifndef RECORD_H
#define RECORD_H
#define size 40
#define MAX_CITIZENS 50
typedef struct{
char name[size];
char address[size];
char citystate[size];
char zipcode[size];
}CITYRECORDS;
int readin(CITYRECORDS *city[]);
void writot(CITYRECORDS *city[], int num);
#endif RECORD_H
Code:
progb6b.c
#define CITIZENS 50
#include "record.h"
int main(void){
CITYRECORDS *city[CITIZENS];
int n=readin(city);
writot(city, n);
return 0;
}
6) As mentioned, that #define statement should be gone, use the one you declare in the header (I've named it MAX_CITIZENS in my example). Nothing in this source file requires any of the stdio/stdlib/string headers so record.h not having them is not an issue here.
Code:
record.c
#define MAXCITYREC 50
#include "record.h"
char buffer[81];
int readin(CITYRECORDS *city[]){
int num=0;
while((gets(buffer)!=NULL)&&(num<MAXCITYREC)){
p=(CRP)malloc(sizeof(CITYRECORDS));
strcpy(p->name,buffer);
gets(p->address);
gets(p->citystate);
gets(p->zipcode);
city[num++]=p;
}
return num;
}
void writot(CITYRECORDS *city[], int num){
CITYRECORDS *pcr;
for(pcr=*city;pcr<*city+num;pcr++){
printf("%s\n%s\n%s\n%s\n", pcr->name, pcr->address, pcr->citystate, pcr->zipcode);
}
}
Disregarding, for the moment, your use of gets we have...
7) If you need to place your typedef for the CITYRECORDS* here then do so in this source file, it doesn't need to be anywhere else.
8) The define should be removed and in its place simply use whatever you choose as its replacement in the header.
9) This source file requires the use of the stdio/stdlib/string headers so they should be included here.
10) The return value from malloc calls should not be typecast. The malloc function returns a void* pointer that can be safely assigned without need of a typecast to CRP.
11) You do not test the return value of the malloc call, you simply assume that it works.
12) There is the possibility your gets call will read more than the number of bytes/characters that can be safely stored in your struct members. In particular, your input into the variable buffer can safely be up to 80 characters (for whatever the reason you've decided). Then, you call strcpy into a destination buffer that can only hold up to 40 characters. So here, there might be a case where a gets call succeeds only to have the strcpy cause problems.
13) Your output code is a bit messy and the likely cause of some of your weird output. I'd suggest something more like:
Code:
void writot(CITYRECORDS *city[], int num){
for(int i = 0;i<num;i++){
printf("%s\n%s\n%s\n%s\n", city[i]->name, city[i]->address, city[i]->citystate, city[i]->zipcode);
}
}
In general...
14) What you malloc you should also free. Modern operating systems will recover the memory as your program exits but you should not rely on this yourself. Be neat and pick up after yourself. Put some code in there to free up that memory.
