6
is there a better way to achieve this functionality?
You can use Function.bind() to create a function with the this context bound to Wyg.Editor:
const node = document.getElementById('myFavoriteNode');
node.addEventListener('click', Wyg.Editor.nodeClickedEvent.bind(Wyg.Editor));
See this demonstrated in the snippet below.
Notice that const was used ...
6
I understand semicolons in JavaScript are optional, however I believe you should always use them. It's easy to have something break later on because you neglected a semi-colon and it lost the whitespace which saved it before in a compression/generation/eval parse.
You could declare the timePerShot as a variable, to ensure the amount is not being calculated ...
5
There is not much I would add to dustytrash's answer:
prefer const over let when a variable does not change
use blank lines only for creating logical regions; to me, an assignment to an instance variable is not a logical region
reload (startTime = 0) {
const whenCanFireAgain = startTime + this.reloadSpeed;
this.bulletsLeftInMagazine = this....
4
OO-Design
key question Identify how and where I can typically use OOP more in my job (as I can usually just use arrays and don't need objects
In your transformation towards OOP, you're still in the habit of resorting to arrays.
You let each new instance of this class be registered in some global array.
class Animal {
constructor (name, description) {
...
4
Accessing statics via this
Defining the object reference outside the object just to gain access via a miss used accessor (this) is a hack and not how to use static objects to handle events.
Binding objects to a function
First what you did could have been a little less complicated
You had something like
class Editor {
static nodeClick(event){
...
4
The main suggestion I have is to take the values from sitesToBlock and put them in the @match metadata key instead of detecting when to update the document content. There is an extensive guide about match patterns here. With this approach there should not be any need to wait for the DOM to be loaded and check if the URL contains elements in sitesToBlock, ...
4
I'm trying to address the same issue.
What I'm currently doing is splitting my big hooks into smaller hooks, each with a single responsability.
For example, in your example, you could split your onKeyDown, rotateRight and rotateLeft functions into a hook which takes a state with values and setters as only parameter:
const useCarRotation = state => {
...
2
The getter method Weapon.timePerShot will return 1 / this.fireRate. There doesn't appear to be anything to prevent fireRate from being 0. If that is the case, Weapon.timePerShot would return Infinity, which would lead to whenCanFireAgain also being set to Infinity. In some programming languages dividing by zero would lead to an exception being thrown or a ...
2
lineHandler
Prefer const over let when a variable gets assigned only once.
function lineHandlerOP(line) {
const id = Number(line.slice(0, 4));
const index = Number(line.slice(8, 10));
const maxIndex = Number(line.slice(12, 14));
const msg = line.slice(16, line.length);
return [id, index, maxIndex, msg];
}
Your implementation is very ...
2
Overall the code looks fine. I just have a couple suggestions about the JavaScript and CSS.
JS
querySelectorAll vs getElementsByClassName
Generally document.getElementsByClassName will be quicker than document.querySelectorAll (see this post for more information) and the former also returns a live HTMLCollection so it wouldn't need to be queried each time....
2
is there a better way to achieve this functionality?
You might use a wrapping function:
let node = document.getElementById('myFavoriteNode');
node.addEventListener('click', event => Wyg.Editor.nodeClickedEvent(event));
// or, using regular function expression
node.addEventListener('click', function(event) {
return Wyg.Editor.nodeClickedEvent(event);
}...
1
This code is quite repetitive, and doesn’t adhere to the Don't Repeat Yourself principle.
One way to DRY it out is to abstract the latter promise callback to a separate function:
const addTrackAndBoundsFromKml = kmltext => {
// Create new kml overlay
const parser = new DOMParser();
kml = parser.parseFromString(kmltext,"text/xml");
...
1
I have mainly two things I'd like to look at:
I'm not a big of fan of the constant deconstructing and recreating of Sets in the loop, especially since this most likely has negative impact on the performance. Unfortunately because there is not direct way to add multiple values to a Set there is no functional "one line" solution. My function to do this would ...
1
The first thing I noticed is that the format of the data is a collection of columns with rows. The variable maxNumCols seems mis-leading. A more appropriate name for that variable would be maxNumRows because it describes how many rows are needed.
In terms of reducing the transformations needed, one option to consider is finding the maximum number of rows in ...
Only top voted, non community-wiki answers of a minimum length are eligible
