Ad
  • Custom User Avatar

    Zebulan, you're criticizing me for holier than thou comments? OOokayyyyy. Not that I fully disagree, but you're the one to talk.

  • Custom User Avatar
  • Custom User Avatar

    @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...)

    • Why would he keep counting up past integer / 2 + 1?
    • Why create a variable named 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.

    • PEP8: expected 2 blank lines found 1 (above def divisors)
    • Local variable divisors is not used (you could delete the line divisors = [] because you don't use it until you redeclare it as divisors = list(primes))
    • Shadows name divisors from outer scope (list has same name as the function, maybe all_divisors or something to set it apart)
    • PEP8: missing whitespace around operator (%, /=, =, +=)
    • Inconsistent indentation: mix of tabs and spaces (to the left of 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:

    return sorted(set(all_divisors)) or '{} is prime'.format(integer)
    

    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 using sorted.

    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.

  • Custom User Avatar

    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.

  • Custom User Avatar

    It's still pretty hard to understand, especially with a few lines like this because there are so many variables declared and no commenting:

    primes = []
    divisors = []
    i = 2
    temp = integer
    while i <= temp and temp > 1 and i < integer:
    

    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:

    divisors(12345678)
    
    megawatt's code: 0:00:02.413134
    jeveley's code: 0:00:00.004000
    

    I still think it can be improved more though. Is the speed worth sacrificing readability?

  • Custom User Avatar

    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.

  • Custom User Avatar

    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.

  • Custom User Avatar

    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.

  • Custom User Avatar

    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.

  • Custom User Avatar

    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"?

  • Custom User Avatar

    Do the solutions have to be short? I'm doing this for myself and implementing the most upvoted solution seemed to easy to me.

  • Custom User Avatar