ver 1 #641
ver 1 #641
Conversation
There was a problem hiding this comment.
Great work on the code! 🎉 There are a few areas that could be improved for better performance and readability, but these do not significantly impact the functionality of your code. Keep these suggestions in mind for future projects. Keep it up! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| @@ -0,0 +1,4 @@ | |||
| import json | |||
|
|
|||
| with open("config.json") as file: | |||
There was a problem hiding this comment.
You should write the full path to the JSON file. It's a good practice to specify the full path to the file to avoid confusion and potential errors. You can use the os module to get the path of the current script and then construct the full path using os.path.join(). Here is an example:
python
import os
script_dir = os.path.dirname(file)
file_path = os.path.join(script_dir, 'config.json')
with open(file_path) as file:
config = json.load(file)
This way, no matter where you run your script from, it will always find the `config.json` file as long as it is in the same directory as your script.
| for product, quantity in self.product_cart.items(): | ||
| cost += shop.products[product] * quantity | ||
| return cost |
There was a problem hiding this comment.
This is a code quality issue. You are calculating the product cost twice: once in the calculate_product_cost method and once in the buy_products method. It would be more efficient to calculate it once and store the result in a variable that can be used in both methods.
|
|
||
| def ride_home(self) -> None: | ||
| print(f"\n{self.name} rides home") | ||
| self.location = [12, -2] |
There was a problem hiding this comment.
This is a code quality issue. The customer's home location is hardcoded in the ride_home method. It would be better to store the home location as an attribute of the Customer class and use it in the ride_home method. This would make the code more flexible and maintainable.
| for customer in config["customers"]: | ||
| car = Car(customer["car"]["brand"], | ||
| customer["car"]["fuel_consumption"]) | ||
| customers.append(Customer(customer["name"], | ||
| customer["product_cart"], | ||
| customer["location"], | ||
| customer["money"], car)) |
There was a problem hiding this comment.
DRY principle: Instead of writing a loop to create a Customer object for each customer in the config['customers'] list and then appending it to the customers list, you could use a list comprehension. This would make your code more concise and pythonic.
| customer.ride_to_shop(next(shop | ||
| for shop in shops | ||
| if shop.name == cheapest_shop)) | ||
| customer.buy_products(next(shop | ||
| for shop in shops | ||
| if shop.name == cheapest_shop)) |
There was a problem hiding this comment.
Performance issue: In these lines, you're using a generator expression inside the next() function to find the shop with the name cheapest_shop. This operation has a time complexity of O(n), which means that the time it takes to execute increases linearly with the number of shops. Since you're doing this twice (once for ride_to_shop and once for buy_products), the time complexity is effectively O(2n). A more efficient way would be to find the shop once and store it in a variable, and then use that variable in the ride_to_shop and buy_products methods. This would reduce the time complexity to O(n).
| for customer in customers: | ||
| print(f"{customer.name} has {customer.money} dollars") | ||
| trip_costs = {} | ||
| for shop in shops: | ||
| trip_costs[shop.name] = customer.calculate_trip_cost(shop) | ||
| print(f"{customer.name}'s trip to the " | ||
| f"{shop.name} costs {trip_costs[shop.name]:.2f}") | ||
| cheapest_shop = min(trip_costs, key=trip_costs.get) | ||
| if customer.money >= trip_costs[cheapest_shop]: | ||
| customer.ride_to_shop(next(shop | ||
| for shop in shops | ||
| if shop.name == cheapest_shop)) | ||
| customer.buy_products(next(shop | ||
| for shop in shops | ||
| if shop.name == cheapest_shop)) | ||
| customer.ride_home() | ||
| print(f"{customer.name} now has {customer.money:.2f} dollars\n") | ||
| else: | ||
| print(f"{customer.name} doesn't have enough money " | ||
| f"to make a purchase in any shop") |
There was a problem hiding this comment.
Code style issue: The function shop_trip is quite long and does a lot of different things. This makes it harder to read and understand. Consider splitting it into smaller functions, each with a single responsibility. For example, you could have separate functions for creating customers and shops, calculating trip costs, and performing the shopping trip. This would make your code more modular and easier to maintain.
No description provided.