History command - Githubissues

ainsleeSmith commented 4 months ago

README is called history.txt. There is no test file, but to test it we have it set to 10, and we were inputting commands to test it out and make sure it prints the commands in the right order. We were testing them with typing numbers in the command line.

katherineanthony commented 4 months ago

I will code review this one!

katherineanthony commented 3 months ago

Hi all! Great work! Here are some suggestions I have for y'all:

I noticed that at the end of the list, if there aren't enough commands to print, it will print nulls. I think you could add a statement that stops printing after null entries.
On the history command in bash/zsh, they add a count of the commands next to the commands, and print out the counts when you run history. Could this be something you implement?
Additionally (this is super nit-picky) the first command does not print in line with the rest of them.
In general, I think y'all could add some more comments (but I'm a comment lover so that could just be me)

Code-specific reqs:

There's some dead code in ulib.c that I'm not sure why y'all commented out
There's some spacing differences in the code (white space around parenthesis/etc)
Would it be possible to change the size? Maybe there is a flag that the user could implement that would show more/less commands so that you can show more/less commands? I know that y'all have it hardcoded, but this is just a suggestion to make it a little more interactive
I'm noticing in queueWriteFile() and queueStart() you never close the file you write to, which could lead to issues for file opening/closing in the future.

Overall I think y'all did a great job!! This was super great & I'm super impressed :)

ghimirelava commented 3 months ago

Thank you Katherine for your code review. From the list of edits you provided we have:

removed some dead code in ulib.c
fixed the spacing issue when the history outputs
fixed the issue of having nulls print out when the history queue is not full

AndrewLiu666 commented 3 months ago

Hi, Hana and I are going to code review this project!

hanag1234 commented 3 months ago

Hi, good job on your project!

Andrew and I (Hana) suggest these upgrades:

Consistency in Coding Style: Consistent comment styles (// for inline comments and /* */ for block comments) and ensuring they are meaningful and explain how the code functions.
Memory Management: In enQueue(), you allocate memory using malloc but there are no corresponding free() for the dequeued elements.
Error Handling: Add error checking for system calls (malloc, open, write, etc.) to handle failures.
queueWriteFile: This function has a bug in calculating sz and using it for modulus operation j = (j+1) % sz; which might not correctly wrap around the queue if it's not full. Use SIZE for modulus operation to correctly cycle through the queue.

Overall, I think you did great and the code is well written.

ainsleeSmith commented 3 months ago

Thank you for your code reviews! From the suggested edits we have fixed(pt2): • fixed the sz for queueWriteFile() • closed our files in queueWriteFile() and queueStart() • added comments and fixed comment style consistency • fixed spacing differences in code

malensek commented 2 months ago

Awesome work! I spent some time trying to break this to no avail. Thanks for already doing all the proposed fixes.

USF-OS / FogOS

History command #43