3
\$\begingroup\$

From this library I wrote, I have created this function in Swift, but I'm not happy with the implementation. Can anyone suggest a better way to do this? I really don't like changing counters inside for-loops...

extension Array {

    /// Removes all items that conform to the given closure
    mutating func removeAll(closure: (T) -> Bool) {
        for var counter = 0; counter < count; counter += 1 {
            if closure(self[counter]) {
                removeAtIndex(counter)
                counter -= 1
            }
        }
    }
}
\$\endgroup\$
7
\$\begingroup\$

Some general remarks:

  • The method name removeAll is misleading, as it does not remove all elements from the array. I would call it for example removeMatching.
  • The method parameter is a closure, but acts as a predicate, so predicate might be a better name.
  • The counter variable in the method is actually the array index so you could call it index.
  • The method documentation can be improved (details at the end of this answer).

Your concrete problem about modifying the loop counter inside the loop can easily be solved by traversing the array indices in reverse order:

for var index = count - 1; index >= 0; index-- { ... }

Using stride() allows you to have a constant loop variable:

for index in stride(from: count - 1, through: 0, by: -1) { ... }

Putting it all together:

extension Array {

    /// Removes all items that satisfy the predicate:
    mutating func removeMatching(predicate: (T) -> Bool) {
        for index in stride(from: count - 1, through: 0, by: -1) {
            if predicate(self[index]) {
                removeAtIndex(index)
            }
        }
    }
}

An alternative implementation would be

extension Array {

    /// Removes all items that satisfy the predicate:
    mutating func removeMatching(predicate: (T) -> Bool) {
        self = self.filter { !predicate($0) }
    }
}

The first method removes the matching elements, while the second method creates a new array and then replaces self. Which one is more efficient depends how many elements are actually removed in relation to the array size.

Notice that in Swift 2, Element should be used in place of T.

As mentioned above, the inline method documentation can be improved (I took this information from the SO thread Swift Documentation Comments). Example:

extension Array {
    /// Removes all items that satisfy the `predicate`
    ///
    /// :param: predicate A boolean predicate.
    mutating func removeMatching(predicate: (T) -> Bool) {
        self = self.filter { !predicate($0) }
    }
}

Now "Option-Click" will show you

enter image description here


Addendum: A removeAll(where:) was added to the Swift Standard library in Swift 4.2, so there is no need to define your own method for that purpose anymore.

As explained in SE-0197 this method is supposed to be both safe and efficient.

\$\endgroup\$
  • \$\begingroup\$ I agree with all points, especially the last using filter, but what is the advantage of using stride()? \$\endgroup\$ – vrwim Apr 11 '15 at 12:43
  • \$\begingroup\$ Ooh I was looking at using better documentation, and I was looking at @param, but that didn't seem to work, thanks! \$\endgroup\$ – vrwim Apr 11 '15 at 12:44
  • \$\begingroup\$ @vrwim: The advantage is that index is a constant inside the loop so that you cannot inadvertently modify it. Using a constant may also allow better compiler optimizations. \$\endgroup\$ – Martin R Apr 11 '15 at 12:45
  • \$\begingroup\$ @MartinR ok, thanks! I will test out the filter vs my implementation for speed and I'll get back to you \$\endgroup\$ – vrwim Apr 11 '15 at 12:46
  • \$\begingroup\$ Holy moly; When using this code (where removeMatching is your last code), I get this result, which is like 4-5 times slower. \$\endgroup\$ – vrwim Apr 11 '15 at 19:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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