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' #1387

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

'solution' #1387

wants to merge 4 commits into from

Conversation

julia4406
Copy link

No description provided.

app/main.py Outdated
Comment on lines 10 to 12
result = func(*args, **kwargs)
cached_data[parameters] = result
print("Calculating new result")

Choose a reason for hiding this comment

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

Avoid using additional variable, it is not needed here

app/main.py Outdated
cached_data = {}

def wrapper(*args, **kwargs) -> Callable:
parameters = tuple(args)

Choose a reason for hiding this comment

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

Are you sure you can't do without tuple?

@julia4406 julia4406 requested a review from petrykivd November 26, 2024 12:11
app/main.py Outdated
Comment on lines 7 to 13
def wrapper(*args) -> Callable:
if args not in storage:
print("Calculating new result")
return storage.setdefault(args, func(*args))
else:
print("Getting from cache")
return storage.get(args)

Choose a reason for hiding this comment

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

Try to rewrite wrapper not using a duplicated return, without duplicating it

else:
print("Getting from cache")
return storage.get(args)
return wrapper

Choose a reason for hiding this comment

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

Always add a blank line at the end of the file.

Copy link
Author

Choose a reason for hiding this comment

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

Yeh, sure, but it is there (flake8 always push me to do this)

app/main.py Outdated
storage = {}

def wrapper(*args) -> Callable:
if args not in storage:

Choose a reason for hiding this comment

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

In many cases , it is better not to use not, because we always thinking if something is in but not is not in. But a it's a minor thing, that can save you a 3 symbols xD
By the way, it's opinion from my side for this task, sometimes we really need to check not in because it looks better with variable names and task requirements.
At that task, if you swap you if code and else to avoid using not, nothing will change logically!

@julia4406 julia4406 requested a review from L1nk3rrr November 26, 2024 18:15
Copy link

@L1nk3rrr L1nk3rrr left a comment

Choose a reason for hiding this comment

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

That was your last try, so review my comment and accept it. Anyway you are doing great, that was nice to see that you used a setdefault method (for previous solution it makes sense!!), remember how it works and use it next time if it's only needed.
Well done!

print("Getting from cache")
else:
print("Calculating new result")
storage.setdefault(args, func(*args))

Choose a reason for hiding this comment

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

There is no more sense use setdefault method, because you are getting value at the end, we can simply set value by a key. Now it's doing one extra move - return you a value into nothing, so we don't need it.

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