current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I am trying to write binary values in a file using fputc. In my case it is a short int. The file must be opened in "w" mode.

I'm using this function, which is working. I need to know if I'm doing it right or if there are cases that I hadn't anticipated.

void writeListPos(Compressor* comp, short int listPos){
    ++listPos;
    char* posChar = (char*)(&listPos);
    int i;
    for(i=0 ; i<(sizeof(short int)) ; ++i){
        fputc(posChar[i] , comp->outputFile);
    }
}

The function to read the file later:

short int readListPos(Compressor* comp){
    char* posChar = (char*)(malloc(sizeof(short int)));
    int i;
    for(i=0 ; i<sizeof(short int) ; ++i){
        posChar[i] = comp->lastRead;/* comp->lastRead initialised with a first fgetc*/
        comp->lastRead = fgetc(comp->inputFile);
    }
    short int* positionp = (short int*)posChar;
    short int position = (*positionp);
    free(posChar);
    --position;
    return position;
}
share|improve this question

migrated from stackoverflow.com May 27 at 15:55

This question came from our site for professional and enthusiast programmers.
2  
1) must be opened in "wb" mode. 2)not x86 has alignment problem. short int* positionp = (short int*)posChar;short int position = (*positionp); 3) fwrite & fread is better. –  BLUEPIXY May 26 at 16:27
1  
plus: you must have read at least one byte and have stored it in comp->lastRead before calling readListPos() for the first time –  Ingo Leonhardt May 26 at 16:29
    
The problem is that with this system I can write a -1 (which is EOF) in the file which is pretty bad... –  TTAK May 26 at 16:37
add comment

1 Answer

up vote 2 down vote

Using malloc() and free() in readListPos() seems like overkill. You could use:

short s;
char *posChar = (char *)&s;

for (int i = 0; i < sizeof(s); i++)
{
    int c;
    if ((c = fgetc(comp->inputFile)) == EOF)
        …handle EOF…
    *posChar++ = c;
}
return s;

Note that this code handles EOF, unlike the original; you must always error check your inputs.

This code matches the code that writes the data. It is, however, tied to a particular endian-ness; the written data is not portable between, say, an Intel machine and a SPARC or PowerPC machine. If that doesn't matter to you, then you're more or less OK.

For non-portable reading and writing of binary data, the fread() and fwrite() functions are simpler.

Reading:

short s;
if (fread(&s, sizeof(s), 1, comp->inputFile) != 1)
    …process error/EOF…
return s;

Writing:

if (fwrite(&listPos, sizeof(listPos), 1, comp->outputFile) != 1)
    …process error…

If you're doing it portably, you define whether the number is written big endian or little endian and then write the bytes in that order on both big endian and little endian machines. (If you can demonstrate it's a performance problem, you can optimize one of the two platform types, but it complicates the code for what is usually negligible benefit. ('Premature optimization is the root of all evil.')

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.