Feedback erwünscht

[Matthias Kolley]

Ich bin gerade dabei das Buch Clean Code von Robert C. Martin durch zu lesen.
Nebenbei versuche ich das erlernte immer mal wieder anzuwenden.
Da ich noch ganz frisch in dem Thema bin, habe ich mir ein einfaches Beispiel genommen.

Ich habe folgende Methode und möchte diese gern so refactoren, dass sie besser lesbar, verständlicher ist.

//validates the topic
//replaces all "." by "_"
//ignores everything that is not a letter, a number, "_" or "-"
private static String validateTopic(final String topic) {
  final char[] chars = topic.toCharArray();
  final StringBuilder sb = new StringBuilder();
  for (int i = 0; i < topic.length(); i++) {
    final char ch = chars[i];
    if (('A' <= ch) && (ch <= 'Z')) {
      sb.append(ch);
    } else if (('a' <= ch) && (ch <= 'z')) {
      sb.append(ch);
    } else if (('0' <= ch) && (ch <= '9')) {
      sb.append(ch);
    } else if ((ch == '_') || (ch == '-')) {
      sb.append(ch);
    } else if (ch == '.') {
      sb.append('_');
    }
  }
  return sb.toString();
}

Ich habe natürlich dafür einige Tests geschrieben, damit ich die Funktionalität nicht kaputt mache:

@Test
  public void testValidateTopic() {
    assertEquals("this_is_a_valid_topic", Services.validateTopic("this_is_a_valid_topic"));
    assertEquals("this-is-also-a-valid-topic", Services.validateTopic("this-is-also-a-valid-topic"));
    assertEquals("this_was_a_non_valid_topic", Services.validateTopic("this.was.a.non.valid.topic"));
    assertEquals("a-topic-with-a-number4", Services.validateTopic("a-topic-with-a-number4"));
    assertEquals("a_mixed-topic_with0different-separators", Services.validateTopic("a_mixed-topic.with0different-separators"));
    assertEquals("ExpectedValue", Services.validateTopic("E!x§p$e%c&t/e(d)V=a?l+u#e"));
  }

Meine Lösung sieht wie folgt aus:

private static final String VALID_TOPIC_REGEX = "[a-zA-z0-9_-]+";

  static String validateTopic(final String topic) {
    if (isAlreadyValidTopic(topic)) {
      return topic;
    }
    final StringBuilder sb = new StringBuilder();
    for (final char c : topic.toCharArray()) {
      if (isValidChar(c)) {
        sb.append(c);
      } else if (c == '.') {
        sb.append('_');
      }
    }
    return sb.toString();
  }

  private static boolean isAlreadyValidTopic(final String topic) {
    return topic.matches(VALID_TOPIC_REGEX);
  }

  private static boolean isValidChar(final char charToCheck) {
    if (isUpperCaseChar(charToCheck)) {
      return true;
    }
    if (isLowerCaseChar(charToCheck)) {
      return true;
    }
    if (isNumberChar(charToCheck)) {
      return true;
    }
    if (isUnderscoreOrDash(charToCheck)) {
      return true;
    }
    return false;
  }

  private static boolean isUpperCaseChar(final char charToCheck) {
    return isBeetweenIncluding(charToCheck, 'A', 'Z');
  }

  private static boolean isLowerCaseChar(final char charToCheck) {
    return isBeetweenIncluding(charToCheck, 'a', 'z');
  }

  private static boolean isNumberChar(final char charToCheck) {
    return isBeetweenIncluding(charToCheck, '0', '9');
  }

  private static boolean isUnderscoreOrDash(final char charToCheck) {
    return charToCheck == '_' || charToCheck == '-';
  }

  private static boolean isBeetweenIncluding(final char charToCheck, final char lowerLimit,
      final char upperLimit) {
    if (lowerLimit <= charToCheck && charToCheck <= upperLimit) {
      return true;
    }
    return false;
  }

