strftime + strfmon

[peterregal8]

strftime and strfmon

This pull request implements two new functions to the user library, strftime and strfmon. Both of these involve formatting a string according to certain conversion specifications. Documentation for each of these functions, as well as build and testing instructions, can be found in docs/strftime.md and docs/strfmon.md.

[Maxxzzz]

I will code review this

[Marcosg1]

I will code review this

[Marcosg1]

Verification
The scope of this pull request was to implement two new user-level functions — strftime() and strfmon(). These functions provide string formatting capabilities for date/time and monetary values.

After testing it locally, I found that both functions compile successfully and behave as expected

Testing
The testing went great! It was easy and simple to create my own tests in the given test files you provided. I made many other test cases and I didn't run into any major issues in your code. One suggestion I had was to consider restricting non-numerical characters in your strfmon implementation, since it’s intended for monetary values.

Code Walkthrough

• You covered your bases when it came to testing, all looked outstanding. I have one only one thing I would like to see which was the non-numerical characters in the strfmon.
• Your documentations were super easy to follow and understand on how to use strftime and strfmon.
• The comments you have were really helpful to see what you were doing in your code.
• After reading everything on both utilities I thought your logic was straightforward and consistent.

Suggestions for improvement:
I did a bit of tests and there were only one minor thing I saw that could be added. For strfmon() consider adding error handling for non-numeric inputs.

Formatting
Your code formatting and naming conventions were consistent all around and the comments are concise. I found no formatting issues.

Checklist:
High Level Checks:

• [✅] PR fully resolves the task/issue
• [✅] PR includes test cases (testTime.c and montest.c)
• [✅] PR follows the project code formatting standards
• [✅] The OS compiles and runs successfully
• [✅] Documentation is included. Very easy to understand how to build and test :)

Code Checks:

• [✅] The code achieves its intended purpose
• [] Almost all edge cases were considered
• [✅] Code is organized and maintainable
• [✅] The code maintained the same level of performance

[aryasri16]

Good job with your documentation! Testing also went well for me. I will have to say that I agree with the other code review that you could have more error handling to account for edge cases such as non-numeric inputs, but I will give you the check mark anyways because you did account for almost all edge cases :-)

However, your code kind of has a lot of unused/commented out code and I noticed that you guys left todo comments in the repo (specifically time.c). I think if you remove all this unused code + finished todos you’ll be all set in terms of keeping your code organized and maintainable.

High Level Checks:

• Is documentation included? This should explain how to build, run, and test the software.
• PR fully resolves the task/issue
• Does the PR have tests?
• Does the PR follow project code formatting standards?
• Does the OS still compile and run?

Code Checks:

• Does the code achieve its intended purpose?
• Were all edge cases considered?
• Is the code organized and maintainable?
• Does the code maintain the same level of performance? (no performance regressions)

[Maxxzzz]

Verification: Both strftime and strfmon worked for me when i tested with the generic inputs that we were assigned by default and produced the output as described in the docs.

Testing: Testing was very simple with thorough instructions on how to run both programs with example inputs and outputs of what we should get to confirm whether our testing resulted in correct or incorrect results.

Code Walkthrough: Honestly while testing both strftime and strfmon I barely ran into any error or edge cases besides what was already stated with non numerical numbers which I didn't really test. Your documentation was clear and consise on exactly how to run each test this time by hard coding either strfmon or strftime then we can run either montest or timeTest.

Formatting: For the most part the formatting was well defined and consistent although there were a few todo comments in time.c and commented out code. Also in time.c comments were consistent and helpful to understand what everything was doing, I especially found the longer comments to be helpful in understanding exactly what was suppose to happen during certain sections of your code.
With long int strfmon(char *restrict s, size_t maxsize, const char *restrict format, ...) { in ulib.c its a bit more unorganized and incosistent with comments as for example lines 190 - 220 we get a bunch of helpful comments like for example // Fills the buffer character by character from format until % is reached but for other sections like from lines 238 - 274 there is only one comment // Handling grouping which is a little big vague especially when it seems like in these lines there is a lot of checking strlen and adding ",". I would suggest adding a bit more comments throughout your code so its a little bit easier to navigate around it and for explanation purposes.

High Level Checks:

• PR fully resolves the task/issue
• Does the PR have tests? (Honestly easy to run tests exactly how I would have formatted it)
• Does the PR follow project code formatting standards?
• Does the OS still compile and run?
• Is documentation included? (Yes perfect documentation on how to setup and run each program)

Code Checks:

• Does the code achieve its intended purpose?
• Were all edge cases considered? (for the most part yes)
• Is the code organized and maintainable? (time.c needs a small cleanup and ulib.c needs a bit more comments so it looks a bit more organized)
• Does the code maintain the same level of performance? (no performance regressions)

[malensek]

This looks good. I think the reviewers above have some good suggestions regarding cleaning this up a bit.

I like that you implemented a separate time header for this.

One thing I am not a fan of is that one of these functions is now part of ulib.c while the other is not. Ideally these should be in one place, and even more ideally there should be a string.h and string.c so users can include these advanced string functions if they want them. Right now strftime works largely because of xv6's build process, (it gets compiled and is available to all utilities just because of how things are built).

4.75/5
Earn +0.25 by moving this into string.h / string.c, cleaning up the code, and then providing clear test files that use the new header(s).