Project

General

Profile

Actions

Task #5769

closed

Code review

Added by Alexander Kamkin over 9 years ago. Updated about 8 years ago.

Status:
Rejected
Priority:
Normal
Category:
MMU Plugin
Target version:
Start date:
03/27/2015
Due date:
% Done:

0%

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

Description

Code review is required. Comments are awful.

Actions #1

Updated by Alexander Kamkin over 9 years ago

  • Target version changed from 2.1 to 2.2
Actions #2

Updated by Andrei Tatarnikov over 9 years ago

Code review on MmuIterator:

  1. The class header does not contain any information on the purpose of the class.
  2. Lots of unnecessary ("noise") comments. If the variable name does not reflect its purpose it better to find a better name? In this case comments duplicate the variable name.
    /** The Memory Management Unit (MMU). */
    private Mmu mmu;
    /** The dependency indexes. */
    private int[][] dependencyIndexes;
  3. Static method "getDependencies". Why is it in MmuIterator? Looks like a utility function. You can create a static factory method in MmuDependency and place this code there.
  4. Field "n" too genetic name.
    /** The number of execution in template. */
    private int n;
    Why not to give it a more meaningful name instead of writing a comment?
  5. What does "n - 1" mean? It is used everywhere. Why not to create a method or variable named with a meaningful name?
  6. Why constant fields are not marked as "final"?
  7. Most of method commends do not give any information that would help inderstanding
  8. Why not to use the "do { ... } while (...)" construct?
    this.template = new MmuTemplate(templateExecutions, templateDependencies);
    
        while (!MmuSolver.checkConsistency(template) && hasNext) {
          if (!nextDependencies()) {
            nextExecution();
          } else {
            assignRecentDependencies();
          }
    
          this.template = new MmuTemplate(templateExecutions, templateDependencies);
        }
    
  9. Use of class variables. There three types of class variables: (1) constants (do not change), (2) state variables (describe the current state of the object and serve as a basis for the new state which will be calculated on next call of a public method), (3) temporary variables that will be forgotten as soon as the objects receives its next state. Variables (3) - should be passed as arguments between methods. This helps to describe dependencies between methods.

P.S. Regarding the use of arrays. It is OK in this code, but take into account "Effective Java - Item 25: Prefer lists to arrays".

Actions #3

Updated by Andrei Tatarnikov over 9 years ago

Notes on comments. Examples:

/**
 * Assigns the recent dependencies.
 */

This is create form the method name that it "присваивает последние зависимости". But it does not answer the following questions:

Куда? Зачем? Каким образом? Какие вычисления? На основе каких данных элементов состояния? Какие переменные состояния поменяются?

This applies also to methods:

  • nextExecution
  • nextDependencies
  • assignExecutions
  • assignRecentDependencies()
  • assignDependency
Actions #4

Updated by Andrei Tatarnikov over 9 years ago

10. Assignments to this.template. Take the code:

@Override
  public void next() {
    if (!nextDependencies()) {
      while (!nextExecution() && hasNext) {
        // if dependencies not solved, then increment the iterator.
      }
    } else {
      assignRecentDependencies();
    }

    this.template = new MmuTemplate(templateExecutions, templateDependencies);

    while (!MmuSolver.checkConsistency(template) && hasNext) {
      if (!nextDependencies()) {
        nextExecution();
      } else {
        assignRecentDependencies();
      }

      this.template = new MmuTemplate(templateExecutions, templateDependencies);
    }
  }

This line of code is repeated twice in this method + it presents in the nextExecution method:

this.template = new MmuTemplate(templateExecutions, templateDependencies);

Question: Why? It depends on the latest state of templateExecutions and templateDependencies. There are no return statements in the method. So it would be enough to assign it in the end of the method. When you assign it several times it looks like code between the assignments depends on the previously assigned value.

Actions #5

Updated by Alexander Kamkin over 9 years ago

  • Assignee changed from Andrei Tatarnikov to Alexander Protsenko
Actions #6

Updated by Alexander Kamkin over 9 years ago

  • Project changed from 161 to MicroTESK
  • Category changed from Iterator to MMU Plugin
  • Target version changed from 2.2 to 2.2
Actions #7

Updated by Andrei Tatarnikov about 8 years ago

  • Status changed from New to Rejected

The issue is outdated. Most likely, the code is already totally rewritten.

Actions

Also available in: Atom PDF