Meine konkrete Frage zum Feedback:
Ist das so schon ok? Bzw. was kann ich noch besser machen und warum?

[Robin Schürer]

Hallo,

ich finde den Methoden-Namen "validateTopic" irgendwie komisch. Vor allem da ein String zurückgegeben wird.

[Matthias Kolley]

Hallo Robin

Erstmal: Danke für dein Feedback. :)

ich finde den Methoden-Namen "validateTopic" irgendwie komisch.

Ich habe überlegt, ob ich den nicht auch schon umbenennen soll, habe es offensichtlich vergessen.
Du hast natürlich vollkommen recht.
Die Methode müsste eher convertTopic oder validateAndConvertTopic oder so heißen.
Woebi wenn ich die 2. Option so sehe, macht die Methode ja im Prinzip wieder 2 Sachen.
Prüfen ob korrekt und wenn nicht dann konvertieren. Es ist also noch Bedarf....

Danke.

[Guenter Schwann]

Hi Matthias,

Die neue Lösung ist weit mehr als doppelt so lang. Und in meinen Augen dadurch nicht wirklich besser lesbar.

Ich frage mich ob die Funktionen isUpperCaseChar(), isLowerCaseChar() etc. wiederverwendet werden (können/sollen). Wenn nicht, würde ich persönlich sie weg lassen.

Allgemein sind mir hier zu viele if statement. Vor allem in der letzten Funktion. Ein simples
return (lowerLimit <= charToCheck) && (charToCheck <= upperLimit);
Ist einfacher.
Auch in isValidChar() könnte man die Checks zusammenfassen.

Die Dokumentation war auch nicht so schlecht (JavaDoc/Doxygen verwenden). Hätte ich gelassen bzw noch leicht verbessert. Kommentar != Dokumentation.

Und beim Thema Funktionsnamen:

isAlreadyValidTopic() - das "already" in dem Namen gibt dem ganzen einen unnötigen zeitlichen Bezug (also ob die Funktion wüsste wann sie aufgerufen wird - weiss sie nicht, sollte sie auch nicht).

isValidTopic() oder validTopic() ist kürzer und besser.

Die "perfekte" Lösung gibt es sowieso nicht. Denn da spielt noch viel eigener "Geschmack" mit - der sich im Laufe der Zeit auch stark ändert ;)

Gruß

Günter




--
Sie erhalten diese Nachricht, weil Sie in Google Groups E-Mails von der Gruppe "Clean Code Developer" abonniert haben.
Wenn Sie sich von dieser Gruppe abmelden und keine E-Mails mehr von dieser Gruppe erhalten möchten, senden Sie eine E-Mail an clean-code-developer+unsub...@googlegroups.com.
Wenn Sie in dieser Gruppe einen Beitrag posten möchten, senden Sie eine E-Mail an clean-code-developer@googlegroups.com.
Gruppe besuchen: https://groups.google.com/group/clean-code-developer
Weitere Optionen finden Sie unter https://groups.google.com/d/optout.

--

Guenter Schwann

[Joachim Spehl]

Bei validate würde ich als Ergebnis eher ein bool oder ein result-Objekt erwarten.
Irgendwie macht deine Methode eher sowas, wie eine Normalisierung, oder?

String getNormalizedTopic(String topic)

Am Mittwoch, 4. Oktober 2017 08:02:38 UTC+2 schrieb Matthias Kolley:

[Jan Lippert]

Hi,

in der vorgeschlagenen Lösung wird ein regulärer Ausdruck verwendet. Ähnlich könnte das normalisieren eines Topics umgesetzt werden. In der ursprünglichen Variante gibt es diesen Kommentar:

//replaces all "." by "_"
//ignores everything that is not a letter, a number, "_" or "-"

Die String-Klasse hat eine Methode replaceAll, welche für eben diese Schritte genutzt werden könnte:

final String INVALID_CHARACTERS_REG_EX = "[^1a-zA-Z0-9_-]";

