Skip to content
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

feature/#191 - added and computed possible product improvements #217

Merged
merged 3 commits into from
Sep 16, 2021

Conversation

monsieurtanuki
Copy link
Contributor

Impacted files:

  • api_getProduct_test.dart: added checks for getProductImprovements
  • Product.dart: create enum ProductImprovement and Product.getProductImprovements()

…vements

Impacted files:
* `api_getProduct_test.dart`: added checks for `getProductImprovements`
* `Product.dart`: create `enum ProductImprovement` and `Product.getProductImprovements()`
@teolemon
Copy link
Member

Is the scope the implementation of the Open Food Facts states ?

/// when displaying a [Product].
Set<ProductImprovement> getProductImprovements() {
final Set<ProductImprovement> result = {};
if (statesTags != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor but I'd avoid nested if structures like this, if possible. You could change it by adding something like

if (statestags == null) {
    return result;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @peterwvj for your comment.
I agree with you, I'm a big fan myself of return, break or continue statements instead of elses (nested or not).
The thing is that I also had in mind #193, that will impact the very same method.
When I implement #193 (next week), I'll take your remark into account if still relevant.

@monsieurtanuki
Copy link
Contributor Author

Is the scope the implementation of the Open Food Facts states ?

@teolemon If the question is for me, could you please elaborate on it?

@teolemon
Copy link
Member

So far, the PR is implementing some of the available states (https://world.openfoodfacts.org/states, https://github.com/openfoodfacts/openfoodfacts-server/blob/main/taxonomies/states.txt)
So perhaps I would name them that way ?

Copy link
Member

@M123-dev M123-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterwvj peterwvj mentioned this pull request Sep 15, 2021
@monsieurtanuki monsieurtanuki merged commit 89c4e12 into openfoodfacts:master Sep 16, 2021
@monsieurtanuki monsieurtanuki deleted the feature/#191 branch September 17, 2021 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants