USF-OS / FogOS

Other
1 stars 24 forks source link

Implement head / tail #36

Open andrewdiep1 opened 4 months ago

andrewdiep1 commented 4 months ago

New Features:

This update introduces two new commands, head and tail, for viewing file content easily.

Options:

Test files are included as the text files added in the commit!

git547lab commented 4 months ago

I will code review this!

Update: I've learned that each person is expected to perform only one code review, so I won't be providing any further feedback on this code. (Your code looks great to me though.) I apologize for any inconvenience this may cause.

ajpolintan commented 4 months ago

I will code review this too

ajpolintan commented 3 months ago

Error Checking

I tried testing the program and here are the errors I found. If I try to enter head or tail for something that is not a text file, it returns a really weird value . For example: If I enter head mkdir it returns this

2024-03-07 09_43_43-ajpolintan@gojira__raid_ajpolintan_leaf-FogOS

Adding error checking to check if the file is a text file/file would help prevent the issue Also if I enter head . which is for a directory, it would return nothing. I think it would be useful to say something like head does not support directories or potentially list the text files in the directory instead.

I also think if you enter head, instead of saying Error: Needs Arguments you could say something like Error: Needs <file> and optional<flags> and print out what the helper flag has.

printf("ERROR: No valid files in any of the arguments.\n");

https://github.com/andrewdiep1/leaf-FogOS/blob/d9ed8614b1eb9f98933f292b52d4e37dd56d9109/user/tail.c#L219-L221

I also think for error checking instead of printing to STDOUT you can print to STDERR instead using fprintf(2, "ERROR: No valid files in any of the arguments.\n") and exiting right after using exit 1

Also if enter two text files such as head small.txt totem.txt it would only print out the small.txt file. I think you could say you do not support displaying multiple files at the moment or loop through each text file and print out the head/tail of its contents

Overall

I love how head and tail works and the ASCII art for the -f function. Great Job!

sanjanarattan commented 3 months ago

Hello! I think the code itself is great. I did not find any issues and what I am about to list is very subjective:

  1. I tried combining multiple flags in one command. If they contradict, the code takes the last flag and executes that. I would probably mention this in help ex: head -n 5 -c 10 small.txt returns This is a I would do the same for multiple input files as well.
  2. Regarding style: main is very long and it got a little hard to read at times. Maybe try breaking up the code or adding some comments to break apart the code and give the reader an idea of whats going on
  3. if you did break apart the code into more functions, you could check for errors before the coin flip so the user doesnt have to wait for the entire flip if their args are incorrect
malensek commented 2 months ago

This looks great, I like the coin flip addition too :-)

The suggestions you got above are good, and shouldn't be too much work to implement.

One more change: usually with head or tail, you can pipe data into them, and they'll read from stdin by default. That will make them both more useful (esp in pipelines). That shouldn't be more than a couple lines to support.

Nice work.