/** 

  * Replaces all "."s by "_" and removes everything that is not a valid character (letters, numbers, dashes, underscores) of the given topic.

  */

static String getNormalizedTopic(String topic) {

  if (topic == null) { /* TODO handle null or mark topic as @NotNull */ return ""; }

  topic.replaceAll("\\.", "_").replaceAll(INVALID_CHARACTERS_REG_EX, "");

}

Vermutlich kann INVALID_CHARACTERS_REG_EX auch zum validieren benutzt werden.

Viele Grüße,
Jan

--

Sie erhalten diese Nachricht, weil Sie in Google Groups E-Mails von der Gruppe "Clean Code Developer" abonniert haben.

Wenn Sie sich von dieser Gruppe abmelden und keine E-Mails mehr von dieser Gruppe erhalten möchten, senden Sie eine E-Mail an clean-code-devel...@googlegroups.com.
Wenn Sie in dieser Gruppe einen Beitrag posten möchten, senden Sie eine E-Mail an clean-code...@googlegroups.com.

[Matthias Kolley]

Hallo Jan.

Wow Danke.
Ja die Methode der String-Klasse ist im Prinzip bekannt. Mein Problem war wahrcheinlich eher die unerfahrenheit mit Reg-Ex.

@Birdy und @Joachim
Vielen Dank auch für euer Feedback.
Selbst ohne Jan's Lösung konnte ich dadurch a) meine Methoden Anzahl reduzieren und es b) trotzdem lesbar halten.
z.B. validateTopic in normalizeTopic umbenennen.
Allerdings ist "meine" Lösung dann immernoch doppelt so lang wie das Original.

Die if's in isValidChar würde ich nur ungern in eine Zeile schreiben, weil das (für mich) zu viele Konditionen in einem if sind.

"Meine" Lösung sieht aktuell wie folgt aus:

private static final String VALID_TOPIC_REGEX = "[a-zA-z0-9_-]+";

  /**
   * Takes a topic and normalizes it by replacing all "." with a "_".
   * The normalization ignores everything that is not a letter, number, "_" oder "-".
   * Therefore everything else will be removed from the given topic.
   * @param topic the topic to normalize
   * @return a normalized topic with "." replaced by "_" and everything that is not a letter, number, "_" or "-" removed
   */
  static String normalizeTopic(final String topic) {
    if (topic.matches(VALID_TOPIC_REGEX)) {

      return topic;
    }
    final StringBuilder sb = new StringBuilder();
    for (final char c : topic.toCharArray()) {
      if (isValidChar(c)) {
        sb.append(c);
      } else if (c == '.') {
        sb.append('_');
      }
    }
    return sb.toString();
  }

  private static boolean isValidChar(final char charToCheck) {
    if (isBeetweenIncluding(charToCheck, 'A', 'Z')) {
      return true;
    }
    if (isBeetweenIncluding(charToCheck, 'a', 'z')) {
      return true;
    }
    if (isBeetweenIncluding(charToCheck, '0', '9')) {
      return true;
    }
    if (charToCheck == '_' || charToCheck == '-') {
      return true;
    }
    return false;
  }

  private static boolean isBeetweenIncluding(final char charToCheck, final char lowerLimit, final char upperLimit) {
    return lowerLimit <= charToCheck && charToCheck <= upperLimit;
  }

Wobei die Lösung von Jan natürlich unschlagbar kurz ist!

[Ralf Westphal]

Ich finde, die Lösung stand schon in der ursprünglichen Aufgabe drin. Warum hast du die nicht erhalten? Du hättest sie nur in Code gießen müssen:

public static string normalize_topic(string topic) {
  topic = Replace_dot(topic);
  return Ignore_invalid_chars(topic);
}

private static string Replace_dot(string topic){
  return topic.Replace(".", "_");
}

private static string Ignore_invalid_chars(string topic) {
  var sb = new StringBuilder();
  foreach(var ch in topic)
    if (Is_valid_char(ch)) sb.Append(ch);
  return sb.ToString();
}

