Easier solution #14
Easier solution #14
Conversation
I found the solutions proposed a bit difficult to read. I think this one is just a direct translation of the instructions and will be easier for everyone to understand.
There was a problem hiding this comment.
@AdamM-git Very nice! I like this solution! I've made some comments on the code you submitted, but overall I agree this is a huge improvement!
The solutions you see here are left over from an old and outdated version of the book, which I have been updating for the past year and half. I haven't gotten around to updating all of the solutions, and I'm sure there are many that need some work. I've only updated the ones where significant changes were made to the challenge or new exercises were added.
I agree that the solutions for this particular challenge are difficult to read and understand, and in fact not all of the solutions work. The second solution in the repo just returns an empty list!
I'd be happy to include this solution as the first one, but there are a couple of changes that I would like to be made. You can see the comments in my review for those.
If you make those changes, I'd be more than happy to merge this in and count you as a contributor!
| # We only look at cat indices that are a multiple of our step size | ||
| for i in range(1, 100//step + 1): | ||
|
|
||
| # We change the hat status for each cat we loop over | ||
| cats[i*step-1] = not cats[i*step-1] |
There was a problem hiding this comment.
I like the trick of using not to reverse the value at the index, but I'm not convinced that the way you calculate indices here is easier to read than using the % operator.
Personally, I think the following is easier to understand:
for i in range(1, 101):
# If the index is a multiple of the step, reverse the
# hat status of the cat at that index
if i % step == 0:
cats[i-1] = not cats[i-1]Granted, that solution requires an extra line of code, but IMO it's a lot easier to digest than first computing the number of multiples of step there are between 1 and 100, then computing those multiples in the indices.
There was a problem hiding this comment.
I agree with you, the way I wrote this part is clearly not easy to read.
I have a new suggestion as I don't like the fact that we loop over each cat and test them when we can know from the start exactly which ones we need to visit.
# For each step size we calculate how many cats we are going to visit
cats_to_visit = 100 // step_size
for i in range (1, cats_to_visit + 1):
# We reverse the hat status for each cat we visit
cats[i*step] = not cats[i*step]What do you think?
If you agree, I am a beginner with Github, should I directly edit my file with this?
| # Print the list of cats that have a hat | ||
| for i in range(100): | ||
| if cats[i]: | ||
| print(f"Cat {i+1} has a hat!") |
There was a problem hiding this comment.
+1 on this part of the solution. The statement of the problem says:
Write a program that simply outputs which cats have hats at the end.
I think the other solutions actually don't do a good job of this. They print a list of indices of cats that have hats.
Printing something like what you have here seems more in line with what the challenge asks for. 👍
Trick to simplify the math with the indices. Co-Authored-By: David Amos <[email protected]>
|
@somacdivad Thank you very much for your reply! I really was not expecting this to go anywhere. I am quite new to Github, and you may notice I have changed my username since the commit. Happy to contribute! |
Making the variables easier to understand and some renaming for clarity.
Now the cats that have a hat are all correctly displayed.
|
Hey @madamak, I'm returning from the holidays and will review this tomorrow, along with your new PR! Thanks again for your submissions! |
I found the solutions proposed a bit difficult to read. I think this one is just a direct translation of the instructions and will be easier for everyone to understand.