-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[refactor] Improve code structure #347 #348
base: master
Are you sure you want to change the base?
[refactor] Improve code structure #347 #348
Conversation
Refactored src/js/netjsongraph.js Fixes openwisp#347
a239c8f
to
fe99e6e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @dee077! 👍
I have two minor requests for improvement regarding comments below.
class NetJSONGraph { | ||
/** | ||
* @constructor | ||
* Initializes a new NetJSONGraph instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any programmer should know that a constructor initializes a new instance of the class, I think this comment can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Initializes a new NetJSONGraph instance. |
Please remove this. Not useful.
"block"; | ||
} | ||
}; | ||
if (this.config.switchMode && this.type === "netjson") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a few more comments in this method would help to make it more readable.
What's the two cases being dealed here? One is canvas and the other SVG or what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both render canvas element. The graph renders only one canvas element, and the map renders two canvas elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code comment is not useful, what you wrote in the github comment is more useful, why does it draw two canvas elements? What are the two canvas elements doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please read https://stackoverflow.blog/2021/12/23/best-practices-for-writing-code-comments/
I want to underline:
Rule 1: Comments should not duplicate the code.
The point is to explain what the comment achieves from a functional perspective or from the user perspective, eg: this code block implements the switching from geographic map to topology chart.
Rule 3: If you can't write a clear comment, there may be a problem with the code.
Rule 4: Comments should dispel confusion, not cause it.
Rule 5: Explain unidiomatic code in comments.
Rule 8: Add comments when fixing bugs.
Rule 9: Use comments to mark incomplete implementations.
And I would add: use comments when implementing workaround for issues, eg:
// This code works around a nasty bug in component X, TODO: solve the root cause,
// github issue link https://github.com/openwisp/.........
"block"; | ||
} | ||
}; | ||
if (this.config.switchMode && this.type === "netjson") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code comment is not useful, what you wrote in the github comment is more useful, why does it draw two canvas elements? What are the two canvas elements doing?
class NetJSONGraph { | ||
/** | ||
* @constructor | ||
* Initializes a new NetJSONGraph instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Initializes a new NetJSONGraph instance. |
Please remove this. Not useful.
Thanks for the clarification! I'll update the comments accordingly. |
return { | ||
...config, | ||
render: | ||
config.render === "map" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dee077 & @nemesifier
I found this in the readme
In extreme cases, you can also pass your own render function if you don't want Echarts to render. We will pass in the processed netjson data and netjsongraph object.
https://github.com/openwisp/netjsongraph.js?tab=readme-ov-file#configuration-instructions
I think, config can be undefined and user can pass custom render function so this need to be handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Left few comments.
Refactored
src/js/netjsongraph.js
Fixes #347
Checklist
Reference to Existing Issue
Closes #347.
Description of Changes
Refactored
src/js/netjsongraph.js