Solution? #553
Solution? #553
Conversation
app/main.py
Outdated
| # date_now = datetime.datetime.strftime(datetime.datetime.now(), | ||
| # "%d/%m/%Y %H:%M:%S") | ||
| # fake_date = datetime.datetime.strftime(datetime.datetime( | ||
| # 2021, 1, 4, 12, 33, 41 | ||
| # ), "%d/%m/%Y %H:%M:%S") | ||
|
|
||
| # print(f"Date: 04/01/2021 12:33:41") - PASSES | ||
| # print(f"Date: {date_now}") - FAILS | ||
| # print(f"Date: {fake_date}") - FAILS | ||
| # I already tested "04/01/2021 12:33:41" == fake_date: | ||
| # is "TRUE" I don't understand why even fake time is failing | ||
| # only hardcoded time passes | ||
| print("Date: 04/01/2021 12:33:41") |
There was a problem hiding this comment.
If you look here, I tried to use datetime.now(), I tried to fake datetime with fake_date the exact data it expects but only way tests were accepting the print statement were just hardcoded print(f"Date: 04/01/2021 12:33:41"). I don't know if it's normal but after 30 minutes of testing, I decided to check other student pull request and saw the same thing.
app/car.py
Outdated
| def load_fuel_price_from_json(filepath: str) -> float: | ||
| with open(filepath, "r") as file: | ||
| data = json.load(file) | ||
| return data.get("FUEL_PRICE") | ||
|
|
There was a problem hiding this comment.
No need to create three separate functions with similar logic, DRY
app/main.py
Outdated
| def shop_trip() -> None: | ||
| customers = customers_ | ||
| shops = shops_ | ||
| fuel_price = FUEL_PRICE | ||
|
|
||
| for customer in customers: | ||
| cheapest_shop = [] | ||
| print(f"{customer.name} has {customer.money} dollars") | ||
| for shop in shops: | ||
| total_price = 0 | ||
| for (_, value_need_customer), (_, shop_item_price) in zip( | ||
| customer.product_cart.items(), shop.products.items() | ||
| ): | ||
| total_price += value_need_customer * shop_item_price | ||
| # our road distance is calculated hypotenuse, | ||
| # I'd argue about it, cars don't drive strait lines :) | ||
| one_way_distance = sqrt( | ||
| (customer.location[0] - shop.location[0]) ** 2 | ||
| + (customer.location[1] - shop.location[1]) ** 2 | ||
| ) | ||
| total_price += round(customer.car.fuel_consumption | ||
| * fuel_price / 100 * one_way_distance * 2, 2) | ||
|
|
||
| # save every shop to chose from | ||
| cheapest_shop.append([total_price, shop]) | ||
| print(f"{customer.name}'s trip to the {shop.name} " | ||
| f"costs {total_price}") | ||
|
|
||
| cheapest_shop = list(sorted(cheapest_shop)).pop(0) |
There was a problem hiding this comment.
Split shop_trip function logic into smaller functions, SRP
app/main.py
Outdated
| print(f"{customer.name} has {customer.money} dollars") | ||
| for shop in shops: | ||
| total_price = 0 | ||
| for (_, value_need_customer), (_, shop_item_price) in zip( |
There was a problem hiding this comment.
Change variables names, they are not really descriptive
I don't really understand what value_need_customer means.
You can also change shop_item_price to product_price, since it means the same and we use product naming here
app/main.py
Outdated
| fuel_price = FUEL_PRICE | ||
|
|
||
| for customer in customers: | ||
| cheapest_shop = [] |
There was a problem hiding this comment.
Fix variable name. You store every shop info here, not just one cheapest_shop, as your variable name says
app/main.py
Outdated
| * fuel_price / 100 * one_way_distance * 2, 2) | ||
|
|
||
| # save every shop to chose from | ||
| cheapest_shop.append([total_price, shop]) |
There was a problem hiding this comment.
Why do u use list, not a dict here?
app/main.py
Outdated
| f"money to make a purchase in any shop") | ||
| continue | ||
|
|
||
| print("Date: 04/01/2021 12:33:41") |
There was a problem hiding this comment.
I mean, my way might not be the best but when I tried to use datetime.datetime.strftime(datetime.datetime.now(), "%d/%m/%Y %H:%M:%S") test were giving me some error that I didn't understand how to handle because I receive same string with this code. But on the other hand - datetime.datetime.now().strftime("%d/%m/%Y %H:%M:%S") this line of code gives absolutely same result, and works..
There was a problem hiding this comment.
well datetime.now() should work remove hardcoded string please
There was a problem hiding this comment.
well datetime.now() should work remove hardcoded string please
already did, there are lots of work to fix so I'll probably commit everything only on weekend
|
|
||
|
|
||
| def shop_trip() -> None: | ||
| data = load_complete_json("app/config.json") |
There was a problem hiding this comment.
I have general questions about the structure of your code:
- The whole OOP concept in application is about creating classes with methods bound to them. So here you create classes but you don't bound any methods to them. IF you did your code would be much cleaner and well structured.
- This function is way too long most of the logic can be rewritten as methods that you will call on your classes. Example:
You could put most of this logic as methods in class Customer - methods such as calculate_shop_trip_cost, get_customer_budget, buy_products etc and then in your main function you just call customer.calculate_shop_trip_cost and it will make code so much cleaner. - On the same note you can add new classes such as Location and put logic of calcualting distance between two locations there and then reuse it in other code. Or you could define a method in Car to calculate the cost of fuel for the ride.
Think how you can refactor your code so that your final shop_trip() function has about 10 lines of code max - the rest should be handled simply by calling one or two methods on customer instance and the rest should happen under the hood.
Let me know if you have questions
There was a problem hiding this comment.
the hardest part is to refactor the code after you already wrote it and it's all over the modules, so you need to tear it appart again :D
|
Well, I hope it's at least got close to be the way it should be. Because I would be disappointed if it's not even moving the right way, feels like I've already spent at least 5-6 hours in total to just fix the code. |
Will check tomorrow. But even if there will be some improvements to be made it is still good that you try different approaches on same problem. This way you will learn much better |
app/main.py
Outdated
| shops = [Shop(**shop) for shop in data.get("shops")] | ||
| fuel_price = data.get("FUEL_PRICE") | ||
|
|
||
| for customer in customers: |
There was a problem hiding this comment.
You can optimize by removing line 8 customers = [Customer(**customer_) for customer_ in data.get("customers")]
and then you can create customer instances here and call .choose_the_cheapest_shop_visit(customer, shops, fuel_price) on newly created instance
app/customer.py
Outdated
| one_way_distance = sqrt( | ||
| (self.location[0] - shop.location[0]) ** 2 | ||
| + (self.location[1] - shop.location[1]) ** 2 | ||
| ) | ||
| total_price += round(self.car.fuel_consumption | ||
| * fuel_price / 100 * one_way_distance * 2, 2) |
There was a problem hiding this comment.
i think this could be moved to separate method to structure code better
app/customer.py
Outdated
| def choose_the_cheapest_shop_visit(self, | ||
| shops: list[Shop], | ||
| fuel_price: float) -> None: | ||
| best_price_and_shop = defaultdict() |
There was a problem hiding this comment.
i think you don't really need a dict here - you can just have to variables:
best_shop_visit_price
best_shop_instance
and set some initial value to it here
|
|
||
| print(f"Total cost is {cart_price} dollars\nSee you again!\n") | ||
|
|
||
| def _checkout_for_products(self, shop: Shop) -> tuple[list[str], float]: |
There was a problem hiding this comment.
Your Customer class has a lot of methods some of which could be moved to a different class. Not a mistake it's just a not so good practice to have one class that does it all and the others do nothing. Her for example you could move interaction with cashier to the shop because it doesn't really have to do anything with the customer
There was a problem hiding this comment.
was figthing with circular imports, so decided to do it all in customer :(
|
|
||
| for customer_data in data.get("customers"): | ||
| customer = Customer(**customer_data) | ||
| best_price, best_shop = customer.choose_the_cheapest_shop_visit( |
There was a problem hiding this comment.
here you could move all those methods calls on customer to a separate method (e.g. complete_shop_trip ) and call these methods in a chain inside of the class. Here you would just call complete_shop_trip on Customer and thats it.
This way you will cleanup this function
No description provided.