New answers tagged javascript
0
Preface
I used AngularJS a few years ago but didn't get into Angular2+ so my knowledge of it is slim-to-none. I do however have a fair amount of familiar with Javascript and various frameworks.
Question responses
Security Isn't the transmission of the password in plaintext a security issue?
I found posts about this question on multiple SE sites. For ...
1
Prefer const over let and var. When you declare a variable with let, you're indicating to any reader of the code that the variable may be reassigned at any time. When the reassignment isn't actually necessary for the code to work, this results in undesirable cognitive overhead - it may be a constant worry in the back of one's mind "This variable name ...
2
Generally I find the code very unstructured making reading it a bit confusing:
First you execute some code outside of a function to read the data from a file, then you define a constant (dublinOffice), then you define a variable (invitedArr), but don't use it in the next block of code, then you sort the data, then you define two functions, and finally ...
4
Indentation Much of the indentation in the HTML, CSS, and JS is inconsistent, which makes the code moderately harder to read than would be ideal. Consider using an IDE which automatically formats code properly so things can be made readable without having to mess with it manually.
Duplicate IDs are invalid HTML You have two elements with div id="egg&...
6
Welcome to Code Review. The CSS looks quite good. I haven’t seen too many style sheets with color lightsalmon 😃. Below are some suggestions.
Cache DOM references
”...DOM access is actually pretty costly - I think of it like if I have a bridge - like two pieces of land with a toll bridge, and the JavaScript engine is on one side, and the DOM is on the ...
6
Variables
Always use a variable declaration when first declaring your variable. If you don't your variable will be put into the global scope, which is almost never what you want. It also makes it more clear that you don't want to re-assign to an already existing global variable.
So for starting_day, for example, put a var, let, or const in front of it.
HTML ...
0
To get into best practices would require volumes and get you plenty of opinion. At any rate, trying to program too far beyond your understanding is no good so I won't get into particulars. In general, the first thing that occurs to me is that if you're really serious about programming, be good to your future self. Programming starts as a thought process ...
3
The JavaScript unnecessarily contains information that belongs in the style sheet. It would be much easier to have the JavaScript simply set/remove a class (for example, on the body) and put the styles into the style sheet.
function openav() {
document.body.classList.add("open-nav");
}
function closenav() {
document.body.classList.remove(&...
4
If possible, I'd change the GroupColumns to be an array instead. That way, all you need to do is use .includes to see if an item is included in it. Other improvements can be:
Let TS automatically infer types when possible. You only need to denote the type of a variable or the type of a return value when TS can't do so on its own. TS can automatically ...
4
Other answers have covered the algorithm so this answer will cover other aspects of review.
Good things
Strict equality comparisons are used
indentation is consistent
variables have limited scope
Bug with object length
Let’s look at that first function:
function countA(str) {
return (str.match(/a/g) || {}).length;
}
String.prototype.match() returns “An ...
12
The most important insight that can be made here is that when there are as to split by, it's only the span of text between the end of the first match and the start of the last match that matter, and the rest is a simple counting problem based on the substring lengths. Look at a few examples.
Input:
babaa
ba // This is segment between the end of the first ...
6
Your algorithm is cubic against the length of the string, as it uses nested for loops that each run proportionally to the length of the string with a linear time countA function inside them. But it is possible to achieve a linear time algorithm.
Note first that you will always be putting the same letter a characters in each split regardless of the split ...
0
Looks good!
I would say maybe the following styles would be just a bit more readable, easier to follow.
We can also alter l1 and l2 with more descriptive variable names.
Line Counting Fallacy:
Sometimes, Line/character countings are helpful for command line languages/scripts (awk, grep, sed, regex, etc.) or maybe Code Golfing, is not a JavaScript ...
3
Overall Your code is quite reasonable, after fixing something that looks to be a typo. The transformation that needs to be applied to the data structure isn't entirely trivial, so the logic that needs to be implemented requires a handful of lines of code.
Typo? But your code also has a typo/bug: the map.set(phone, r.name); will set the value in the map to be ...
6
Async login bug The try/catch around User.findOne does not accomplish anything because findOne is asynchronous. When findOne fails, it'll pass an error to the callback, but when the callback does throw err, nothing is there to catch the asynchronous error, so no response will be sent to the user. Another issue is that you aren't checking if user exists - if ...
5
Error handling You await inside an async function, but without a try/catch surrounding it. So, if Article.findAndCountAll rejects, you'll get an unhandled rejection (which is deprecated in Node) and no response will be sent.
Default destructuring and variable names
const { page } = req.params; // current Page
const { size } = req.params; // items per page
...
3
Consider fetch In modern browsers, which your code is guaranteed to be running in, fetch is usually a better choice than XMLHttpRequest - fetch is Promise-based (Promises are usually a bit nicer to work with than callbacks), its API is a bit cleaner to read and write, and it's a bit more concise.
Error handling The front-end has no error handling. If the ...
4
Performance The main optimization that can be made here is to, while iterating over the sorted houses, to inch along the heaters one-by-one while testing their distance. Increment a heater index only if the difference between the house and the next heater is less than the difference between the house and the current heater. This way, whenever a heater is ...
5
Prefer modern syntax You're using a few ES2015+ features already, including async/await. If you're going to write a script containing modern syntax (which you should - it can make code much easier to read and write), best to use it everywhere. In particular:
Use const (or let when you need to reassign) instead of var. var has too many problems to be worth ...
3
I agree it doesn't make sense to use promises with event-based programming and using observables is a good solution.
On another note, it is recommended to default to using const instead of let for all variables as it can cause bugs. When you determine re-assignment is necessary (mostly for loop/iterator variables) then use let.
Another suggestion is to use a ...
3
You should use observables and the myriad of functions available for them. It's called reactive programming, but I think it might fall into the category of functionalish.
An example from memory:
import { fromEvent, takeUntil } from 'rxjs';
...
const eventListener = document.addEventListener('serviceChosen', () => console.log('chosen'));
const ...
4
First of all, I would not use Promises to do event based programming.
It is not used like that, so your code will get harder to follow.
Also, I would advise you to split up your functions more and omit some comments that way.
//// MAIN ///////
let services = [
{ url: "x", label: "1" },
{ url: "y", label: "2" },
{ url: "z", label: "3" },
];
...
1
Overall Feedback
This code seems to work acceptably, though the semantics of "old - new" seems reversed.
The first bullet in the notes states:
I organize the JavaScript arrays with the youngest at the top and oldest at the bottom (with dates, it would look like this: oddDate = ['Oct. 1', 'Oct. 2', 'Oct. 3', 'Oct. 4', 'Oct. 5']; )
Yet the third ...
3
Bug: no assignment when calling filter
There are places where Array.filter() is called but not assigned to anything.
Array.from(links).filter(link => Scraper.internalLinks.add(link.getAttribute('href')));
and this in addEmails()
Array.from(emailsInDom).filter(el => {
if(el !== null) {
el.filter(el => this.emails.add(el))
}
});
...
4
I choose to focus on the Single Responsibility Principle in this answer.
To move closer to the Single Responsibility Principle, I think some of the functionality needs to be moved around. I have decomposed the class and removed all state that is shared on the class (rhetorical question: is the class responsible for state or functionality? If it is "both&...
1
Based on @CertainPerformance's feedback, here is where I landed.
This is the most DRY version possible.
const chartSettings = (gridInterval, data) => {
if (isEmptyObject(data)) return;
const ticks = {
yTopTick: Math.ceil(+data.yMax / gridInterval) * gridInterval,
yBottomTick: Math.floor(+data.yMin / gridInterval) * gridInterval,
xTopTick: ...
3
Since you have a reasonable number of variables that all relate to one particular thing (chart settings), rather than having lots of standalone variables, you might consider using a single object instead. Regardless of any other issues with the code, I'd consider a single object to be slightly more appropriate than multiple similar standalone variables ...
3
Edge cases
In the real world, input isn't always well-formed. What happens if there is a typo in the data? For example:
const data = [
"(2) 2020-09-15", "(3] 2020-09-16"
];
This will lead to undefined appearing in the output for the dates. In other cases/frameworks/languages an exception might be thrown that could crash your script/...
1
I would use something like this:
const data = [
"(2) 2020-09-15", "(3) 2020-09-16"
];
let pairs = [];
const regex = /^\((\d+)\)\s+(\d{4}-\d{2}-\d{2})$/;
for (let datum of data) {
// Idea from the previous answer:
let pair ...
4
Rather than declaring the variables with let (you should always prefer const) and concatenating and reassigning, consider creating arrays of numbers and dates instead, eg:
[2, 3] and ["2020-09-15", "2020-09-16"]
Then after the loop is done, join all elements by newlines.
For the regular expression, rather than split, I think match would ...
2
Stray console You have window.console.log( this.getAttribute( 'data-pos' ) );. They're useful for debugging, but remember to remove logs for production. Consider no-console.
n A variable named n is not all that informative. How about calling it totalNumberOfTiles instead? Also, rather than
for ( let i = 0; i < n; i++ ) {
initialTileArray.push( String( i + ...
1
Narrow the requestMethod Its type should be narrower. There are only a small number of possible methods. Instead of string, you should permit only those which make sense. You probably want something like:
requestMethod?: 'GET' | 'POST' | 'PUT' | 'DELETE' // etc
This will both reduce typos (eg, prevent someone from accidentally using 'PUST') and make it ...
3
DRY your code You reference element[filterField] quite a few times in filterCallback. It would be nicer to save the value in a standalone variable first - then you can reference that variable instead of doing object property lookup each time.
Nested object bug filterCallback returns a function, but here, if an array or object is found, you're returning a ...
2
An array is perfectly fine, it's what I'd prefer as well. But there are some improvements you can make:
When you have an array and you want to create a new array by transforming every item of an old array, it's most appropriate to use Array#map:
// I'd call it `filenames` and `audios`, not `sources` and `sounds`;
// `audios` is plural of Audio and is a bit ...
1
Question Responses
Using and tracking this is confusing at times. Have I been over-the-top in my use of this?
I don't feel it is "over-the-top" though storing a reference to this in another variable is a sign that context isn't bound properly. Bear in mind that an arrow function "Does not have its own bindings to this or super, and should ...
2
This review will focus on what object oriented programming [oop] is and how we can use two concepts of oop (encalpsulation and abstraction) in your code to make it more object oriented.
The self-drawn pictures of this review come from my Github-Repository for a presentation that is supposed to show the basics of oop.
How to write Object-Oriented Code
Object-...
1
Overall Remarks
I must admit that this code makes use of more bitwise operators than I normally see in JavaScript. Nonetheless it looks to be sophisticated. There is a lot of code so the remarks here may not be comprehensive but I’ll cover what I can.
Question Responses
Is Object.freeze() the normal way to implement enum logic in JavaScript?
That seems to ...
2
Always use const when possible if you're going to write in ES6+; only use let when you need to reassign a variable
Consistent spacing You have lines like: [0].value) *66 this.events[tickIndex +indexAdjust +1] stopRecordBuffer+(this.recordingMultipliers Code is easiest to read when operators have a space between the operator and its operands. Consider using ...
2
This solution may not be appropriate for your project depending on how else commands are used in your codebase, or if the commands are provided by an external library, among other things. But based on the snippet you give here, it might be a more suitable way to structure your code.
Let's start by reviewing two key elements of your code here: commands and ...
1
The premise of your question indicates that you've identified one of the flaws of promises.
Asynchronous code like this should return an observable which you can then choose to convert to a promise. When it's an observable you can use the full range of operators available in RxJs, make the changes you need in that pipeline, and then finish off with a ...
5
You could do like so if you prefer, essentially just pass forward the data i the promises. You could write your own promiseAllObject if you want it to be slightly neater.
const result = getMovies()
.then(movies => {
return Promise.all([
Promise.all(movies.map(({ movieId }) => getTitle(movieId))),
movies.map(_ => _.rating)
]);
...
2
First, your code seems globally ok, and much of what I'll write here are details. Also of course, it does reflect my personnal opinion. While I try not to discuss points about my personnal opinion, but more generic matters, my personnals opinion may impact what I consider generic matters or not.
About comments
Far from me the idea to discourage you to ...
7
My answers will comport several steps. Some steps won't be present anymore in the final answer, but I mention them because you might find interesting to follow that in other contexts.
Multiple usage of the same reference to command.parameters[0], command.parameters[1], ...
You are using command.parameters[0] literally everywhere. If what you're writing look ...
3
Modules You're already using export syntax, so you're currently in a module. Rather than defining multiple IIFEs, you might consider using separate sub-modules:
// fadeInHeadings.js
// Renaming from headingFadeIn to fadeInHeading for readability
function fadeInHeading(heading) {
if (!heading) return;
const parent = heading.parentNode.parentNode;
...
8
Spread Similar to Mohammed's answer, but without mutation and with spread syntax, which I find easier to read than .apply, you can do something like:
const executeModeration = async (command, channel) => {
await moderation[command.command](...command.parameters);
}
Default parameters
You use a number of conditional operators when calling functions, ...
3
Condition bug Your current implementation has a bug. Your lines like these have the problem:
(secondaryStatus === 'payment1' || 'payment2' || 'payment3')
JavaScript only has unary operators, binary operators, and a single ternary operator. All the operators used above are binary operators; two expressions will be evaluated into a single expression until a ...
1
Date sorting Your dates are of the format YYYY-MM-DD. You currently sort them with:
(a, b) => b.date > a.date ? 1 : b.date === a.date ? 0 : -1
When you want to lexiographically compare strings to determine which comes first in the alphabet for sorting, you can do it more concisely with localeCompare, which returns a number indicating whether one ...
5
Like Mohammed suggested, Function.apply() could be used to replace the switch statement.
Variable naming
If the first argument ‘command‘ is an object that contains a string at key ‘command‘ then a better name would be in line for readability- e.g. ‘options‘.
Argument use
Because Ecmascript-2015 (A.K.A. ES-6) features like arrow functions are used (as well as ...
7
I'm not a JavaScipt programmer, so my suggestions are limited
Is there any obvious beginner flaws?
Using a large case statement like this is a beginners flaw, because it is difficult to maintain (expansion and contraction are difficult) and performance could be improved. In C++ I would use a map of keywords to functions and I would do something similar in ...
7
You can use Function.prototype.apply(), your code can be like below:-
const executeModeration = async (command, channel) => {
command.parameters.unshift(channel);
await moderation[command.command].apply(null, command.parameters);
}
you can handle ternary operators logic inside corresponding method.
Top 50 recent answers are included
