Add sublist exercise #224
Add sublist exercise #224
Conversation
exercises/sublist/src/example.c
Outdated
| size_t listTwoLength) | ||
| { | ||
| return (listOneLength == listTwoLength | ||
| && !memcmp(listOne, listTwo, listOneLength)); |
There was a problem hiding this comment.
Passing null pointers to memcmp works to an extent, but it's undefined.
There was a problem hiding this comment.
I think a more explicit check for null is in order then.
exercises/sublist/src/example.c
Outdated
| return EQUAL; | ||
| } else if (is_sublist(listOne, listTwo, listOneLength, listTwoLength)) { | ||
| return SUBLIST; | ||
| } else if (is_superlist(listOne, listTwo, listOneLength, listTwoLength)) { |
There was a problem hiding this comment.
is_superlist(One, Two, OneLength, TwoLength) = is_sublist(Two, One, TwoLength, OneLength)
There was a problem hiding this comment.
Yes, yes it does 😄
There was a problem hiding this comment.
I think keeping the helper is_superlist and just having it call is_sublist with reversed params is still clearer though. Thoughts?
exercises/sublist/src/sublist.h
Outdated
| SUPERLIST | ||
| }; | ||
|
|
||
| int check_lists(int *listOne, int *listTwo, size_t listOneLength, |
There was a problem hiding this comment.
The return value should be the enum type.
| { | ||
| int listTwo[] = { 1, 2, 3 }; | ||
|
|
||
| TEST_ASSERT_EQUAL(SUBLIST, check_lists(NULL, listTwo, 0, sizeof(listTwo))); |
There was a problem hiding this comment.
Passing the length argument as the size in bytes is pretty weird, and I don't think we do it in other array exercises. The length argument should instead be the number of elements.
There was a problem hiding this comment.
Yeah, I initially had it passing the number of elements and then I somehow convinced myself that it was weird, so I changed it. But, now I agree that it was right the first time.
|
Also remember that this needs the TEST_IGNORE() things too. #179 |
exercises/sublist/src/example.c
Outdated
| #include <stdbool.h> | ||
| #include <string.h> | ||
|
|
||
| static bool is_equal(int *listOne, int *listTwo, size_t listOneLength, |
There was a problem hiding this comment.
Parameter names should also be snake_case rather than camelCase. Applies to parameters of all functions as well as variable names (such as those in the test file). Basically snake_case everything, but upper-case SNAKE_CASE enum values and macros 🐍 > 🐫 😄
exercises/sublist/src/example.c
Outdated
| (list_one, list_two, list_one_element_count, list_two_element_count)) { | ||
| return EQUAL; | ||
| } else | ||
| if (is_sublist |
There was a problem hiding this comment.
Did indent put else and if on two lines?
There was a problem hiding this comment.
Yeah, it definitely did. But now it doesn't seem to want to do it anymore.
exercises/sublist/src/sublist.h
Outdated
| } comparison_result_t; | ||
|
|
||
| comparison_result_t check_lists(int *listOne, int *listTwo, | ||
| size_t listOneLength, size_t listTwoLength); |
There was a problem hiding this comment.
Make sure these variable names are lower_snake_case
|
I don't have a great suggestion for new names, but the current names of the arguments makes it difficult for me to tell which list is being checked against which. Is list 1 being checked against list 2 or vice-versa? It would be great if we could find names that make that clear without having to infer it from the tests. |
|
|
|
LGTM |
config.json
Outdated
| "control_flow_if_statements", | ||
| "control_flow_loops" | ||
| ], | ||
| "uuid": "928e9860-0df9-5180-8529-7024c991b3616adc21a" |
There was a problem hiding this comment.
does this need an "unlocked_by" field?
There was a problem hiding this comment.
Yeah, according to exercism/discussions#175, if it's core: true, then it needs unlocked_by: null. I guess my question now is if everyone thinks this should be a core exercise or if it should be unlocked by something else.
There was a problem hiding this comment.
Let's go with non-core. They want to have very few core exercises (I think 6-8?) so better to not burn them without a good reason.
There was a problem hiding this comment.
Ok, palindrome-products seems like a reasonable exercise to unlock this one, in my opinion
There was a problem hiding this comment.
Yes, palindrome-products seems like the best choice to unlock this IMO
| return false; | ||
| } | ||
|
|
||
| static bool is_superlist(int *list_to_compare, int *base_list, |
There was a problem hiding this comment.
Is this function really necessary? I assumed that the calls to this would be replaced by calls to is_sublist, not that this function would be implemented as is_sublist.
|
This looks fine to me except for setting it to a non-core exercise (I think that needs a change to the config.json) |
|
Oops... I made the change a few days ago but forgot to commit. |
Problem specification: https://github.com/exercism/problem-specifications/tree/master/exercises/sublist