refactor: use "for of" loop in the plugin folder#896
Conversation
Prefer the use of for-of loop over the standard for loop where possible. This simplifies the syntax and make the code easier to read.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughTraditional indexed for-loops were replaced with modern for-of loops across three core plugin files. Variable names within these loops were clarified for readability. No logic, control flow, or public API changes were introduced; the updates focus solely on iteration syntax and code clarity. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
ℹ️ Converted to Draft, I am going to add some tests |
|
| const state = new CellState(graph.view, cell, {}); | ||
| graph.view.getState = () => state; | ||
|
|
||
| const dict = new Map<Cell, CellState>(); |
There was a problem hiding this comment.
nitpick: rename dic to cellStates for clarity. We are not using a Dictionary type (which has been removed) and it is clearer to name the variable with the context it holds instead of using a name related to a type.
NOTE: this applies to all tests in this file
| * var changes = evt.getProperty('edit').changes; | ||
| * var nodes = []; | ||
| * var codec = new Codec(); | ||
| * model.addListener(mxEvent.CHANGE, function(sender, evt) { |
There was a problem hiding this comment.
nitpick: mxEvent does not exist, this is a mxGraph value
NOTE: this applies to all references in this file
| * @param cell | ||
| * @param dict | ||
| */ | ||
| addStates(cell: Cell, dict: Map<Cell, CellState>) { |
There was a problem hiding this comment.
nitpick: rename parameter, see remark about the dic variable
|
|
||
| /** | ||
| * Suspends the livew preview. | ||
| * Suspends the live preview. |
There was a problem hiding this comment.
suggestion:
| * Suspends the live preview. | |
| * Resumes the live preview. |
| for (let i = 0; i < parents.length; i += 1) { | ||
| if (this.shouldRemoveParent(parents[i])) { | ||
| temp.push(parents[i]); | ||
| for (const aParent of parents) { |
There was a problem hiding this comment.
suggestion: use a filter method on the original array (add tests before doing the refactoring)
| const childCount = cell.getChildCount(); | ||
|
|
||
| for (let i = 0; i < childCount; i += 1) { | ||
| count += this.addStates(cell.getChildAt(i), dict); |
There was a problem hiding this comment.
todo: add tests here and for all methods where using direct loop instead of using cell.getChildCount and cell.getChildAt
| const points: Point[] = []; | ||
|
|
||
| if (geometry?.points) { | ||
| const geometryPoints = geometry.points; | ||
| for (const point of geometryPoints) { | ||
| point && points.push(new Point(point.x + dx / s, point.y + dy / s)); |
There was a problem hiding this comment.
suggestion: use a filter and map methods on the original array
| @@ -1552,17 +1555,17 @@ class SelectionHandler implements GraphPlugin, MouseListenerSet { | |||
| // Collects all non-selected parents | |||
| const dict = new Map<Cell, boolean>(); | |||
There was a problem hiding this comment.
Rename to detectedParents or foundParents or nonSelectedParents, ...
| let graph: AbstractGraph; | ||
| let plugin: SelectionHandler; | ||
|
|
||
| beforeEach(() => { |
There was a problem hiding this comment.
Add afterEach to destroy the graph and release resources
|
Prefer the use of for-of loop over the standard for loop where possible.
This simplifies the syntax and make the code easier to read.
Tasks
Summary by CodeRabbit