HiveBrain v1.2.0
Get Started
← Back to all entries
patterncModerate

Showing the contents of a file

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
contentstheshowingfile

Problem

This program reads a filename from standard input and then prints its content. Please review:

#include 
#include 
#include 
#include 

int main()

{
    char fileName[20];

    // Get the filename from the user:
    int fileNameLength = read(0,fileName,19);
    // We need to get rid of the new line character caused by terminal
    // Replace the new line character with \0 
    fileName[fileNameLength-1] = fileName[fileNameLength];

    printf("You want to see the contents of: %s\n", fileName);

    // open the file:
    int fd = 0;
    fd = open(fileName,O_RDONLY);

    if(fd == -1) {
        // Something went wrong, perhaps no such file:
        perror(NULL);
        printf("%s\n", fileName);
    } else {
        // Read until the read method returns 0 bytes. 
        char buf[20];
        int numBytesRead = read(fd,buf,20);
        while(numBytesRead) {
            write(1,buf,numBytesRead);
            numBytesRead = read(fd,buf,20);
        }
        close(fd);
    }
    puts("");
}


And in action:

Korays-MacBook-Pro:~ koraytugay$ gcc koray.c
Korays-MacBook-Pro:~ koraytugay$ ./a.out
k.txt
You want to see the contents of: k.txt
Hello Code Review!
This is the contents of k.txt.
Have a good day!
Korays-MacBook-Pro:~ koraytugay$

Solution

Magic numbers 20 and 19?

Instead of this:

char fileName[20];

// Get the filename from the user:
int fileNameLength = read(0,fileName,19);


Define MAX_FILENAME_LENGTH somewhere, and use that as the buffer size parameter, and MAX_FILENAME_LENGTH + 1 as the array size containing the buffer, to account for the terminating null character.

Write in code your intention exactly

Write in code more explicitly what you want.
For example here you say in comment that you want to write a \0 to the final position of the char[], but the code doesn't do exactly that:

// We need to get rid of the new line character caused by terminal
// Replace the new line character with \0 
fileName[fileNameLength-1] = fileName[fileNameLength];


Don't assume what might be at an uninitialized memory location.
If you want to set the terminating character to \0,
then do exactly that:

fileName[fileNameLength-1] = '\0';


More magic number 20

All those 20 everywhere:

char buf[20];
int numBytesRead = read(fd,buf,20);
while(numBytesRead) {
    write(1,buf,numBytesRead);
    numBytesRead = read(fd,buf,20);
}


Why not introduce a variable so that you can change it later if you want to:

int bufsize = 20;
char buf[bufsize];
int numBytesRead = read(fd,buf,bufsize);
while(numBytesRead) {
    write(1,buf,numBytesRead);
    numBytesRead = read(fd,buf,bufsize);
}


And why use a buffer of size 20? Why read a file 20 byte at a time? It would be faster to read in larger chunks. Surely you have enough memory to load 4 kbyte at a time. Luckily, now it's easy to change that:

int bufsize = 4096;


Avoid code duplication

In the previous code snippet, read(fd,buf,bufsize); appears twice,
which is not pretty. It can be rewritten without such duplication:

int numBytesRead;
while ((numBytesRead = read(fd, buf, bufsize)) > 0) {
    write(1, buf, numBytesRead);
}
close(fd);


On the other hand, some people find this writing style potentially error prone or confusing. In my opinion code duplication is the bigger evil,
so I still prefer this writing style, which is also shorter.

Comments stating the obvious

// open the file:
int fd = 0;
fd = open(fileName,O_RDONLY);


Is that comment really necessary there? Or is it just noise?

Pointless variable initialization

int fd = 0;
fd = open(fileName,O_RDONLY);


If you're going to set fd to something else, why set it to 0?

Usability

When you run the program,
it prints nothing,
it's just waiting for user input.
It would be better to print a prompt,
for example:

puts("Enter file name:");

Code Snippets

char fileName[20];

// Get the filename from the user:
int fileNameLength = read(0,fileName,19);
// We need to get rid of the new line character caused by terminal
// Replace the new line character with \0 
fileName[fileNameLength-1] = fileName[fileNameLength];
fileName[fileNameLength-1] = '\0';
char buf[20];
int numBytesRead = read(fd,buf,20);
while(numBytesRead) {
    write(1,buf,numBytesRead);
    numBytesRead = read(fd,buf,20);
}
int bufsize = 20;
char buf[bufsize];
int numBytesRead = read(fd,buf,bufsize);
while(numBytesRead) {
    write(1,buf,numBytesRead);
    numBytesRead = read(fd,buf,bufsize);
}

Context

StackExchange Code Review Q#92238, answer score: 13

Revisions (0)

No revisions yet.