patterncModerate

Hexadecimal command line file viewer

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

filehexadecimallineviewercommand

Problem

I have been working on a project for several days. It is a command line program for viewing files in hexadecimal. It is usually very fast, but is there any way I could make it more efficient?

#include 
#include 

int main(int argc, char *argv[]){
    if(argc == 2){
        FILE *in = fopen(argv[1], "rb");
        if(in == NULL){
            printf("Could not open %s\n", argv[1]);
        }else{
            int i = 0, j, br, ln[8];
            char l[16];
            while((br = fread(&l, 1, 16, in)) > 0){
                printf("%08x ", i);
                i += br;
                for(j = 0; j  0){
                        printf("%02x ", l[j]);
                    }else{
                        printf("?? ");
                    }
                }
                for(j = 0; j  31 && l[j] < 127){
                        printf("%c", l[j]);
                    }else{
                        printf(".");
                    }
                }
                printf("\n");
            }
            fclose(in);
        }
    }else{
        printf("Please use the syntax 'hex [file]'\n");
    }
    return 0;
}

Solution

If a program fails for some reason then it should

Write a diagnostic message to standard error (not standard output),

so that it can not be mistaken for normal output, e.g. when
the program runs in a pipe. strerror(errno) gives you an error
message string corresponding to the last error, e.g.
"No such file or directory" or "Permission denied".

Terminate with a non-zero status code. You can use predefined

exit codes from `

 if that is available on your system,
or make up your own.

Combining this with the "Avoiding deep nesting" advice from
Caridorc's answer, the overall structure would be

int main(int argc, char *argv[]) {

    if (argc != 2) {
        fprintf(stderr, "Usage: hex [file]\n");
        exit(EX_USAGE);
    }

    FILE *in = fopen(argv[1], "rb");
    if (in == NULL) {
        fprintf(stderr, "Cannot not open %s: %s\n", argv[1], strerror(errno));
        exit(EX_NOINPUT);
    }

    // ... print hex dump ...

    fclose(in);

    exit(EXIT_SUCCESS); // EXIT_SUCCESS == 0
}


Your variable names are too short and non-descriptive.

i is the current offset into the file. The correct type for



a file offset is

off_t

br is the amount of bytes read. The correct type would

be

size_t (which is what fread()

 returns, so this avoids
a warning when compiling on a 64-bit architecture).

l is a line buffer.

 is OK as a iteration variable, but the scope should be limited to the
for-loop. 

The main loop would then start like this:

off_t offset = 0;
size_t bytesRead;
char line[16];

while ((bytesRead = fread(&line, 1, 16, in)) > 0){
    printf("%08llx ", offset);
    offset += bytesRead;
    for (size_t j = 0; j < bytesRead; j++){
        printf("%02x ", line[j]);
    }


I don't know why you want to print a NUL-byte as

??`, therefore
I have removed that part.

The padding can be done by printing an empty string
with a field width:

printf("%*.s", 3*(16 - (int)bytesRead), "");

The remaining part of the main loop is OK, only that I would
generally use more space, at least around parentheses and keywords:

for (int j = 0; j  31 && line[j] < 127) {
            printf("%c", line[j]);
        } else {
            printf(".");
        }
    }
    printf("\n");
}

Code Snippets

int main(int argc, char *argv[]) {

    if (argc != 2) {
        fprintf(stderr, "Usage: hex [file]\n");
        exit(EX_USAGE);
    }

    FILE *in = fopen(argv[1], "rb");
    if (in == NULL) {
        fprintf(stderr, "Cannot not open %s: %s\n", argv[1], strerror(errno));
        exit(EX_NOINPUT);
    }

    // ... print hex dump ...

    fclose(in);

    exit(EXIT_SUCCESS); // EXIT_SUCCESS == 0
}

off_t offset = 0;
size_t bytesRead;
char line[16];

while ((bytesRead = fread(&line, 1, 16, in)) > 0){
    printf("%08llx ", offset);
    offset += bytesRead;
    for (size_t j = 0; j < bytesRead; j++){
        printf("%02x ", line[j]);
    }

printf("%*.s", 3*(16 - (int)bytesRead), "");

for (int j = 0; j < bytesRead; j++) {
        if (line[j] > 31 && line[j] < 127) {
            printf("%c", line[j]);
        } else {
            printf(".");
        }
    }
    printf("\n");
}

Context

StackExchange Code Review Q#115661, answer score: 12

Revisions (0)

No revisions yet.