Project

General

Profile

Task #5639

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

Added by Andrei Tatarnikov over 4 years ago. Updated almost 3 years ago.

Status:
Closed
Priority:
High
Category:
MMU
Target version:
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";
      

History

#1

Updated by Alexander Kamkin over 4 years ago

  • Priority changed from Normal to High
  • Target version set to 2.1
#2

Updated by Alexander Protsenko over 4 years ago

  • Status changed from New to Open

Andrei Tatarnikov wrote:

Класс Mmu
  1. Метод getExecutions
    • Суффикс List в названиях переменных ИМХО избыточен. В названиях переменных-членов мы его не используем. Тип переменной и так виден, особенно если метод короткий. Это на заметку.

Поправил.

  • Здесь удобнее сделать return сразу и не городить городушку из вложенных блоков:
    [...]
    И вообще в каком случае это проиcходит? Такое состояние валидное? Почему мы его просто игнорируем?

Если при создании переходов их количество 0 и передается null нам ничего с этим делать не надо.
Вообще возможно, если, например, модель из которой получаем переходы: пустая.

  • Класс MmuAction
  • Реализация метода equals:
    [...] Нет проверки o == this (стандартное сравнение). В данном случае это мелочь, а в более сложных объектах - это накладные рассходы.

Добавлено.

  • Класс MmuConflict
    1. А количество возможных конфликтов конечно? А почему так, а не через enum:
      [...]

Создано через enum.

#3

Updated by Alexander Protsenko over 4 years ago

Исправлен п.2.

#4

Updated by Alexander Kamkin over 4 years ago

  • Category set to Iterator
#5

Updated by Alexander Protsenko over 4 years ago

  • % Done changed from 0 to 40

r362

Исправлен п.1, п.5.

#6

Updated by Alexander Kamkin over 4 years ago

  • Target version changed from 2.1 to 2.2
#7

Updated by Alexander Protsenko over 4 years ago

  • % Done changed from 40 to 70

Исправлен п.7, все Сеттеры убраны.

#8

Updated by Alexander Kamkin about 4 years ago

  • Project changed from MicroTESK for MIPS64 [MMU] to MicroTESK
  • Category changed from Iterator to MMU
  • Target version changed from 2.2 to 2.2
#9

Updated by Andrei Tatarnikov almost 3 years ago

  • Status changed from Open to Closed

Partially done. Everything else is outdated.

Also available in: Atom PDF