最近見た残念なコード

最近見た残念なコード。なお、プロダクションで使われているコードなので、意味内容を変えつつ誇張して書いているため、実際はここまでひどくはない。

class App {
  Map<String, String[]> map;

  public void add(String protocol, String base, String[] paths) {
    for (String path : paths) {
      String[] items = new String[3];
      if (!map.containsKey(path)) {
        items[Constants.APP_URL_PROTOCOL] = protocol;
        String sep = paths.startsWith("/")? "" : "/";
        items[Constants.APP_URL_URL] = protocol.concat("://").concat(base).concat(sep).concat(path);
        items[Constants.APP_URL_PATH] = path;
        map.put(path, items);
      }
    }
  }

  public boolean storeUrls() {
    for(String key : map.keySet()) {
      String[] infos = map.get(key);
      // infos[Constants.APP_URL_PROTOCOL] などを使った処理
      // infos[Constants.APP_URL_URL] などを保存する処理
      // infos[Constants.APP_URL_PATH] などを保存する処理
    }
  }
}

他のところで取り出した URL を保存する処理なのだが、 URL だけでなく、プロトコルとかパスなども保存しているらしい。それは特段問題ではないのだが、問題なのはこれを URL を使えば意味も明確に処理できるのに対して、 String と配列という抽象的な要素をやりくりしてアプリを構築しているところである。

class App {

  Set<URL> urls;

  public void add(String protocol, String base, String[] paths) {
    for (String path : paths) {
      String sep = paths.startsWith("/")? "" : "/";
      URL url = new (protocol, base, sep.concat(path));
      urls.add(url);
    }
  }

  public boolean storeUrls() {
    for(URL url : urls) {
      // url.getProtocol() などを使った処理
      // url などを保存する処理
      // url.getPath() などを保存する処理
    }
  }
}

URL を使うことによって、何を保持しているか明確になった。もちろん、一部実装が変わっているところがある。それは、パスで一意性を保証していたのに対して、 URL で一意性を保証するように変わってしまったとことであるが、問題であるようならその場合にはパスによって一意性を保証するような専用の型を作れば良い。

---

まとめると

• String や配列のような抽象的な型をロジック中でやりくりしてデータを表現しない
• 何でも自力でやらない。標準APIやライブラリーが提供しているものを使う。
• API で提供していない、ライブラリーがないなら、専用の型を定義する。抽象的な型で頑張らない。