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

adding list comprehensions #5

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

adding list comprehensions #5

wants to merge 2 commits into from

Conversation

moonpatroller
Copy link

No description provided.

@Frimkron
Copy link
Owner

@moonpatroller Hi! Thanks for the contribution :) The list comprehensions certainly make the code more concise (and probably perform better), however I intentionally left them as verbose nested ifs as I thought this might be simpler for beginners to follow. Maybe there's a sweet spot in the middle somewhere? What do you think? :)

@wowpin
Copy link

wowpin commented Aug 14, 2018

Hi! Speaking from the perspective of a person who used this piece of code as a basis of my own project, I certainly appreciate the code simplicity. It helps you grasp basic concepts, especially if someone without Python experience tries to get their teeth stuck in.

In this particular case, I would probably prioritize code simplicity over performance and sophistication - although I will likely be incorporating proposed changes into my own code :)

Best regards

@moonpatroller
Copy link
Author

@Frimkron I doubt they make it perform any better. I think beginners have to learn at some point. I see the argument for not constructing complicated lc's, and maybe some or all of these are on the more complicated side. I think I'd be excited to see them when I was learning, but I know lots of students go apeshit over everything, too. This also doesn't seem like a super beginner project. Do as you will. :)

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