7
\$\begingroup\$

The problem:

2520 is the smallest number that can be divided by each of the numbers from 1 to 10 without any remainder.

What is the smallest positive number that is evenly divisible by all of the numbers from 1 to 20?

The solution in Swift:

import Foundation

var number = 2520
var found = false

while found == false {
    var counter = 0
    
    for var iterator = 2; iterator < 21; iterator++ {
        
        if number % iterator == 0 {

            counter++
            
        } else {
            
            println("\(number) is not the number")
            break;
            
        }
        
    }
    
    if counter == 19 {
        
        found = true;
        
    } else {
        
        number = number + 2
        
    }
}
println("\(number) is the number!!")

Comments on making this code more efficient, making it more cleaner or violations of best practices would be highly appreciated.

\$\endgroup\$
0

3 Answers 3

12
\$\begingroup\$

== false

while found == false

We'd prefer to see either this:

while !found

or this:

while notFound

(where in the second example, we reverse the boolean logic you've used)


Vertical White Space

A blank line between every line of code is not productive. It adds wear to my scroll wheel and makes your code harder to digest.

Instead, we should think of blank lines as a way of taking a breather. They should separate logical chunks of code within a function or method.


Single Responsibility

I'd guess that this is all simply dumped into a playground, which is fine. But we can still write functions in the playground. And we should.

We've got quite a lot of nesting. Any time we have nesting is an indicator that we should be refactoring into functions.

Most importantly, functions have names. Using functions allows us to write more readable code without using comments because we're breaking our code up into small logic chunks that make sense together and giving each of these chunks a name (the function name).

Also, breaking our code into these chunks makes sure we don't choke on the elephant.


;

Semicolons are optional in Swift.

Before anything else, we should be consistent. Using semicolons on two lines and omitting on other lines has the potential to be confusing. If you want to use semicolons, you need to use them everywhere.

But this is Swift, and in Swift, we prefer omitting semicolons. I don't use semicolons anywhere in Swift.


Traditional for Loops.

We should basically never use these in Swift. I can't think of any scenario which requires their usage. First of all, from a style and readability standpoint, this is actually harder to read than the Swift alternatives, which make more sense from a plain-English point of view. The only reason the traditional for loops are even remotely readable is because of the decades of history that the C language has dumped on us.

We should vastly prefer something more like this:

for iterator in 2...20 {
    // do something with 2 through 20 inclusive...
}

And beyond the style comment, these Swift loops are actually faster than the traditional for loops. (And this explains how I manage to never use semicolons in Swift.)


var versus let

var declares a variable. let declares a constant.

We should always prefer let to var. The only time we should use var is when we're confident we will need to modify the value. Very few of your values need to be modified and most can be declared as let values.


Magic Numbers

Your code is littered with magic numbers. Rather than magic numbers, we should declare constants and name all of these numbers. Although, if we properly broke our code up into functions, most of these magic numbers would go away in place of function input arguments.

The only magic number in here we can't really get rid of is 0. Any time a number's best variable name would simply be the name of that number, it's not really a magic number. The only number for which this actually applies in your code is 0.

However, with that said, we can take care of zero with a function in this case:

extension Int {
    func isMultipleOf(divisor: Int) -> Bool {
        return self % divisor == 0
    }
}

We still have 0 in here, but it's wrapped inside a very small, very concise, very aptly named function that describes exactly what's going on here.


In summary...

Of course, this answer hasn't addressed any performance concerns. I've only addressed style concerns. I think style is one of the most important things to get right very early, before you've dug yourself into lots of bad habits.

For examples on what this might look like both in terms of better style and better performance, you can check out my question regarding this very same Project Euler challenge.

I'll be glad to help you with performance issues, but others have already addressed this, and I wouldn't really be able to say anything besides "go look how I did it, plus the performance-related answers I got". I wanted to emphasize Swift-style however, because I'm not sure how likely it is that any performance-related comments will be applicable to the next Swift code you write, but all of these style-related comments will absolutely be applicable.

And the better your style it is, the easier it is for both yourself and any reviewer to look at and focus on performance issues.

\$\endgroup\$
0
10
\$\begingroup\$

You are using a brute force attack to solve this problem but you should better think about it and use some maths.

Right now you are checking if the number is evenly dividable by 2, 3, 4, 5, 6 etc.

If you reverse the logic for instance you first check if it is evenly dividable by 20 you know that the number is also evenly dividable by 2, 4, 5 and 10.

If 18 is evenly dividable you know that this is also true for 3, 6, 9.

So by checking these 2 numbers you will know the result of 2, 3, 4, 5, 6, 9 and 10 too.

Altogether you need to check only the numbers from 20, 19, 18, 17, 16, 15, 14, 13, 12 and 11 which will result in 100% performance improvement.

Based on the great comment from @MartinR stating

Actually you need only to test for 19, 17, 13, 11, 7, 5, 9, 8

this can be enhanced further by using his second comment

What you really want to do (but that has been pointed out in many answers to similar questions, e.g. codereview.stackexchange.com/a/25204/35991), is compute the "least common multiple" of the numbers 2, ..20

\$\endgroup\$
0
3
\$\begingroup\$

There's a few things to point out:

  • You've got extraneous lines, you don't need to seperate them with an empty line (In most cases)
  • for var iterator = 2; iterator < 21; iterator++, this should be iterator <= 20 instead, as you're testing between less than or equal to 20, not less than 21. (They perform identically, but, it's a maths thing)
  • number = number + 2: you can use the += operator instead of var = var +: number += 2
  • is magic and doesn't need semi-colons at the end of a line, so the following examples have extraneous semi-colons:
    • found = true;
    • break;
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.