Loading collection data...
Collections are a way for you to organize kata so that you can create your own training routines. Every collection you create is public and automatically sharable with other warriors. After you have added a few kata to a collection you and others can train on the kata contained within the collection.
Get started now by creating a new collection.
Zebulan, you're criticizing me for holier than thou comments? OOokayyyyy. Not that I fully disagree, but you're the one to talk.
Thanks zebulan! That was really helpful.
@jeveley,
You don't have to change your code just because megawatt doesn't understand your algorithm. I started laughing when I read his quote
"In terms of speed though your code beats mine by a landslide, which I was actually shocked by"
. I'm not shocked because I've read a bunch of megawatts solutions. Mathematics is not my best area but 0.004 seconds compared to 2.413 seconds, seems like megawatt loses by a long shot.Even his suggested list comprehension has problems. (I know it's a different algorithm but for fun...)
integer / 2 + 1
?primes
when you just return it on the next line (without having to use it more than once)?Since I am known for PEP8 comments (got a kata, Zebulan's Nightmare, named after me because of it) here are the automatic warnings on your code using PyCharm.
def divisors
)divisors
is not used (you could delete the linedivisors = []
because you don't use it until you redeclare it asdivisors = list(primes)
)divisors
from outer scope (list has same name as the function, maybeall_divisors
or something to set it apart)divisors.append(divisor)
)The format of megawatts return statement is fine but it seems like your algorithm is better. You could replace the last three lines of your code with this:
Instead of creating the list of unique divisors and then sorting in-place with
.sort()
, you could sort as you create the list of unique divisors by usingsorted
.Feel free to ignore any of my comments or suggestions (and megawatts for that matter). I just get annoyed seeing most of megawatts 'holier than thou' comments. Not everyone on Codewars is that arrogant.
It definitely can be improved, and I will refactor it when I have the time, since from what I see, other people read those solutions. For this kata I focused on quickly writing this faster approach and just testing that it works. However, I don't think it's too long, which was your original concern. You just didn't understand it.
It's still pretty hard to understand, especially with a few lines like this because there are so many variables declared and no commenting:
Can you tell me that 2 months from now, you will come back to this code, and understand everything?
In terms of speed though your code beats mine by a landslide, which I was actually shocked by:
I still think it can be improved more though. Is the speed worth sacrificing readability?
If my code is hard to follow for you, I'll explain. I do prime factorization, and then compute combinations of those factors to get all divisors. I didn't write it for anyone to read, that's why it's all in one function and variables are named poorly.
Oh I never said my code was pretty, it's very far from ideal. I just disagree that I should always go for the shortest code. Definitely not in the expense of readability. I agree that your algorithm fits in two lines. However,its a different algorithm than mine. I came up with your solution in the first second after reading this kata. It's the most obvious way. Unfortunately, it's not the most efficient.
Yes I do know the book "Clean Code." One of the rules is to make functions as short as possible, with as few nests as possible. The author recommends 3-5 lines per function maximum.
"Each function has to do one thing, and do that one thing well"
You also have to give variable names that make sense. In a short function, something like 'x' or 'i' you can get away with, but not with a longer code like yours.
Comments also shouldn't be necessary in a function, but I think in your long version they should be added because I have a hard time following it.
The one line version may be hacky, but at least in my experience it's much easier to understand especially if you understand it.
BTW, have you noticed that I don't simply iterate from 2 to integer? Your one line doesn't replace my code.
Also, thanks for correcting my last line, I'm fairly new to python.
Thanks for your input. However, I strongly disagree with this approach. Making code shorter doesn't necessarily make it better. One liners tend to be hackiest and the most difficult to understand. Being able to read and understand code written by someone else is sometimes as important as efficiency. That's especially true with big projects that require maintenance. Do you know the book "Clean Code"?
Do the solutions have to be short? I'm doing this for myself and implementing the most upvoted solution seemed to easy to me.
Too long? :\