Упрощение кода с case classes - пример

Давненько я ничего не писал с приверами кода... Вот немного практики в продолжение октябрьского поста про читабельность. Несколько месяцев назад довелось написать вот такой вот класс. Ни функционал ни производительность этого кода особо не важны - затыкалась отдельно взятая дыра в дизайне, кажется успешно :) Если описать этот класс кратко - его экземпляры должны позволять дожидаться получения фиксированного количества результатов. Однако даже беглый осмотр позволяет понять что  код тяжеловато читать, для задачи такой сложности, думаю в районе третьей строчки все закрыли гист и вернулись на эту страницу.

Как мне кажется проблемы с читабельностью имеют две основные причины.

1. 3 возможных состояния объекта закодированы в 3 аттрибутах. При этом возможны заведомо бессмысленные состояния (doneCnt != res.size() или ex != null && doneCnt == totCnt).
2. Методы fail, add, listen судя по коду предпринимают какие-то действия  в слепую, иногда проверяя лишь часть переменных. Как следствие о корректности кода нельзя судить по отдельным фрагментам - нужно думать о поведении всех методов сразу.

Это в принципе типичные неприятности при дизайне классов в классическом джавовском стиле. Давайте посмотрим как пара простых изменений позволили их устранить.

Суть выражена в переходе от набора из нескольких аттрибутов к одному но с более сложной структурой.
Было:

private var doneCnt = 0
private var ex: Exception = null
private val res = new ArrayList[T](totCnt)

Cтало:

private var res: Either[util.ArrayList[T], Exception] = Left(new util.ArrayList[T](totCnt))

Помимо экономии пары строк мы добавили немного семантики изменяемому состоянию объектов. Теперь видно что наличие исключения и списка результатов - взаимоисключающие состояния. Но есть ли ещё какие-то скрытые значения в состоянии списка? С одной стороны можно бы это описать в комментариях, но они как известно не проверяются компилятором. Так что я попробовал сделать так чтобы она стала ясна из прочтения первого-же метода.

Было:

def add(item: T) {
  lock.lock()
  try {
    res.add(item)
    doneCnt = doneCnt + 1
    if (doneCnt == totCnt) {
      done.signalAll()
      for (lsnr <- succLsnrs)
        lsnr(res)
    }
  }
  finally {
    lock.unlock()
  }
}

Cтало:

def add(item: T) {
  lock.lock()
  try {
    res match {
      case Left(list) if list.size < totCnt => {
        list.add(item)
        if (list.size == totCnt) {
          done.signalAll()
          for (lsnr <- succLsnrs)
            lsnr(list)
        }
      }
      case _ => return
    }
  }
  finally {
    lock.unlock()
  }
}

Смотрим метод add, из него видно что добавления результатов разрешены пока количество элементов в списке меньше totCnt. Здесь уже вырисовывается точная семантика для аттрибута res.

1. Right(exception) - сбор результатов провалился.
2. Left(list) && list.size < totCnt - сбор результатов всё ещё в процессе.
3. Left(list) && list.size == totCnt - сбор результатов завершён их все можно получить.

По-доброму стоило бы сделать предикаты для res отвечающие в каком именно состоянии он находится, и это добавило бы коду ясности. Но я отказался от этого варианта в пользу match по двум причинам. Во-первых количество обращений к этим предикатам было бы крайне небольшим, повторное использование получалось весьма небольшим. Во-вторых case одновременно с определением состояния позволяет извлечь из него какие-то значения, что немного экономит место на экране. По-моему в коде такой сложности место на экране и необходимость прокруток являются существенными источниками проблем с читаемостью, так что было выбрано решение с матчингом. Надо заметить что есть тут и негативный эффект - выросло количество уровней вложенности.

Метод fail тоже неплохо прибавил в выразительности - теперь оба add и fail явно переводят объект из одного состояния в другое. Надо признать что новая версия ведёт себя не точно так-же как старая, новый подход сделал более строгое поведение и более лёгким. add больше не принимает результаты сверх заданного числа, а fail - не изменяет состояние завершённого объекта.

Было:

def fail(e: Exception) {
  lock.lock()
  try {
    ex = e
    done.signalAll()
    for (lsnr <- failLsnrs)
      lsnr(ex)
  }
  finally {
    lock.unlock()
  }
}

Cтало:

def fail(e: Exception) {
  lock.lock()
  try {
    res match {
      case Left(list) if list.size < totCnt => {
        res = Right(ex)
        done.signalAll()
        for (lsnr <- failLsnrs)
          lsnr(ex)
      }
      case _ => return
    }
  }
  finally {
    lock.unlock()
  }
}

Существенно улучшилась и структура метода get - теперь он сначала дожидается одного из завершённых состояний, потом предпринимает некие действия. Раньше он вынужден был выполнять две по-сути параллельные проверки во время ожидания и по-разному из него выходить.

Было:

def get: JavaCollection[T] = {
  lock.lock()
  try {
    while (doneCnt != totCnt) {
      if (ex != null)
        throw ex
      done.await()
    }

    res
  }
  finally {
    lock.unlock()
  }
}

Cтало:

def get: JavaCollection[T] = {
  lock.lock()
  try {
    while (res.left.toOption.exists(_.size < totCnt))
      done.await()

    res match {
      case Left(list) => list
      case Right(ex) => throw ex
    }
  }
  finally {
    lock.unlock()
  }
}

Метод listen в принципе изменился так же как get, так что я не буду приводить его код. Вот окончательная версия.

Я вижу следующие явные улучшения:

1. Вместо 3х скоординированных значений - одно, содержащее результат. Состояний по-прежнему три, но способ их кодирования радикально упростился.
2. Методы add, fail, listen изменили форму. Раньше они более а в основном менее явно предполагали некую историю изменений и обновляли состояние на основе этих предположений. Теперь методы инспектируют текущее состояние, и устанавливают новое на его основе. Практически конечный автомат.
3. Метод get сначала неявно ожидал нужного состояния одной из двух переменных (ex и size), "произошло исключение или количество результатов сравнялось с ожидаемым". Теперь этот метод явно ожидает пока нарушится одно условие, "существует неполный список результатов". Точек принятия решения в методе изначально было две, стала одна.

Одно общее следствие: класс стало возможно анализировать и читать более мелкими шагами, по одному методу. При этом пространство возможных состояний уменьшилось лишь немного, изменилось лишь их представление и интерпретация.