private static bool Is_valid_char(char ch) {
  if (('A' <= ch) && (ch <= 'Z')) return true;
  if (('a' <= ch) && (ch <= 'z')) return true;
  if (('0' <= ch) && (ch <= '9')) return true;
  if ((ch == '_') || (ch == '-')) return true;
  return false;
}

• Der Kommentar wird überflüssig, weil das, was darin stand, jetzt im Code steht.
• Die Lösung ist kürzer als das Original.
• Der Funktionsname passt jetzt auch besser ;-)
  
• Der Prozess der Transformation ist klar ersichtlich.
• Die Prüfung auf valide Zeichen ist jetzt fokussiert. Sie tut nur das und fügt nicht auch noch Zeichen zu einer StringBuilder hinzu.
• Die Prüfung auf valide Zeichen ist erstmal so gelassen, wie im Original. Aber sie kann auch optimiert werden mit RegEx oder sonstwie.
• Ignore_invalid_chars() ist umständlich, aber das ist der Sprache geschuldet. Ich habe zwar C# benutzt, aber kein Linq, um nah deinem Java zu bleiben. Wenn du mal Java Streams anschaust, kannst du da aber noch kürzer werden.

private static string Ignore_invalid_chars(string topic) {
  var validChars = topic.ToCharArray().Where(Is_valid_char);
  return new string(validChars.ToArray());
}

[Robin Schürer]

Hallo,

ausgehend von Ralfs Lösung nochmal als Java-Version.

public static String normalizeTopic(String topic) {
   String replaced  = topic.replace(".", "_");
   return ingoreInvalidChars(replaced);
}

private static String ingoreInvalidChars(String topic) {
   return topic
         .chars()
         .mapToObj(i -> (char)i)
         .filter(Application::isValidChar)
         .map(Object::toString)
         .collect(Collectors.joining());
}

private static boolean isValidChar(char ch) {
   if (('A' <= ch) && (ch <= 'Z')) return true;
   if (('a' <= ch) && (ch <= 'z')) return true;
   if (('0' <= ch) && (ch <= '9')) return true;
   if ((ch == '_') || (ch == '-')) return true;
   return false;
}

Am Mittwoch, 4. Oktober 2017 08:02:38 UTC+2 schrieb Matthias Kolley:

[Daniel Rothmaler]

Hab mal noch eine Alternative Java-Version zusammen gebaut: 

static String normalizeTopic(final String topic) {

    IntStream result = topic.chars()

            .map(Services::replaceDots)

            .filter(Services::isValidCharacter);

    return collectToString(result);

}

private static int replaceDots(int c) {

    if (c == '.') {

        return '_';

    }

    return c;

}

private static boolean isValidCharacter(int c) {

    return Character.isLetter(c) ||

            Character.isDigit(c) ||

            c == '_' ||

            c == '-';

}

private static String collectToString(final IntStream characterStream) {

    return characterStream

            .collect(StringBuilder::new, StringBuilder::appendCodePoint, StringBuilder::append)

            .toString();

}

Aber so richtig gut gefällt mir das selber noch nicht, weil ich das Stream-Handlung nicht wirklich intuitiv/clean finde.

Ich tendiere eher dazu, dass Ganze in eine kleinen "Topic"-Klasse zu kapseln (aber die Idee ist noch nicht richtig ausgegoren)... Die könnte das Tonic dann als privaten State halten und verändern, was die ganze Sache lesbarer machen kann.

VG Daniel

[Matthias Kolley]

Hallo Daniel und Robin.

Danke für eure Java Umsetzungen.
Das Problem an diesen Umsetzungen ist allerdings, das diese deutlich langsamer sind als meine aktualisierte Lösung.
MeinLösung: ca 0.015s
EureLösung: ca 0.050s

@Ralph: Zu deiner Frage:

Ich finde, die Lösung stand schon in der ursprünglichen Aufgabe drin. Warum hast du die nicht erhalten?

