1
\$\begingroup\$

On looking at the components(separatedBy separator: String) -> [String] method from the Swift Standard Library, I tried to come up with an implementation just for practice. Your comments are welcome to improve the same. Thanks.


Input:

func main() {
    let sampleString = "Do not be sorry. Be better."
    print(sampleString.components(separatedBy: "."))
}

Output:

["Do not be sorry", " Be better", ""]

Implementation:

extension StringProtocol {

    func components<T>(separatedBy separatorString: T) -> [String] where T: StringProtocol {

        var currentIndex = 0; var stringBuffer = ""; var separatedStrings:[String] = []

        forEach { (character) in

            if String(character) == separatorString {
                separatedStrings.append(stringBuffer); stringBuffer = ""
            } else {
                stringBuffer += .init(character)
            }

            if currentIndex == lastIndex { separatedStrings.append(stringBuffer) }
            currentIndex += 1
        }
        return separatedStrings
    }
}

extension Collection {
    var lastIndex:Int {
        get {
            return self.count - 1
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Right off the bat I can see that the array returned could be named better as separatedComponents instead of separatedStrings. \$\endgroup\$ – Badhan Ganesh Dec 13 '18 at 22:05
  • \$\begingroup\$ Could you please point to the SL implementation of components(separatedBy separator: String) -> [String]? \$\endgroup\$ – ielyamani Dec 16 '18 at 13:15
  • \$\begingroup\$ @Carpsen90 Sorry I am not able to find it. I just tried to implement out of my own interest. \$\endgroup\$ – Badhan Ganesh Feb 27 at 8:16
  • 2
    \$\begingroup\$ @Carpsen90 - It’s found in swift-corelibs-foundation: github.com/apple/swift-corelibs-foundation/blob/…. When looking for stuff like this, go to the repo in question and then search for "func components" (with the quotes). \$\endgroup\$ – Rob Mar 1 at 19:26
  • 1
    \$\begingroup\$ Martin has answered your questions. But let’s assume for a second that you really did want to advance currentIndex yourself. I would not make it an Int. I’d also eliminate that lastIndex property. Instead, define startIndex as var currentIndex = startIndex, advance it with currentIndex = index(after: currentIndex) and then check if I’m at the end with if currentIndex == endIndex { ... }. It’s all moot, as this should be refactored out, as Martin points out, but just a few observations. \$\endgroup\$ – Rob Mar 1 at 19:43
2
\$\begingroup\$

Coding style

This is of course a matter of personal taste, but would split multiple statements like

var currentIndex = 0; var stringBuffer = ""; var separatedStrings:[String] = []

into separate lines

var currentIndex = 0
var stringBuffer = ""
var separatedStrings:[String] = []

I would also start new lines for nested code blocks, i.e.

if currentIndex == lastIndex { separatedStrings.append(stringBuffer) }

becomes

if currentIndex == lastIndex {
    separatedStrings.append(stringBuffer)
}

Correctness

Your function takes a separator of type string (protocol) as an argument, but actually works only for separators consisting of a single character. Example:

let sampleString = "Do not be sorry. Be better."
print(sampleString.components(separatedBy: ". "))
// ["Do not be sorry. Be better."]

The reason is that here

if String(character) == separatorString

a single character of the source string is compared with the separator.

Your method also behaves differently from the standard library version when called with an empty string,

"".components(separatedBy: ".")

returns an empty array [] instead of a single-element array [""]. The reason is that the check

if currentIndex == lastIndex { separatedStrings.append(stringBuffer) }

is never done for an empty input string. (Checking for the last iteration inside a loop always makes me suspicious.)

Simplifications

Instead of converting a character to a string for appending

stringBuffer += .init(character)

you can append it directly:

stringBuffer.append(character)

Keeping track of the current character position can be done with enumerated()

for (currentIndex, character) in self.enumerated() {
    // ...
}

instead of incrementing var currentIndex.

Efficiency

The main bottleneck is the

var lastIndex:Int

extension method. For Strings (and other collections which are not random accessible) determining self.count is a O(N) operation (N = number of characters). It requires traversing the entire string.

This method is called for each character in the source string, so that this contributes O(N^2) to the execution time.

It would also be more efficient to locate the next occurrence of the separator and append an entire substring to the result array, instead of appending single characters repeatedly.

Alternative implementation

Here is an alternative implementation, considering the above suggestions:

func components<T>(separatedBy separatorString: T) -> [String]
    where T: StringProtocol, Index == String.Index {
        var separatedStrings: [String] = []
        var pos = startIndex
        while let range = self[pos...].range(of: separatorString) {
            separatedStrings.append(String(self[pos..<range.lowerBound]))
            pos = range.upperBound
        }
        separatedStrings.append(String(self[pos...]))
        return separatedStrings
}
\$\endgroup\$
  • \$\begingroup\$ @Carpsen90: I doubt it. I assume that the subscript transforms pos... to pos..<endIndex using this method, so there should be no difference. – See also github.com/apple/swift-evolution/blob/master/proposals/…. \$\endgroup\$ – Martin R Dec 19 '18 at 10:29
  • \$\begingroup\$ Thanks. I had a faint doubt that that transformation would cost some extra work, either at compile or run times. \$\endgroup\$ – ielyamani Dec 19 '18 at 10:40
  • \$\begingroup\$ Thank you so much for your review, @MartinR! 🙂 \$\endgroup\$ – Badhan Ganesh Jan 4 at 15:39

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.