Skip to content

Conversation

@rick2244
Copy link

Some basic array method are added, and some graphic art is added to user space

Modification:
Makefile - arraytests.c is added to show tests of new methods
user - array.h, array.c, vagabond.c, mittens.c

Limitations:
Sort only works on integer arrays, identical with getMin and getMax method
graphic art files vagabound.c and mittens.c don't run in kernel, although a quick change allows that.

@atheneem
Copy link

atheneem commented Sep 25, 2024

I think your tests are great and your implementation is straight forward and readable. You definitely achieved what you outlined in the issue!

Some thoughts/advice:

  • Some lingering comments on the top of array.c you can erase :)
  • (^also I added an extra /n at the end of the test code so it looked nicer)
  • Nit-picky, but imo it might look nice to have ‘2147483647’ and ‘-2147483648’ be named constants or at least have a comment on it being MAX/MIN INT
  • Does swap() have to be in the .h file? (/would there be a time when the user would want to access this alone ig)

High Level Checks:

  • 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?
  • Is documentation included?
    (^ could add javadoc for args if you wanted)

Code Checks:

  • Does the code achieve its intended purpose?
  • Were all edge cases considered?
    (^ could also add special prints for null/empty lists if you wanted)
  • Is the code organized and maintainable?
  • Does the code maintain the same level of performance? (no performance regressions)

@chansrinivas
Copy link

I can code review this

@fhshaik
Copy link

fhshaik commented Sep 27, 2024

I would like to code review this

@fhshaik
Copy link

fhshaik commented Oct 1, 2024

High Level Checks:

  • PR fully resolves the task/issue
    Arraytests seems to highlight the utilities added to help deal with arrays. Works well. I am giving this a pass even though Vagabond art does not work, because the author mentioned a quick fix that adds it.
  • Does the PR have tests?
    The project has test cases that display the functionality of array utils.
  • Does the PR follow project code formatting standards?
    The PR does follow the correct coding format. Comments are well typed and easy to read.
  • Does the OS still compile and run?
    Yes it does.
  • Is documentation included?
    Documentation is included in array.c but not in arraytests.c, though I arraytests.c is easy to understand and thus does not need documentation. Comments are added periodically making it easy to read.
    Code Checks:
  • Does the code achieve its intended purpose?
    I don't think the code intended its complete purpose since the graphic art programs are not added to xv6.
  • Were all edge cases considered?
    The edge cases were considered well in the test util given, going over negative, positive, large, and small numbers.
  • Is the code organized and maintainable?
    The code is sufficiently organized, and also maintainable.
  • Does the code maintain the same level of performance? (no performance regressions)
    No performance decreases observable.

Overall a great project. Only suggestions are making the graphic art work, and adding other programs that allow for custom input so that I can test it more easily. With small improvements I think your project will be amazing!!

@chansrinivas
Copy link

All the features mentioned in the issue are implemented! I think your project is great and checks all the requirements mentioned.

  1. Your edge cases are handled well with duplicate numbers and negatives as well.
  2. Some minor changes, maybe have a test case that handles empty arrays.
  3. For documentation, you could also mention parameters and return values.
  4. You could use define the min and max variables as constants.

High level checks:

  • 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?
  • Is documentation included?

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?

@malensek
Copy link
Contributor

Had to modify the graphic art files to remove std libraries and include the xv6 headers, then add to Makefile, but after that they work great. You should fix those small issues so others can run this directly.

Array tests look good. I'd probably add more exhaustive tests with additional arrays here, but it's a good start. You could run the entire procedure in main but with several different arrays in a loop and then provide the expected outputs.

With these fixes and those above you can earn +0.5 to your project score.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants