Hot answers tagged

21

Profiling: We could use a library like Tracy to do the sort of profiling you mention in the question. But it's quicker to ask "what's taking the most time" and get an answer with a profiler like Very Sleepy, or the built in Visual Studio profiler. So... I stuck your code into a project created with create-vs-napi, and ran stringify on some json ...


6

From a short review; Why hardcode the batch length to 500 ? Why not have the batch length be a nice constant ? You have even hard coded the length in the function name, which is really unfortunate batchNumber++ is more canonical than batchNumber += 1 I would have gone for maxIndex = Math.min(startingIndex + 500, dataArray.length); because now you have a lot ...


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

A few thoughts; Most would write const flatFeature = []; to initialize an array in javascript, though what you have is certainly correct. You should strive to de-nest your code. Having code unnecessarily placed in if or if-else blocks increases the number of paths through your code, making it harder to follow and unit test. In this case, you could de-nest ...


5

From a short review; arr1 and arr2 are terrible names, we could call them consonants and vowels accessing sp[i] so often is not efficient, I would store that value in a variable when dealing with lists, consider going functional (using forEach, map, reduce, join etc.) sp is not a great name There is a nicer way to iterate over the characters of a string ...


5

Can I use other modules to make this job easier? You can use some web framework like fastify or express. This would ease the code a lot For a job this simple, is it worth to work with HTML and CSS as separate files? Assumig your project will grown, it is a good approach to move some "static text" in a separated file. How are HTML and CSS ...


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 ...


5

Test case structure It doesn't seem like the code totally "lacked effort and structure" but the structure, at least of the test cases, could be better. Since all three describe() blocks contain a test that calls getLatlonFromPostcode so there could be a single describe() block with three it() blocks to test the various aspects of how the function ...


5

The code could be improved with a few tweaks, but it's not bad, and the overall structure looks just fine to me. Error handling is in the wrong place Some of your route handler are async, and inside them you await. Whenever you have a Promise, make sure to handle possible errors properly. Although the functions outside of your router handlers handle errors - ...


5

When you have multiple asynchronous requests to make, and you want them to operate in parallel, usually the appropriate tool to use is Promise.all. You can use it here to wait for the groups and the profile data at once, despite the fact that the two requests are pretty distinct from each other: async function gatherUserInfo(userId) { try { const [...


5

Your code boils down to just executing getGroups() in parallel with the getProfile()-getStatusText() sequence. You might want to extract that sequence out to a separate function. This way, it's obvious that they're treated as one thing (two async calls build one object). You can then Promise.all() the return of this new function (getStatus() in the following ...


5

To handle codes, use a parser. Build an AST from parser (maybe comments are already removed there). And then convert it to codes. A FSM implementation may work. (I'm not sure). But I believe your current RegExp approach are far from working. Anyway, 1. Line break JavaScript support 5 different line terminator sequence. Your code only support one or two of ...


4

The main culprit here is your progress indicator. You're continuously refreshing a line in stdout every time write is called, and write is called a very large number of times. If you reduce the frequency of console writes, you'll make your script a whole lot faster. One option is to write to the console only every 0.5 seconds or so: // Performance checker: ...


4

You can override the executor and can pass an onCancel function to it like following: class ResponsePromise extends Promise { constructor(executor) { const onCancel = (cb) => { //using nextTick because we cant use "this" before super() setTimeout(()=> { this.cancelCb = cb; }) } const oExecutor = (...


4

Comments While the code is mostly simple to follow, it would be good to add comments to document what decisions you made. Even if you are the only one maintaining this code your future self might not remember what considerations you had in the past. At least document the input, output and purpose of functions. Some style guides call for comments to be in ...


4

Your code has a bug: d.getHours() will return a plain number. Calling toLocaleString on that number calls Number.prototype.toLocaleString, which: returns a string with a language-sensitive representation of this number. It does not do anything with timezones; the second parameter does not recognize a timeZone property. For example, where I am now, ....


4

In ES6, when you have a variable that's going to be used as a property of an object, you can use the same variable name as the property so as to use shorthand syntax, if you want: const data = await readStream(filteredStream); //Reads in a sample of data and closes the connection. return { data }; You do: filteredStream = streamConnect(); You are ...


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 ...


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&...


4

Use precise variable names getFiles resolves not to an array of Files, but to an array of file name strings. Consider renaming appropriately. Processing issues Very large amounts of parallel file system accesses can make the system seem unresponsive. One solution to this would be to throttle the number of files that can be processed at any one time. The ...


4

You could create a look up table (a simple object) where you can pre calculate the amount of times a domain is in the repeated in the domains array, this reduces the algorithmic complexity of your current solution which is n * m (dataElements * domainElements) Whereas my suggestion has a lower complexity of (n + m) const data = [ { Domain: '...


3

The use of arrow functions looks okay to me. One advantage to using them is that they can be simplified to a single line but one might hold the opinion that is less readable. For example: app.get('/welcome', (req, res) => { res.render('pages/welcome'); }); Could be simplified to: app.get('/welcome', (req, res) => res.render('pages/welcome')); Note ...


3

Let me share my async await solution for that: Because Your fetch function is pretty similar till the res.text(), I would create an own function (fetchData ) for that, what is returning a Promise, what is resolving a res.text() Now you can assign an async to your getLogsfunction Lets reorganise your code above Error handling is missing here, but take care ...


3

Be aware that mixing await statements with .then()/.catch() statements can be a Code Smell, indicating an incomplete understanding of how Promises and async/await work. That said, there are times/places for mixing these constructs but you need to be careful when doing so. It's generally better to prefer one or the other (with async/await usually being the ...


3

Some thoughts: You are using async but not "using" it. You can simplify this code for the reader by doing const accessToken = await axios.post(...); and similar to de-nest and make the code more understandable. This also helps your avoid bad variable names like res1, res2, res3, etc. Promise.all() is your friend when it comes to making batch ...


3

I'm no expert in JavaScript, but it seems to me if you changed the scope of token so that it is defined at the beginning of the function and then just modify the value as necessary that you can have only one return statement after all the if statements.


3

Your code looks very structured and nicely written, but to my understanding, your solution is not "clean architecture" (CA) as described by uncle bob. Your solution is an MVC solution. In clean architecture you can (for example): easily replace express with another framework easily replace mongo with another DB In your case the framework is embedded into ...


3

I think the main cause of your delays is not a nested for but rather a fact that you extract all data from your mongodb collection into memory. What you can do is You can calculate formatted date once. Then you can query just those users that contain given event date instead of populating all users in memory. Here's the guide This will look roughly like ...


3

I am not primarily a JS developer, therefore I won't assist you with the JavaScript part itself, but I can give you a few hints regarding several issues I've found. Lack of transactional safety In your example, you're executing two DELETE statements on the database. 1) delete t1, t2 from postschema t1 join commentschema t2 on t1.id = t2.postid where t1.id = ?...


3

Are the fs and path packages needed? Do they simplify things enough to justify the inclusion? fs is needed. How else would you get your data down the line without reading it in? path could be replaced with your own path utility library, but I wouldn't waste time reinventing that when a built-in one already exists. Can this be written more concisely? This ...


Only top voted, non community-wiki answers of a minimum length are eligible