This is tangential to the article's point, but that `replace` function is a complete WTF in a way both authors completely ignore. Because it replaces things in the entire string in a loop, it will translate symbols recursively or not depending on ordering. Imagine you have the following dictionary:

    a=$b
    b=oops

if your input string just has one of these, it will just be translated once as the programmer was probably expecting:

    input:  foo $a bar
    output: foo $b bar

but if your input string first references $b later, then it will recursively translate $a.

    input:  foo $a bar $b
    output: foo oops bar oops

Sometimes translating recursively is a bizarre behavior and possibly a security hole.

The sane thing would be to loop through building the output string, adding the replacement for each symbol as you go. Using String.replace and the alreadyReplaced map is just a bad idea. Also inefficient, as it and throws away strings and does a redundant search on each loop iteration.

Feels typical of this whole '90s-era culture of arguing over refactoring with Java design patterns and ornate styles without ever thinking about if the algorithm is any good.

Edit: also, consider $foo $foobar. It doesn't properly tokenize on replacement, so this will also be wrong.

▲ scottlamb 3 days ago | parent | next [-]

> The sane thing would be to loop through building the output string, adding the replacement for each symbol as you go.

As follows:

    // SYMBOL_REF should be a class-level static final to avoid recompiling on each call.

    int pos = 0; // input[..pos] has been processed.
    StringBuilder out = new StringBuilder(); // could also guess at length here.
    Matcher m = SYMBOL_REF.matcher(input);
    while (m.find()) {
      String replacement = symbols.get(m.group(1));
      if (replacement == null) {
        continue; // no such symbol; keep literal `$foo`.
      }
      out.append(input, pos, m.start());
      out.append(replacement);
      pos = m.end();
    }
    out.append(input, pos, input.length());

(Apparently there's also now a Matcher.replaceAll one could use, but it's arguably cheating to outsource the loop to a method that probably didn't exist when the Uncle Bob version was written, and it's slightly less efficient in the "no such symbol" case.)

Coding style must serve the purpose of aiding understanding. If you have strong opinions about the coding style of `replace` but those opinions don't lead to recognition that the implementation was incorrect and inefficient, your opinions are bad and you should feel bad. Stop writing garbage books and blog posts!

</rant>

▲ shiandow 4 days ago | parent | prev [-]

The insane thing to do would be to implement a variant of loeb so it works regardless of order.

	▲	scottlamb 3 days ago \| parent [-]
		That'd still be less surprising!