Task #5639: Code review notes (package ru.ispras.unitesk.processor.test.mmu) - MicroTESK - Open-Source Projects

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

Общие замечания

Все неизменяемые поля классов (которые присваиваются единыжды в конструкторе или объявлении) должны быть final.
В методах, изменяющих сосотояние объекта, вида addXXX и setXXX должны быть проверки на то, что передаваемые им параметры валидны. В первую очередь не равны нулю. А то после их добавления в коллекцию, будем долго искать откуда пришли невалидные значения.
То же самое (п. 2) касается и конструкторов.
В контексте (п. 1) обратить внимание на утилитные методы Collections.unmodifiableList, Collections.unmodifiableMap и т.д. Они создают обертки, гарантирующие, что состояние коллекций будет неизменным.
Обратить внимание на метод toString. Он может очень помочь при отладке. Он сейчас нигде не реализован.
Из-за того, что позволяется создавать невалидные объекты (п. 2 и 3), везде куча проверок на null (ненужных), что затрудняет понимание кода.
Почему столько много mutable классов? Они создаются и инициализуруются только один раз. Насколько я вижу, их дальнейшее использование не предполагает изменение состояния. Объект создается постым (или попросту невалидным), а затем инициализируется методами setXXX, addXXX. А конструкторы, фабричные методы, паттерн Builder не используются. А можно было бы сразу создать хороший, валидный объект, неизменяемый объект и не думать где он может измениться или могут ли его методы вернуть null, пустую коллекцию или другие невалидные данные.
Использование null для специальных случаев - не самая лучшая практика. Для пользователей класса может быть неочевидно что значит null. Может, имеет смысл сделать MmuGuard.GOTO или MmuGuard.UNCONDITIONAL.
```
/** The guard condition or {@code null} if the transition is interpreted as {@code goto}. */
private MmuGuard guard;
```

Класс Mmu

Метод 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

А количество возможных конфликтов конечно? А почему так, а не через 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

Updated by Alexander Kamkin over 9 years ago

Priority changed from Normal to High
Target version set to 2.1

Actions

Copy link

Updated by Alexander Protsenko over 9 years ago

Status changed from New to Open

Andrei Tatarnikov wrote:

Класс Mmu

Метод getExecutions

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

Поправил.

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

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

Класс MmuAction

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

Добавлено.