Project

General

Profile

Actions
Copy link

Task #5639

closed

Code review notes (package ru.ispras.unitesk.processor.test.mmu)

Added by Andrei Tatarnikov almost 10 years ago. Updated about 8 years ago.

Status:
Closed
Priority:
High
Assignee:
Alexander Protsenko
Category:
MMU Plugin
Target version:
2.2
Start date:
02/17/2015
Due date:
% Done:

70%

Estimated time:
Detected in build:
svn
Published in build:

Description

Общие замечания
  1. Все неизменяемые поля классов (которые присваиваются единыжды в конструкторе или объявлении) должны быть final.
  2. В методах, изменяющих сосотояние объекта, вида addXXX и setXXX должны быть проверки на то, что передаваемые им параметры валидны. В первую очередь не равны нулю. А то после их добавления в коллекцию, будем долго искать откуда пришли невалидные значения.
  3. То же самое (п. 2) касается и конструкторов.
  4. В контексте (п. 1) обратить внимание на утилитные методы Collections.unmodifiableList, Collections.unmodifiableMap и т.д. Они создают обертки, гарантирующие, что состояние коллекций будет неизменным.
  5. Обратить внимание на метод toString. Он может очень помочь при отладке. Он сейчас нигде не реализован.
  6. Из-за того, что позволяется создавать невалидные объекты (п. 2 и 3), везде куча проверок на null (ненужных), что затрудняет понимание кода.
  7. Почему столько много mutable классов? Они создаются и инициализуруются только один раз. Насколько я вижу, их дальнейшее использование не предполагает изменение состояния. Объект создается постым (или попросту невалидным), а затем инициализируется методами setXXX, addXXX. А конструкторы, фабричные методы, паттерн Builder не используются. А можно было бы сразу создать хороший, валидный объект, неизменяемый объект и не думать где он может измениться или могут ли его методы вернуть null, пустую коллекцию или другие невалидные данные.
  8. Использование null для специальных случаев - не самая лучшая практика. Для пользователей класса может быть неочевидно что значит null. Может, имеет смысл сделать MmuGuard.GOTO или MmuGuard.UNCONDITIONAL. 
    /** The guard condition or {@code null} if the transition is interpreted as {@code goto}. */
private MmuGuard guard;
Класс Mmu
  1. Метод getExecutions
    • Суффикс List в названиях переменных ИМХО избыточен. В названиях переменных-членов мы его не используем. Тип переменной и так виден, особенно если метод короткий. Это на заметку.
    • Здесь удобнее сделать return сразу и не городить городушку из вложенных блоков: 
      if (transitionList != null && !transitionList.isEmpty()) {

      И вообще в каком случае это проиcходит? Такое состояние валидное? Почему мы его просто игнорируем?
  • Класс MmuAction
  • Реализация метода equals: 
    @Override
public boolean equals(final Object o) {
  if (o == null || !(o instanceof MmuAction)) {
    return false;
  }

  final MmuAction r = (MmuAction) o;
  return name.equals(r.name);
}
    Нет проверки o == this (стандартное сравнение). В данном случае это мелочь, а в более сложных объектах - это накладные рассходы.
  • Класс MmuConflict
    1. А количество возможных конфликтов конечно? А почему так, а не через enum: 
      // Address no equal.
public static final String NO_EQUAL = "NoEqual";
// Address equal.
public static final String EQUAL = "Equal";
// Index1 != Index2.
public static final String INDEX_NO_EQUAL = "IndexNoEqual";
// Index1 == Index2 && Tag1 != Tag2.
public static final String TAG_NO_EQUAL = "TagNoEqual";
// Index1 == Index2 && Tag1 != Tag2 && Tag1 == Replaced2.
public static final String TAG_REPLACED = "TagReplaced";
// Index1 == Index2 && Tag1 != Tag2 && Tag1 != Replaced2.
public static final String TAG_NO_REPLACED = "TagNoReplaced";
// Index1 == Index2 && Tag1 == Tag2.
public static final String TAG_EQUAL = "TagEqual";
Actions
Copy link

Also available in: Atom PDF