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

Simplified version of your look_w function #1

Merged
merged 3 commits into from
May 2, 2018

Conversation

Stack-of-Pancakes
Copy link

Saw your pull request: dwyl#38

What you were doing, finding words that could be made from word. It was a bit convoluted and I just wanted to show you it was basically a one-liner. All you had to do was check that the length was correct, and that the number of each letter being used was available in the source word.

Also this version only modifies the dictionary words when checking so when they are returned they maintain their case. You can see that in the 'quit' example.

Stack-of-Pancakes added 3 commits April 29, 2018 14:46
This reverts commit 1bf0341.
Saw your pull request: dwyl#38
Wanted to show you there was a simpler way to accomplish what you
were doing.  Also this solution returns words in the original case.
sys imported and unused.
os only used to create cwd which is implied with open
try/except block that doesn't handle anything
json loaded to create a more expensive defaultdict
dict created just for membership testing instead of set
membership test performed by fetching key value instead of using in
@kakkarja
Copy link
Owner

kakkarja commented May 1, 2018

Hi Stack-of-Pancakes,

Thank you so much for the pull request. I am pretty amaze with the solutions you gave. I almost wanted to merge at once. However, i was taught that "one-liner" is more complex to understand. I did spend more time to understand it.

Yesss, i agree that the existing one was too complex as well. But it made more clearer through comments.

If you could just made changes on the comments, it will be much appreciated. I prefer that the solution will be made new (instead of changing the existing one). I am willing to merge your new approach and deleted the existing one instead (after some review approval).

Thank you again, and hope you enjoy the rest of the repos as well. You are most welcome to suggest at the others as well. Wait for more pull request from you 😄

Copy link
Owner

@kakkarja kakkarja left a comment

Choose a reason for hiding this comment

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

I will still keep few of the existing one that you have deleted. Thank you for your support and idea.

@kakkarja kakkarja merged commit 13ef74f into kakkarja:master May 2, 2018
Repository owner locked as resolved and limited conversation to collaborators May 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants