Release #573
Release #573
Conversation
feat: Init v1 ingredients
|
|
Reviewer's Guide by SourceryThis PR introduces a new version (v1) of the ingredients API while maintaining the existing v0 endpoint. The main changes include a restructured project layout following a modular approach, improved ingredient matching logic, and updated vegan status determination rules. The PR also includes updates to the E-number and ingredient classification lists. Architecture diagram for Ingredients Modulegraph TD;
AppModule --> IngredientsModule
IngredientsModule --> IngredientsController
IngredientsModule --> IngredientsV1Controller
IngredientsModule --> TranslationService
IngredientsController -->|uses| TranslationService
IngredientsV1Controller -->|uses| TranslationService
Class diagram for Ingredients API versioningclassDiagram
class IngredientsController {
+onModuleInit()
+getIngredients(ingredientsParam: string, res: Response, translateFlag: boolean)
-sendResponse(res: Response, notVeganItems: string[], unknownItems: string[])
}
class IngredientsV1Controller {
+onModuleInit()
+getIngredients(ingredientsParam: string, res: Response, translateFlag: boolean)
-sendResponse(res: Response, notVeganItems: string[], maybeNotVeganItems: string[], unknownItems: string[])
}
class V0ResponseData {
+boolean vegan
+string[] surely_vegan
+string[] not_vegan
+string[] maybe_vegan
}
class V1ResponseData {
+boolean vegan
+string[] surely_vegan
+string[] not_vegan
+string[] maybe_not_vegan
+string[] unknown
}
IngredientsController --> V0ResponseData
IngredientsV1Controller --> V1ResponseData
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @philipbrembeck - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding documentation explaining the rationale behind the E-number removals and ingredient additions in the JSON files
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| private sophisticatedMatch(ingredient: string, list: string[]): boolean { | ||
| const normalizedIngredient = ingredient.toLowerCase().replace(/\s+/g, ""); |
There was a problem hiding this comment.
suggestion (performance): Consider caching compiled regular expressions for better performance
Creating RegExp objects repeatedly in sophisticatedMatch() is expensive. Consider pre-compiling the word boundary regex pattern at class initialization and reusing it.
| const normalizedIngredient = ingredient.toLowerCase().replace(/\s+/g, ""); | |
| const WHITESPACE_REGEX = /\s+/g; | |
| const normalizedIngredient = ingredient.toLowerCase().replace(WHITESPACE_REGEX, ""); |
| ingredients.join(","), | ||
| "EN", | ||
| 1500 | ||
| ); |
There was a problem hiding this comment.
issue (bug_risk): Relying on array order preservation through translation is risky
The back-translation logic assumes the translated items will maintain their order. Consider adding identifiers or using a more robust mapping approach to match translated items back to their categories.
| @@ -136,7 +136,7 @@ describe("IngredientsController", () => { | |||
| status: "200", | |||
| message: "Success", | |||
| data: { | |||
| vegan: true, | |||
| vegan: false, | |||
There was a problem hiding this comment.
issue (testing): Test case needs to be updated to reflect new vegan status logic
The test case was updated to change vegan from true to false, but it's not clear why this change was made. The test should verify the new logic where vegan is false when there are unknown items (as implemented in V0ResponseData). Consider adding a comment explaining this behavior change and additional test cases to cover different combinations of not_vegan and unknown items.
| @@ -136,7 +136,7 @@ | |||
| status: "200", | |||
| message: "Success", | |||
| data: { | |||
| vegan: true, | |||
| vegan: false, | |||
| surely_vegan: [], | |||
| not_vegan: [], | |||
| maybe_vegan: ["soy milk", "egg white"], | |||
There was a problem hiding this comment.
issue (testing): Test data contains contradictory ingredients
The test data includes 'egg white' in maybe_vegan, which is typically considered not vegan. This could be misleading. Consider using more appropriate test data that better reflects real-world scenarios and doesn't contradict common vegan classifications.
| } | ||
|
|
||
| // Use sophisticatedMatch for categorizing ingredients | ||
| let notVeganResult = response.filter((item: string) => |
There was a problem hiding this comment.
issue (complexity): Consider consolidating ingredient categorization into a single pass operation with dedicated translation handling
The ingredient categorization can be simplified significantly while maintaining all functionality. Here's how to reduce complexity:
- Replace the multiple filter passes with a single pass that categorizes ingredients:
private categorizeIngredients(response: string[]) {
const categories = {
notVegan: [] as string[],
maybeNotVegan: [] as string[],
vegan: [] as string[],
unknown: [] as string[]
};
response.forEach(item => {
if (this.sophisticatedMatch(item, this.isNotVegan)) {
categories.notVegan.push(item);
} else if (this.sophisticatedMatch(item, this.isMaybeNotVegan)) {
categories.maybeNotVegan.push(item);
} else if (this.sophisticatedMatch(item, this.isVegan)) {
categories.vegan.push(item);
} else {
categories.unknown.push(item);
}
});
return categories;
}- Extract translation logic to a dedicated service method:
// In TranslationService
async translateWithBackTranslation(
text: string,
targetLang: DeeplLanguages
): Promise<string> {
const forward = await this.translateText(text, "EN", 1500);
if (targetLang === "EN") {
return forward.data.translations[0].text;
}
const backward = await this.translateText(
forward.data.translations[0].text,
targetLang,
1500
);
return backward.data.translations[0].text;
}These changes will:
- Eliminate redundant iterations over the ingredient list
- Reduce cognitive complexity by centralizing categorization logic
- Improve maintainability by separating translation concerns
- Maintain all existing functionality and error handling
| private sophisticatedMatch(ingredient: string, list: string[]): boolean { | ||
| const normalizedIngredient = ingredient.toLowerCase().replace(/\s+/g, ""); | ||
|
|
||
| if (list.includes(normalizedIngredient)) return true; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (list.includes(normalizedIngredient)) return true; | |
| if (list.includes(normalizedIngredient)) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| if (list.some((item) => wordBoundaryRegex.test(item.replace(/\s+/g, "")))) | ||
| return true; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (list.some((item) => wordBoundaryRegex.test(item.replace(/\s+/g, "")))) | |
| return true; | |
| if (list.some((item) => wordBoundaryRegex.test(item.replace(/\s+/g, "")))) { | |
| return true; | |
| } | |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
Summary by Sourcery
Add a new API endpoint for product details retrieval and introduce version 1 of the ingredients API with improved functionality. Refactor the existing ingredients controller to utilize shared services and update the response structure for better accuracy. Update tests to align with the new logic.
New Features:
Enhancements:
Tests: