Hexadecimal command line file viewer 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 &lt;stdio.h&gt;
#include &lt;stdlib.h&gt;

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(&amp;l, 1, 16, in)) &gt; 0){
                printf("%08x ", i);
                i += br;
                for(j = 0; j &lt; br; j++){
                    if(l[j] &gt; 0){
                        printf("%02x ", l[j]);
                    }else{
                        printf("?? ");
                    }
                }
                for(j = 0; j &lt; 16 - br; j++){
                    printf("   ");
                }
                for(j = 0; j &lt; br; j++){
                    if(l[j] &gt; 31 &amp;&amp; l[j] &lt; 127){
                        printf("%c", l[j]);
                    }else{
                        printf(".");
                    }
                }
                printf("\n");
            }
            fclose(in);
        }
    }else{
        printf("Please use the syntax 'hex [file]'\n");
    }
    return 0;
}
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 &lt;sysexits.h&gt; 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.


j 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(&amp;line, 1, 16, in)) &gt; 0){
    printf("%08llx ", offset);
    offset += bytesRead;
    for (size_t j = 0; j &lt; 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 &lt; bytesRead; j++) {
        if (line[j] &gt; 31 &amp;&amp; line[j] &lt; 127) {
            printf("%c", line[j]);
        } else {
            printf(".");
        }
    }
    printf("\n");
}