Weil ich dieses große if Konstrukt nicht wirklich verständlich fand.

Mit all euren Anmerkungen habe ich meine Lösung allerdings noch ein bisschen verfeinert:

private static final String VALID_TOPIC_REGEX = "[a-zA-z0-9_-]+";

  /**

   * Takes a topic and normalizes it by replacing all "." with a "_".
   * The normalization ignores everything that is not a letter, number, "_" oder "-".
   * Therefore everything else will be removed from the given topic.
   * @param topic the topic to normalize
   * @return a normalized topic with "." replaced by "_" and everything that is not a letter, number, "_" or "-" removed
   */

  static String normalizeTopic(final String topic) {

    if (topic.matches(VALID_TOPIC_REGEX)) {
      return topic;
    }
    final String result = topic.replace('.', '_');

    final StringBuilder sb = new StringBuilder();

    for (final char c : result.toCharArray()) {
      if (isValidChar(c)) {
        sb.append(c);
      }
    }
    return sb.toString();
  }

  private static boolean isValidChar(final char charToCheck) {
    return Character.isLetter(charToCheck) || Character.isDigit(charToCheck) || charToCheck == '_' || charToCheck == '-';
  }

Und ich muss sagen, dass ich persönlich damit recht zufrieden bin.

Wobei wir dann wieder zu dem Punkt von Birdy zurück:

[Ralf Westphal]

Siehste mal, das geht auch mit Streams :-) Java ist doch nicht so schlecht :-D (allerdings scheint mir das Stream-Handling etwas verrauscht gegenüber Linq.)

Was ich jedoch nicht ideal finde, dass du das replace() in normalizeTopic() direkt drin hast.

replace() auf dem String und der Aufruf von ignoreInvalidChars() liegen auf unterschiedlichen Abstraktionsniveaus.

In der ersten Zeile muss man mehr interpretieren, erstmal die Bedeutung finden. In der zweiten Zeile steht sie qua Funktionsname schon da.

Insofern war auch mein Funktionsname ReplaceDot() nicht ideal. Der sollte nur dicht am Kommentar des Originals sein.

Eigentlich geht es inhaltlich um zwei Schritte:

1. Replace invalid chars

2. Filter invalid chars

Manche Zeichen können halt ersetzt werden, andere müssen rausfliegen.

Welche das sind, kann man sich bei Bedarf in den Funktionen anschauen. Um das Ganze - Normalisierung - zu verstehen, muss man das aber nicht. Da reicht ein Blick auf zwei Funktionsaufrufe. So wird aus der Normalisierung eine reine Integration, die keine Logik enthält (Integration Operation Segregation Principle eingehalten).

[Ralf Westphal]

Hm... warum ist die Performance relevant für dich? Hast du gemessen, dass durch EureLösung das Programm ein Problem hat?

Ansonsten würde ich eine Performanceoptimierung nicht durchführen. Die kostet Entwicklungszeit - das ist eine garantierte Ausgabe - bei ungewissem Wert für den Kunden - man weiß nicht, ob man das bezahlt bekommt. Außerdem tendieren Optimierungen auf die Performance hin dazu, dass eine andere Qualität leidet, insbesondere die Verständlichkeit eines Algorithmus.

Für mich ist denn auch das passiert: MeinLösung mag schneller sein - aber ich finde sie nicht mehr so verständlich wie EureLösung.

Außerdem ist das DRY-Prinzip verletzt: was zu einem normalisierten Topic gehört, ist einerseits in der RegEx versteckt, andererseits aber auch im Algorithmus.

Und der Algorithmus steht als Logik direkt in der Normalisierung. Da lassen sich seine zwei Aspekte nur schwer erkennen (Ersetzung und Filterung).

Der Kommentar widerspricht ebenfalls dem DRY-Prinzip. Darin ist all das nochmal aufgeführt, was im Code ebenfalls steht. Doppelte Arbeit, Risiko der Inkonsistenz bei Veränderungen.