0
\$\begingroup\$

I answered the problem of Find common directory path from Rosetta Code.

The problem consists of passing '/home/user1/tmp/coverage/test', '/home/user1/tmp/covert/operator', and '/home/user1/tmp/coven/members' to a function, the resultant path should be the valid directory '/home/user1/tmp' and not the longest common string '/home/user1/tmp/cove'.

The solution (the working code) is:

import Foundation


func getPrefix(_ text:[String]) -> String? {
    var common:String = text[0]
    for i in text {
        common = i.commonPrefix(with: common)
    }
    return common
}

var test = ["/home/user1/tmp/coverage/test", 
 "/home/user1/tmp/covert/operator",
 "/home/user1/tmp/coven/members"]

var output:String = getPrefix(test)!
print(output)

Could the above code be further optimized?

\$\endgroup\$
  • \$\begingroup\$ Looks to me that if your code doesn't return the right result, it doesn't need to be optimized but to be fixed: that would make your question off-topic for Code Review and you would have to ask that on Stack Overflow instead. \$\endgroup\$ – ayaio Sep 3 '17 at 12:39
  • \$\begingroup\$ I don't understand, the code works! What do you mean? \$\endgroup\$ – lf_araujo Sep 3 '17 at 12:40
  • \$\begingroup\$ The code runs but doesn't resurn the right result. I think this question belongs on Stack Overflow. \$\endgroup\$ – ayaio Sep 3 '17 at 12:44
  • \$\begingroup\$ The result should be /home/user1/tmp, and this is what I have in my terminal. What do you get? I am running it on the REPL. \$\endgroup\$ – lf_araujo Sep 3 '17 at 12:47
  • \$\begingroup\$ Ah! I get /home/user1/tmp/cove. See my screenshot of my Playground. \$\endgroup\$ – ayaio Sep 3 '17 at 12:48
3
\$\begingroup\$

Naming

Good names can greatly improve the readability of a program. Many of the names in the posted code can be improved:

  • getPrefix: the function returns the common prefix. It would be good to include that important clarifying detail in the name.
    • If you intend to return the common parent directory, I would call the function getCommonParent.
  • text: the parameter is not just any text, it's an array of paths, so paths would be a better name that conveys the intention.
  • i: is fine in simple counting loops, but when you iterate over values that have a meaningful name, it's better to use that, for example in this example path.
  • test is also an overly generic name, instead of the more descriptive paths

Algorithm

The implementation will call commonPrefix method text.count + 1 times. This is probably fine, but you could reduce the number of calls to commonPrefix by using a divide and conquer strategy.

For example, consider you have paths p1, p2, ..., pn. You could call commonPrefix for each pair, to get n / 2 results. Then you could call commonPrefix for each of those pairs. And so on, until there is only one pair left:

p1 p2 p3 p4 p5 p6 p7 p8
|__|  |__|  |__|  |__|
 |_____|     |_____|
    |___________|
          |
          = the common prefix

Admittedly, this might be overkill for your use case.

\$\endgroup\$

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.