-
-
Notifications
You must be signed in to change notification settings - Fork 176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add 'allergies' exercise #83
Conversation
Wow... I have no idea why every test is failing on the Travis build. They all pass on my machine. |
if (is_allergic_to(allergen, score)) { | ||
score -= scores[allergen]; | ||
list->allergens[list->count] = allergen; | ||
list->count++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this guy is never initialized to 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I originally had get_allergens()
mallocing the entire Allergen_List_t
and then switched to having it passed in. Will fix.
|
||
void get_allergens(int score, Allergen_List_t * list) | ||
{ | ||
list->allergens = calloc(0, sizeof(Allergen_t) * Allergen_Last); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allergen_Last
is 7, but there are actually 8 allergens.
I think it will make things a bit simpler if the allergen enum is rearranged like this:
enum {
Allergen_Eggs,
Allergen_Peanuts,
...
Allergen_Count
}
Then you can rearrange your for
to use allergen < Allergen_Count
as the termination condition and use sizeof(Allergen_t) * Allergen_Count
as your allocation size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, the signature for calloc
is calloc(<number of items>, <item size>)
so I think this should be calloc(Allergen_Last + 1, sizeof(Allergen_t))
(this is probably the reason that all of the tests fail on Travis).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I'm sure that's why the tests are failing.
|
||
typedef struct { | ||
int count; | ||
Allergen_t *allergens; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we know exactly how many allergens there are prior to declaring this type, why wouldn't we simply create a properly sized array for allergens
instead of requiring a memory allocation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess leaving this as an allocation means that the implementation could potentially allocate only as many as are actually necessary for the given score.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could go either way. I do like having the option allocate only as many as necessary.
Allergen_t *allergens; | ||
} Allergen_List_t; | ||
|
||
bool is_allergic_to(Allergen_t allergen, int score); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the score be an unsigned value? If you're treating this as a bitfield it will be safer and less confusing to have it be an unsigned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Will fix.
Alright, tests are passing now. |
} Allergen_List_t; | ||
|
||
bool is_allergic_to(Allergen_t allergen, unsigned int score); | ||
void get_allergens(int score, Allergen_List_t * list); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update this score
to be unsigned as well?
Looks good once |
Oops, missed that one... fixed |
LGTM |
See https://github.com/exercism/x-common/tree/master/exercises/allergies