USF-OS / FogOS

Other
1 stars 24 forks source link

Array Operations (#12) request #30

Open atanaspatterson opened 4 months ago

atanaspatterson commented 4 months ago

Added the following functions (user/arrays.h, user/arrays.c, user/testarrays.c file ):

void print_array(int arr, int size); void sort_int_array(int arr, int size); void delete_at_index(int* arr, int index, int size); void add_at_index(int* arr, int value, int index, int size); int binary_search(int arr, int target, int size); int array_union(int arr1, int arr2, int size1, int size2, int new_size); int array_intersection(int arr1, int arr2, int size1, int size2, int *new_size);

git547lab commented 4 months ago

I will code review this!

Ndewedo-Newbury commented 4 months ago

I will be peer reviewing this :)

Ndewedo-Newbury commented 3 months ago

Code Review suggestions:

image

amayaling commented 3 months ago

Your functions are well named! A few minor tweaks: hard coding the size is sort of iffy but sizeof() does return an error so I understand. Similar to above, the intersection function seems to print the entire array rather than only the items that intersect. Same thing happens with union: int testarray[] = {1, 4, 18, 3, 34, 6, 7, 4, 9}; int array[] = {1, 3, 3, 3, 3, 4, 5}; Union: 1 3 3 3 3 4 5 6 7 9 18 34 Adding to the above comment, other data types like floats or doubles would be a way to add to your project. Otherwise good job!

git547lab commented 3 months ago

Nice work! Here are my suggestions for some minor issues:

  1. The test program only tests one instance of an integer array for each function you created. You can consider demonstrating more test cases.
  2. In arrays.h, you could add more comments to explain the parameters and return values of the functions. For binary_search, you might want to specify that the input array should be sorted.
  3. From line 136 to line 138 in arrays.c, all arr1 values are inserted to union_arr regardless of whether arr1 includes duplicate values or not. This can cause errors in finding the union. For example, if arr1 is {1, 1} and arr2 is {1}, the generated union is {1, 1} instead of just {1}.
  4. In arrays.c, functions delete_at_index, add_at_index, array_intersection, and array_union all have some dynamically-allocated memory buffers that are not freed in these functions or in the main(). They should be freed when they are no longer used.
  5. It seems the function print_array in array.c can only print an array of integers and it's mainly used for testing your code. You can consider defining it in testarrays.c instead.
  6. To be consistent with other existing C files in FogOS, consider using two spaces instead of a tab for indentation in your C code.
  7. For line 93 and line 124 in arrays.c, add one space to the left of ":" to change "size1: size2" to "size1 : size2".
  8. Remove line 132 in array.c and change "j = 0" to "int j = 0" in line 141 since variable j is only used inside the for loop.
malensek commented 2 months ago

Looks good. The 3 code reviews cover what needs to be changed, although you don't need to support data types beyond int (that can be added in a future semester) or move anything to user.h.

This is lacking documentation as well, and I think in this case it's certainly something the library needs.

atanaspatterson commented 1 month ago

At this point I believe I have addressed all of the code review points!

git547lab commented 1 month ago

I found memory leaks after running my leak check on your testarrays.c:

-- Leak Check -- [BLOCK 0x0000000000006070] 64 '' [BLOCK 0x00000000000060B0] 64 '' [BLOCK 0x00000000000060F0] 80 '' [BLOCK 0x0000000000006140] 80 ''

-- Summary -- 4 blocks lost (288 bytes)

Adding the following code to testarrays.c could resolve the issue: free(intersection); free(arr); free(union_arr); free(union_arr2);

Thanks!