Skip to main content
Code Review

Return to Answer

deleted 9 characters in body
Source Link
Martin R
Martin R
  • 24.2k
  • 2
  • 38
  • 96

There is one small bug in your code, a two-characterscharacter string is formatted with an an initial dash:

func format(_ str: String) -> String {
    
    switch str.characters.count {
    case 0, 1, 2, ...3:  // No separators for strings up to length 3.
        return str
    case 4: // "abcd" -> "ab-cd"
        let idx = str.index(str.startIndex, offsetBy: 2)
        return str.substring(to: idx) + "-" + str.substring(from: idx)
    default: // At least 5 characters. Separate the first three and recurse:
        let idx = str.index(str.startIndex, offsetBy: 3)
        return str.substring(to: idx) + "-" + format(str.substring(from: idx))
    }
    
}

There is one small bug in your code, a two-characters string is formatted with an initial dash:

func format(_ str: String) -> String {
    
    switch str.characters.count {
    case 0, 1, 2, 3:  // No separators for strings up to length 3.
        return str
    case 4: // "abcd" -> "ab-cd"
        let idx = str.index(str.startIndex, offsetBy: 2)
        return str.substring(to: idx) + "-" + str.substring(from: idx)
    default: // At least 5 characters. Separate the first three and recurse:
        let idx = str.index(str.startIndex, offsetBy: 3)
        return str.substring(to: idx) + "-" + format(str.substring(from: idx))
    }
    
}

There is one small bug in your code, a two-character string is formatted with an initial dash:

func format(_ str: String) -> String {
    
    switch str.characters.count {
    case 0...3:  // No separators for strings up to length 3.
        return str
    case 4: // "abcd" -> "ab-cd"
        let idx = str.index(str.startIndex, offsetBy: 2)
        return str.substring(to: idx) + "-" + str.substring(from: idx)
    default: // At least 5 characters. Separate the first three and recurse:
        let idx = str.index(str.startIndex, offsetBy: 3)
        return str.substring(to: idx) + "-" + format(str.substring(from: idx))
    }
    
}
added 4 characters in body
Source Link
Martin R
Martin R
  • 24.2k
  • 2
  • 38
  • 96

There is one small bug in your code, a two-characters string is formatted with an initial dash:

bywith

$0.offset == count - 2 && $0.offset % 23 == 2

I would use for ... in, so that we can give the loop variables names:

There is also no need to call return at the andend of the iteration block.

  • Choose a different name instead of format(), which indicates the purpose of the formatting.
  • Make the separator characterscharacter a parameter of the function.

There is one bug in your code, a two-characters string is formatted with an initial dash:

by

$0.offset == count - 2 && $0.offset % 2 == 2

I would use

There is also no need to call return at the and of the iteration block.

  • Choose a different name instead of format(), which indicates the purpose of the formatting.
  • Make the separator characters a parameter of the function.

There is one small bug in your code, a two-characters string is formatted with an initial dash:

with

$0.offset == count - 2 && $0.offset % 3 == 2

I would use for ... in, so that we can give the loop variables names:

There is also no need to call return at the end of the iteration block.

  • Choose a different name instead of format() which indicates the purpose of the formatting.
  • Make the separator character a parameter of the function.
Source Link
Martin R
Martin R
  • 24.2k
  • 2
  • 38
  • 96

There is one bug in your code, a two-characters string is formatted with an initial dash:

print(format("12")) // -12

There are various ways to fix that, e.g. by replacing the condition

$0.offset == count - 2 && count % 3 != 0

by

$0.offset == count - 2 && $0.offset % 2 == 2

Now some suggestions to simplify the code and make it more readable. Instead of

unformatted.characters.enumerated().forEach {
     // ... use `$0.offset` and `$0.element` ...
}

I would use

for (offset, char) in unformatted.characters.enumerated() {
     // ... use `offset` and `char` ...
}

There is no need to convert $0.element to a String because you can append a character directly:

formatted.append($0.element)

There is also no need to call return at the and of the iteration block.

The condition

if $0.offset % 3 == 0 && $0.offset != 0 && $0.offset != count - 1 || $0.offset == count - 2 && $0.offset % 2 == 2

is quite complex, I would split it into two conditions with if/else if.

Putting it all together, the function would then look like this:

func format(_ unformatted: String) -> String {
    
    var formatted = ""
    let count = unformatted.characters.count
    
    for (offset, char) in unformatted.characters.enumerated() {
        if offset > 0 && offset % 3 == 0 && offset != count - 1 {
            formatted.append("-")
        } else if offset % 3 == 2 && offset == count - 2 {
            formatted.append("-")
        }
        formatted.append(char)
    }
    
    return formatted
}

The condition whether to insert the separator at an offset is quite complex and it is easy to make an error. A completely different approach would be a recursive implementation, which is (almost) self-explaining:

func format(_ str: String) -> String {
    
    switch str.characters.count {
    case 0, 1, 2, 3:  // No separators for strings up to length 3.
        return str
    case 4: // "abcd" -> "ab-cd"
        let idx = str.index(str.startIndex, offsetBy: 2)
        return str.substring(to: idx) + "-" + str.substring(from: idx)
    default: // At least 5 characters. Separate the first three and recurse:
        let idx = str.index(str.startIndex, offsetBy: 3)
        return str.substring(to: idx) + "-" + format(str.substring(from: idx))
    }
    
}

More suggestions:

  • Choose a different name instead of format(), which indicates the purpose of the formatting.
  • Make the separator characters a parameter of the function.