Task #5769
closedCode review
Added by Alexander Kamkin over 9 years ago. Updated about 8 years ago.
0%
Description
Code review is required. Comments are awful.
Updated by Alexander Kamkin over 9 years ago
- Target version changed from 2.1 to 2.2
Updated by Andrei Tatarnikov over 9 years ago
Code review on MmuIterator:
- The class header does not contain any information on the purpose of the class.
- 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;
- 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.
- Field "n" too genetic name.
Why not to give it a more meaningful name instead of writing a comment?/** The number of execution in template. */ private int n;
- What does "n - 1" mean? It is used everywhere. Why not to create a method or variable named with a meaningful name?
- Why constant fields are not marked as "final"?
- Most of method commends do not give any information that would help inderstanding
- 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); }
- 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".
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
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.
Updated by Alexander Kamkin over 9 years ago
- Assignee changed from Andrei Tatarnikov to Alexander Protsenko
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
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.