patterncModerate
Showing the contents of a file
Viewed 0 times
contentstheshowingfile
Problem
This program reads a filename from standard input and then prints its content. Please review:
And in action:
#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:
Define
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
Don't assume what might be at an uninitialized memory location.
If you want to set the terminating character to
then do exactly that:
More magic number 20
All those 20 everywhere:
Why not introduce a variable so that you can change it later if you want to:
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:
Avoid code duplication
In the previous code snippet,
which is not pretty. It can be rewritten without such duplication:
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
Is that comment really necessary there? Or is it just noise?
Pointless variable initialization
If you're going to set
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:
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.