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

Develop #414

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

mykolamateichuk
Copy link

No description provided.

@Naz-iv
Copy link

Naz-iv commented Oct 24, 2023

final submit has failed git auto check, please fix

Copy link

@Naz-iv Naz-iv left a comment

Choose a reason for hiding this comment

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

test failed, please fix

Comment on lines +13 to +17
def from_dict(cls_dict: dict) -> Car:
new_car = Car()
for key in cls_dict:
setattr(new_car, key, cls_dict[key])
return new_car

Choose a reason for hiding this comment

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

To create instances you can just use ** to unpack and list comprehension. It will look much more readable, and you won't need to create FromDict class. Also think about method and parameter names to increase readability
And this method better to be classmethod, not static

Choose a reason for hiding this comment

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

Same everywhere

Comment on lines +7 to +8
brand: str = "",
fuel_consumption: int | float = 0.0) -> None:

Choose a reason for hiding this comment

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

Do you really want to accept creating cars with "" brand and 0.0 fuel_consumption?
Skip adding default values here, fix everywhere

Comment on lines +10 to +13
customers = [Customer.from_dict(customer)
for customer in config["customers"]]
shops = [Shop.from_dict(shop)
for shop in config["shops"]]

Choose a reason for hiding this comment

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

You can just use unpacking, that I mentioned in previoius comments

util.calculate_trip_cost(fuel_price,
customer,
shops[closest_shop]),
2

Choose a reason for hiding this comment

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

this 2 looks really lonely here (

Comment on lines +6 to +48
def shop_trip() -> None:
config = util.parse_json("app/config.json")

fuel_price = config["FUEL_PRICE"]
customers = [Customer.from_dict(customer)
for customer in config["customers"]]
shops = [Shop.from_dict(shop)
for shop in config["shops"]]

for customer in customers:
print(f"{customer.name} has {customer.money} dollars")

closest_shop = 0
min_cost = 100
for index, shop in enumerate(shops):
trip_cost = round(
util.calculate_trip_cost(fuel_price, customer, shop),
2
)

print(f"{customer.name}'s trip to the"
f" {shop.name} costs {trip_cost}")

if trip_cost < min_cost:
min_cost = trip_cost
closest_shop = index

if customer.money < min_cost:
print(f"{customer.name} "
f"doesn't have enough money to make a purchase in any shop")
continue

print(f"{customer.name} rides to {shops[closest_shop].name}\n")
shops[closest_shop].print_receipt(customer)
customer.money -= round(
util.calculate_trip_cost(fuel_price,
customer,
shops[closest_shop]),
2
)

print(f"{customer.name} rides home")
print(f"{customer.name} now has {customer.money} dollars\n")

Choose a reason for hiding this comment

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

Consider split into methods

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants