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

Solution #928

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

Solution #928

wants to merge 5 commits into from

Conversation

PythonZem
Copy link

No description provided.

Copy link

@juliastetsko juliastetsko left a comment

Choose a reason for hiding this comment

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

in genaral looks good

app/main.py Outdated
def __repr__(self) -> str:
return f"Distance(km={self.km})"

def __add__(self, other: Distance) -> Distance:

Choose a reason for hiding this comment

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

you can add | float to type annotations

app/main.py Outdated
return f"Distance(km={self.km})"

def __add__(self, other: Distance) -> Distance:
if isinstance(other, Distance):

Choose a reason for hiding this comment

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

this code duplicates several times

Choose a reason for hiding this comment

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

agree, try to remove such duplication

Copy link
Author

Choose a reason for hiding this comment

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

Yp, I created new static method for checking the type other. To make my code shorted

app/main.py Outdated
return f"Distance(km={self.km})"

def __add__(self, other: Distance) -> Distance:
if isinstance(other, Distance):

Choose a reason for hiding this comment

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

agree, try to remove such duplication

app/main.py Outdated
class Distance:
# Write your code here
pass
def __init__(self, km: float) -> None:

Choose a reason for hiding this comment

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

I think km could be int as well

Copy link
Author

Choose a reason for hiding this comment

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

I think user can input Float or Int types

app/main.py Outdated
def __repr__(self) -> str:
return f"Distance(km={self.km})"

def __add__(self, other: Distance) -> Distance:

Choose a reason for hiding this comment

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

Is other only Distance instance?)

Copy link
Author

Choose a reason for hiding this comment

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

add any types

app/main.py Outdated
Comment on lines 46 to 54
def __le__(self, other: Distance) -> bool:
if isinstance(other, Distance):
other = other.km
return self.km <= other

def __ge__(self, other: Distance) -> bool:
if isinstance(other, Distance):
other = other.km
return self.km >= other

Choose a reason for hiding this comment

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

You can simplify these methods with not + previously implemented operators.

Copy link
Author

Choose a reason for hiding this comment

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

It's ingeniously simple

app/main.py Outdated
Comment on lines 30 to 46
def __truediv__(self, other: float | int) -> Distance:
return Distance(km=round(self.km / other, 2))

def __lt__(self, other: Distance | float) -> bool:
return self.km < self.convert_obj_number(obj=other)

def __gt__(self, other: Distance | float) -> bool:
return self.km > self.convert_obj_number(obj=other)

def __eq__(self, other: Distance | float) -> bool:
return self.km == self.convert_obj_number(obj=other)

def __le__(self, other: Distance | float) -> bool:
return not self.__gt__(other=other)

def __ge__(self, other: Distance | float) -> bool:
return not self.__lt__(other=other)

Choose a reason for hiding this comment

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

suggestion:
Distance | float | int

Copy link
Author

Choose a reason for hiding this comment

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

Sure. Done!

app/main.py Outdated
Comment on lines 23 to 27
def __iadd__(self, other: Distance | float) -> Distance:
self.km += self.convert_obj_number(obj=other)
return self

def __mul__(self, other: float) -> Distance:

Choose a reason for hiding this comment

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

you can't add int here?)

Copy link
Author

Choose a reason for hiding this comment

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

I can)

app/main.py Outdated
Comment on lines 15 to 18
def convert_obj_number(obj: Distance | float | int) -> float | int:
if isinstance(obj, Distance):
obj = obj.km
return obj

Choose a reason for hiding this comment

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

That's pretty strange naming, I mean even if func name is convert_obj_to_number, why then you return obj, not simple obj.km?)

Copy link
Author

Choose a reason for hiding this comment

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

I too had doubts about this function name)

@PythonZem PythonZem requested a review from NEkropo1 January 11, 2024 11:08
Copy link

@NEkropo1 NEkropo1 left a comment

Choose a reason for hiding this comment

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

Good job!

Comment on lines +15 to +16
def get_km(obj: Distance | float | int) -> float | int:
return obj.km if isinstance(obj, Distance) else obj

Choose a reason for hiding this comment

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

great!

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.

4 participants