Any better & efficient alternative to this C prg.

I have wrote the following C prg. to display random record contents from text file 'pix.db' (free format). It generates random num from 1 to 10 and then fetches corr. record from 'pix.db'

File pix.db format: (note '|' is my field sepator)
ID|pix file|pic name

for example:
1|pix1.gif|pix1name
2|pix2.gif|pix2name
3|pix3.gif|pix3name
;;
;;
;;
10|pix10.gif|pix10name

Here is the 'C' code which I consider best but I don't know how efficient it is..... Does anyone know better & efficeint code for the following code....I am not 'C' guru....but I know this forum has some finest 'C' experts....hope they help me out here....Thanks in advance.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>

void file_process(char *flname, int rnd_num, char *cdata);
void err_default(void);

int main(void)
{
FILE *rd_fp;
char *temp, *pch, *pname, *pix, got_err='N';
int pnum = 10; /* for random no. any record between 1..10 */

temp = (char *) malloc( sizeof(char) * 150);
pch = (char *) malloc( sizeof(char) * 150);

/* calling function passing 'pnum' */
file_process("pix.db", pnum, temp);

/* split the record */
strcpy(pch, strtok(temp, "|"));
if ( (pix = strdup(pch)) == NULL) got_err='Y';
strcpy(pch, strtok (NULL, "|"));
if ( (pname = strdup(pch)) == NULL) got_err='Y';

if(got_err == 'Y') print_error(); /* if error show default pix */

/* display content */
printf ("%s = %s", pix, pname);

free(temp); free(pch);
free(pix); free(pname);

return(0);
}

void file_process(char *flname, int rnd_num, char *cdata)
{
FILE *fp;
char recflag = 'N';
int rec_number, rnd_number;
time_t t;

/* generate random number */
srand((unsigned) time(&t));
rnd_number = rand() % rnd_num+1;

/* now get the corr. record */
if ((fp = fopen(flname, "r")) == NULL) print_error();
fscanf(fp, "%d", &rec_number);

while( !feof(fp))
{
fgets(cdata, 150, fp);
if(rec_number == rnd_number)
{
recflag = 'Y';
break;
}
fscanf(fp, "%d", &rec_number);
}
fclose(fp);
if(recflag == 'N') err_default();
}

void print_error(void)
{
printf ("ERROR - NO REC FOUND \n");
exit(1);
}

Yes, there's a much better way to do this. Use binary files. Use structures. Use fread. Use fseek.

Fixed size records are your friend.

Quzah.

> Fixed size records are your friend.
Code tags as well.

I have never used binary file....is there some good tutorial on the web where I can see some examples.......I appreciate your help.....My problem is I have a '|' delimiter in my database so I have no idea how to use fread and fseek functions.......
can you help me out with some sample coding. Thanks in advance.

> Fixed size records are your friend
Except that the original post said free format

Too much malloc and free where none is really required.

And you use the dreaded feof()

You also call fgets() and fscanf() to read the file, this is sure to lead to problems

Quote:

---

Originally posted by Salem
> Fixed size records are your friend
Except that the original post said free format

Too much malloc and free where none is really required.

And you use the dreaded feof()

You also call fgets() and fscanf() to read the file, this is sure to lead to problems

---

Free format meaning what exactly? He asked for a more efficient way of doing it. Fixed size records are the answer. If you really want to use a text file, you can, it's just not as efficient as fixed size.

Besides, this isn't really a "free format" anyway. The records are a fixed format. They just have varying lengths of strings. "Free format" sounds like you can have any arbitrary data in there, or any number of a given 'variable' in it. Example:

Code:

---

RECORD record1
integer 1000
string My name is BillyBobJimJack.
integer 3420
float 3.5
string Hello world
END

RECORD record2
string Use code tags.
string Read the FAQ.
string Stop asking how to clear the screen.
string Stop using 'void main'.
END

---

The parser in this case strips off the first word to see what the following will be. Then it reads to the end of the line. (Alternately, you could specify a delimit so strings could span multiple lines.)

The information would be treed or stored in a list of some form.
That'd be what I consider a 'free format'.

Quzah.

Can someone fix my code OR give me alternative solution for the above program. Acc.to Salem(moderator) my code has too many problems like he mentioned with feof and fscanf... etc. What would you suggest Salem if you were asked to find solution for
.....can you modify my code and give me correct one that you think.
I have posted same problem in other forumsbut no one could able to find better solution. Hope you or other would throw some light on.....

Thanks in advance. Appreciated.

1) Use a buffer to read into instead of calling malloc/free for simple text reads. Once you actually have valid data, then malloc for your strings if you need to.

2) Test the return value of your file read functions instead of using feof.

Those are the first improvements I'd make. I am not going to do all your work for you, but with the above, you should be able to take it from there.

Quzah.

Code:

---

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>

int main() {
    char buff[BUFSIZ];
    FILE *fp;
    int  i, rec;

    srand( time(NULL) );
    rec = rand() % 10 + 1;

    fp = fopen("sample.txt","r");
    for ( i = 0 ; i < 10 ; i++ ) {
        int n;
        fgets(buff,BUFSIZ,fp);
        if ( sscanf( buff, "%d", &n ) == 1 && n == rec ) {
            char *filename, *filedesc, *p;
            p = strtok( buff, "|" );
            filename = strtok( NULL, "|" );
            filedesc = strtok( NULL, "|" );
            printf( "%s %s\n", filename, filedesc );
            break;
        }
    }
    fclose( fp );

    return 0;
}

---

Hi Salem,
Thanks for the alternative code ......looks clean and shorter than mine. BUT I have couple of concerns regd. the code they are:

1) char buff[BUFSIZ]
what is BUFSIZ and its size, don't you need to define it somewhere.

2) int n;
why did you defined it inside for loop......any specific reason.

Appreciated. Once again thanks a million.

perlguy.

Quote:

---

1) char buff[BUFSIZ]
what is BUFSIZ and its size, don't you need to define it somewhere.

---

Yep, it's defined in stdio.h, so don't worry.

Quote:

---

2) int n;
why did you defined it inside for loop......any specific reason.

---

That way the variable is only visible within the loop, and not within the rest of the function. This is the same for most variables created within braces.

Thanks for propmt reply....this forum rocks....

1) should I free memory used by *filename, *filedesc, *p variables or not? or is it done by system automatically when prg. exits...

2) Is there any software to test performance of this prg.?
for example: In perl/cgi we have some test tools that tells how many request perl/cgi prg. can handle at a time......

let me know if any such tools fo C prg.

regards
perlguy.

Quote:

---

1) should I free memory used by *filename, *filedesc, *p variables or not? or is it done by system automatically when prg. exits...

2) Is there any software to test performance of this prg.?
for example: In perl/cgi we have some test tools that tells how many request perl/cgi prg. can handle at a time......

---

1 - No you don't. The memory is owned by the buff variable.

2 - Dunno! But you could write your own timer functions to encapsulate your code.