Cebolwethu Mzanywa#6
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements solutions for six analytics functions in a Python assessment, along with comprehensive unit tests for the inventory management function.
Summary of Changes:
- Implementation of list/string filtering functions (
filter_sales_above_threshold,count_product_codes) - Implementation of data analysis functions (
calculate_moving_average,get_top_seller) - Implementation of dictionary manipulation (
merge_inventory) - Implementation of inventory status analysis (
check_inventory_status) - Addition of comprehensive test suite for inventory status functionality
Key Issues Identified:
- Multiple critical bugs in function implementations that will cause test failures
- Logic errors in
count_product_codes,calculate_moving_average,merge_inventory, andcheck_inventory_status - Spelling errors in variable names (
tresholdinstead ofthreshold) - Missing return statement in
check_inventory_status
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 19 comments.
| File | Description |
|---|---|
| test_inventory.py | New comprehensive test suite covering all scenarios for the inventory status function including invalid inputs, overstocked, critical, reorder, low stock, and optimal conditions |
| analytics.py | Implementation of six analytics functions with various data processing, filtering, and business logic capabilities |
| pycache/analytics.cpython-312.pyc | Compiled Python bytecode (auto-generated) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| above_treshold = [] | ||
| if len(sales) == 0: | ||
| return above_treshold | ||
| else: | ||
| for i in sales: | ||
| if i > threshold: | ||
| above_treshold.append(i) | ||
| return above_treshold |
There was a problem hiding this comment.
Typo in variable name: 'treshold' should be 'threshold'.
| above_treshold = [] | |
| if len(sales) == 0: | |
| return above_treshold | |
| else: | |
| for i in sales: | |
| if i > threshold: | |
| above_treshold.append(i) | |
| return above_treshold | |
| above_threshold = [] | |
| if len(sales) == 0: | |
| return above_threshold | |
| else: | |
| for i in sales: | |
| if i > threshold: | |
| above_threshold.append(i) | |
| return above_threshold |
| if warehouse_a and warehouse_b: | ||
| return new_warehouse | ||
| else: | ||
| for a in warehouse_a: | ||
| for b in warehouse_b: | ||
| if a == b: | ||
| new_warehouse[a] = warehouse_a[a] + warehouse_b[b] | ||
| else: | ||
| new_warehouse[a] = warehouse_a[a] | ||
| new_warehouse[b] = warehouse_b[b] |
There was a problem hiding this comment.
The nested loop logic is incorrect for merging dictionaries. This creates a Cartesian product of keys, leading to incorrect results. For example, products unique to one warehouse would be added multiple times. Use new_warehouse.update() or iterate through each warehouse separately to properly merge the inventories.
| if warehouse_a and warehouse_b: | |
| return new_warehouse | |
| else: | |
| for a in warehouse_a: | |
| for b in warehouse_b: | |
| if a == b: | |
| new_warehouse[a] = warehouse_a[a] + warehouse_b[b] | |
| else: | |
| new_warehouse[a] = warehouse_a[a] | |
| new_warehouse[b] = warehouse_b[b] | |
| if not warehouse_a and not warehouse_b: | |
| return new_warehouse | |
| else: | |
| all_keys = set(warehouse_a.keys()) | set(warehouse_b.keys()) | |
| for key in all_keys: | |
| new_warehouse[key] = warehouse_a.get(key, 0) + warehouse_b.get(key, 0) |
| return F"LOW STOCK" | ||
| else: | ||
| return F"OPTIMAL" |
There was a problem hiding this comment.
Inconsistent string formatting. Use lowercase 'f' for f-strings consistently throughout the file (other return statements use lowercase 'f').
| return F"LOW STOCK" | |
| else: | |
| return F"OPTIMAL" | |
| return f"LOW STOCK" | |
| else: | |
| return f"OPTIMAL" |
| above_treshold = [] | ||
| if len(sales) == 0: | ||
| return above_treshold | ||
| else: | ||
| for i in sales: | ||
| if i > threshold: | ||
| above_treshold.append(i) | ||
| return above_treshold |
There was a problem hiding this comment.
Typo in variable name: 'treshold' should be 'threshold'. This misspelling is used throughout the function.
| above_treshold = [] | |
| if len(sales) == 0: | |
| return above_treshold | |
| else: | |
| for i in sales: | |
| if i > threshold: | |
| above_treshold.append(i) | |
| return above_treshold | |
| above_threshold = [] | |
| if len(sales) == 0: | |
| return above_threshold | |
| else: | |
| for i in sales: | |
| if i > threshold: | |
| above_threshold.append(i) | |
| return above_threshold |
| above_treshold = [] | ||
| if len(sales) == 0: | ||
| return above_treshold | ||
| else: | ||
| for i in sales: | ||
| if i > threshold: | ||
| above_treshold.append(i) | ||
| return above_treshold |
There was a problem hiding this comment.
Typo in variable name: 'treshold' should be 'threshold'.
| above_treshold = [] | |
| if len(sales) == 0: | |
| return above_treshold | |
| else: | |
| for i in sales: | |
| if i > threshold: | |
| above_treshold.append(i) | |
| return above_treshold | |
| above_threshold = [] | |
| if len(sales) == 0: | |
| return above_threshold | |
| else: | |
| for i in sales: | |
| if i > threshold: | |
| above_threshold.append(i) | |
| return above_threshold |
| count = 0 | ||
| if len(numbers) == 0: | ||
| return 0.0 | ||
| elif len(numbers) >= 3: |
There was a problem hiding this comment.
The hardcoded check len(numbers) >= 3 is incorrect. The function should work with any window_size, not just 3. This condition should check against window_size instead.
| elif len(numbers) >= 3: | |
| elif len(numbers) >= window_size: |
| count += numbers[i] | ||
| average = count/window_size | ||
| return average | ||
| else: | ||
| for i in numbers: | ||
| count += i | ||
| average = count/len(numbers) |
There was a problem hiding this comment.
The variable average is being recalculated on every iteration of the loop instead of once after the loop completes. Move line 70 outside the loop (dedent it to align with the return statement).
| count += numbers[i] | |
| average = count/window_size | |
| return average | |
| else: | |
| for i in numbers: | |
| count += i | |
| average = count/len(numbers) | |
| count += numbers[i] | |
| average = count / window_size | |
| return average | |
| else: | |
| for i in numbers: | |
| count += i | |
| average = count / len(numbers) |
| pass | ||
| tie = [] | ||
| if len(sales_data) == 0: | ||
| return f"No Data" |
There was a problem hiding this comment.
Unnecessary f-string formatting. The return value is just a plain string with no variables, so use "No Data" instead of f"No Data".
| return f"No Data" | |
| return "No Data" |
| return f"Invalid Input" | ||
| elif stock_level > max_capacity: | ||
| return f"Invalid Input" | ||
| elif stock_level > (max_capacity * 0.9): | ||
| f"OVERSTOCKED" | ||
| elif stock_level < (reorder_point*0.5): | ||
| return f"CRITICAL" | ||
| elif stock_level <= reorder_point: | ||
| return f"REORDER" | ||
| elif (daily_sales > 0) and (stock_level < 7 or daily_sales < 7): | ||
| return F"LOW STOCK" | ||
| else: | ||
| return F"OPTIMAL" |
There was a problem hiding this comment.
Unnecessary f-string formatting. The return value is just a plain string with no variables, so use "REORDER" instead of f"REORDER".
| return f"Invalid Input" | |
| elif stock_level > max_capacity: | |
| return f"Invalid Input" | |
| elif stock_level > (max_capacity * 0.9): | |
| f"OVERSTOCKED" | |
| elif stock_level < (reorder_point*0.5): | |
| return f"CRITICAL" | |
| elif stock_level <= reorder_point: | |
| return f"REORDER" | |
| elif (daily_sales > 0) and (stock_level < 7 or daily_sales < 7): | |
| return F"LOW STOCK" | |
| else: | |
| return F"OPTIMAL" | |
| return "Invalid Input" | |
| elif stock_level > max_capacity: | |
| return "Invalid Input" | |
| elif stock_level > (max_capacity * 0.9): | |
| return "OVERSTOCKED" | |
| elif stock_level < (reorder_point*0.5): | |
| return "CRITICAL" | |
| elif stock_level <= reorder_point: | |
| return "REORDER" | |
| elif (daily_sales > 0) and (stock_level < 7 or daily_sales < 7): | |
| return "LOW STOCK" | |
| else: | |
| return "OPTIMAL" |
| return F"LOW STOCK" | ||
| else: | ||
| return F"OPTIMAL" |
There was a problem hiding this comment.
Inconsistent string formatting. Use lowercase 'f' for f-strings consistently throughout the file (lines 183, 185, 189, 191 use lowercase 'f', but this line uses uppercase 'F').
| return F"LOW STOCK" | |
| else: | |
| return F"OPTIMAL" | |
| return f"LOW STOCK" | |
| else: | |
| return f"OPTIMAL" |
No description provided.