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

    @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'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

    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.