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

dict-and-join simplify for raindrops approaches #3620

Merged
merged 2 commits into from
Feb 6, 2024

Conversation

Nerwosolek
Copy link
Contributor

@Nerwosolek Nerwosolek commented Feb 5, 2024

Probably this was done on purpose but as a way to discuss this I wanted to propose a little simplification to the approach.
I think this is more sound and readable to just use the item from the dict.
Let me know what you think.
Thanks in advance.

@BethanyG
Copy link
Member

BethanyG commented Feb 5, 2024

Hi there! 👋🏽

Thanks for submitting this PR. 😄

Probably this was done on purpose but as a way to discuss this I wanted to propose a little simplification to the approach.
I think this is more sound and readable to just use the item from the dict.

I did do it on purpose, but my 'copypasta' got the better of me! It is more sound when using dict.items() to have the dictionary view unpack both the keys and the values (and then use them!). And that was my intent here -- to show an approach that was different looking, but the same as what is in the introduction.md summary:

def convert(number):
    #This is clear and easily added to.  Unless the factors get
    # really long, this won't take up too much memory.
    sounds = {3: 'Pling',
              5: 'Plang', 
              7: 'Plong'}

    results = (sounds[divisor] for 
               divisor in sounds.keys()
               if number % divisor == 0)

    return ''.join(results) or str(number)

Buuut. I copied the code above and missed modifying sounds[divisor]! 😱

Now that I am looking at all this again, I think perhaps the best thing to do is use the example above in the approach document, and use the code with dict.items() in the introduction.md. I'll update this PR with those changes, and you can let me know if it reads better to you that way.

Swapped the two dictionary approaches and adjusted `introduction.md`, `Benchmark.py`, `Performance`, and the approaches doc accordingly.
@Nerwosolek
Copy link
Contributor Author

Yes, looks even better. The distinguish between using items() and keys() adds even more knowledge to the already rich article!

Copy link
Member

@BethanyG BethanyG left a comment

Choose a reason for hiding this comment

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

Thanks for giving this one last read-through! 🌈

@BethanyG
Copy link
Member

BethanyG commented Feb 6, 2024

Yes, looks even better. The distinguish between using items() and keys() adds even more knowledge to the already rich article!

Going to push the big, green button then!

@BethanyG BethanyG merged commit 51f3798 into exercism:main Feb 6, 2024
8 checks passed
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.

2 participants