USF-OS / FogOS

Other
1 stars 24 forks source link

Added Character Functions #40

Open knchan03 opened 4 months ago

knchan03 commented 4 months ago

Implemented simple c character functions and strpbrk(). Uses Test file : testchar.c

Usage: For isdigit, isalpha, isupper, islower, toupper, tolower, ispunct: testchar _functname char

For isspace: testchar isspace

For charcount, findindexes, strpbrk, strcount: testchar _functname string substring

  1. int isdigit(int c);
  2. int isalpha(int c);
  3. int isupper(int c);
  4. int islower(int c);
  5. int toupper(int c);
  6. int tolower(int c);
  7. int ispunct(int c);
  8. int isspace(int c);
  9. int charcount(char* str, int c);
  10. void findindexes(char str, int c, int indexes, int* count);
  11. char strpbrk(char str, char* substr);
  12. int strcount(char str, char substr);
amayaling commented 3 months ago

This was a good program and is easy to use, but I would suggest adding documentation to explain what you are doing especially in ulib.c. Also, returning 0 and 1 for functions 1-4 does nothing for the user-- these are more boolean-like functions so maybe return true/false depending on what you have defined 1 and 0 to mean. Additionally, I am assuming that when something returns 0 that it is "true" and 1 is "false" since more c functions are like that, but I think you have it reversed, so that may be something to think about.

For functions 1-4, if I don't provide an argument after the function name, the program runs regardless. Perhaps you could error handle and output something to the user to input something and then re-ask for input; when I test isdigit() without any arguments it returns 1, when I feel like it should return 0 or give me an error.

I think isspace may also have some issues with its implementation; I think it would be more efficient to take in a string and iterate over every character and check for all of the spaces, not just the first one it encounters. Here is a problem I encounter as well: testchar isspace "this is a test" func : isspace / char : : 1 func : isspace / char : s : 0 $ Not sure what the output is really telling me.

Otherwise, these are pretty useful functions! I do recommend renaming strpbrk and strcount, its not clear what you are doing until I read the code and test it out, and strcount sounds like we are doing something else entirely. They are good functions though. Nice work

MessiBest07 commented 3 months ago

Code review: Code reuse: isalpha, isupper, islower implementations have repetitive range judgments, which can be simplified by calling other existing functions, such as isupper and islower implementations of isalpha

findindexes function In this function, you try to store the indexes in the passed indexes array. You should initialize each element of the indexes array to -1 (indicating that no characters were found at that location) and update the correct location when the characters are found. Memory access: This function assumes that the indexes array has enough space to store all possible indexes, which can cause the array to go out of bounds.

It's even more useful to add some applicable fixations.

malensek commented 2 months ago

Looks good. For the last few functions where their inputs/outputs are not completely obvious, I do agree with the two ChatGPT-generated code reviews above that they could use some documentation. You followed the standard when implementing these so I would not recommend changing the return values.

Now that you can (probably) write shell scripts in your OS (with your new shell), the only addition to this I would request is a script that does several runs of testchar to demonstrate the functionality. As it stands now, the testing is on the user to do and if there's a regression or something like that we won't be able to catch it later. So a standardized set of tests would be great to have.