Hot answers tagged

29

A few comments: 1) fetch already returns a promise, which means this: new Promise((resolve, reject) => { return fetch(url).then(response => { if (response.ok) { resolve(response) } else { reject(new Error('error')) } }, error => { reject(new Error(error.message)) }) }) Is pretty much the same as: fetch(url).then(...


20

Well, you normally don't want to use the Promise constructor in your higher level code, you want to Promisify as low as possible. So let's create a function that checks for a single image, and resolves whenever you know the status of that image: // When there's only one statement, you can drop the {} and the return // x => y is equivalent to x => { ...


15

Code looks fine - but it strikes me as somewhat unnecessary. A promise can only be resolved/rejected once, so if you just keep the promise object itself around, its result is automatically cached within. I.e. if you call .then() on an already-resolved/already-rejected promise object, it'll just immediately invoke your handler(s) with whatever data it was ...


13

I like it, you could clean up the commented out code and possibly replace $window.googleMapsInitialized = function () { mapsDefer.resolve(); }; with $window.googleMapsInitialized = mapsDefer.resolve; Personally, I would re-arrange the code a tiny bit, keep the URL completely on top, keep asyncLoad and the call closer together. Keep ...


12

I don't see much benefit to the enum. The Notification.permission string (or the return value of requestPermission()) is already pretty clear: 'denied', 'granted', or 'default'. The specification requires that it be one of those, if window.Notification exists. Passing around a number instead of a more intuitive string seems a bit odd. In Javascript, at ...


10

Note this reads more like a hypothetical question but I'm giving you the benefit of the doubt. Your example is alright, but it has a few issues. I'm going to mostly touch on semantics here, so bear with me. Note I'll be going with the WHATWG spec that's implemented in browsers now, which should be the same as the repo you've linked, but just so you are ...


7

This is very weird. If I read the code without this line, I wouldn't have a clue what that might be: var that = $(this); // this is very weird // I suggest the following name, (or thisElement, or something like that) var $elem = $(this); // I use the $ in the name to indicate 'jQuery Object' Saving the object is a good thing, saving multiple DOM look-ups, ...


7

Okay I'm going to be a bit more comprehensive here since I've already read your code quite a few times, and I'm gonna assume that you are on node v6 The setup for the TodoControllerSpec feels kind of verbose. Can it be simplified? If you mean all the require calls you have to do...unfortunately not as far as I know. As a side note, I really like jest for ...


7

There's simply no reason to wrap your existing promise with another promise and no reason to use await or async here either: const getData = function(){ return axios.get(some_data); } Wrapping an existing promise with a manually created promise is a promise anti-pattern and should not be done. Instead, you should just return the promise you already have....


6

This took a while to parse, it's probably me, I found few flaws with the code, except for a distinct lack of comments. I would have made url in preload an optional, 2nd parameter, I would probably want to call preload a few times with different URL's ( for 'data' ) I think using a switch for media and data is overkill, I would have done a simple if ...


6

Interesting question, it was hard to find things I would do different. To Malachi's point, there are a few strings you could put in a config object, I would definitely add also 10000 to that config object as config.delay and your statuses like 'ONGOING'. Also, I would write this: .then(function(results) { if (results.length > 0) return results[0]; ...


6

Without the real code to be tested, I can only provide an example of how I've tested something similar with QUnit and Sinon.JS, though you don't even need the latter. You can either use Sinon.JS to mock a response to the AJAX call or substitute your own manual mock for $.getJSON or $.ajax. I find the former preferable as it documents the AJAX call and tests ...


6

Bearing in mind that you claim that you do not to maintain this code anymore1, there are a few suggestions I would have about this code: define functions and accept parameters- that should allow the nested functions to be moved out... You may be able to utilize Partially applied functions. catch rejected promises The code calls $q.all() with 6 promises, yet ...


6

implementation The biggest thing that jumped out at me is your all function takes a callback instead of returning a Promise like Promise.all would do. (EDIT: it looks like the interview asked that of you, so that makes sense then. A section has been added below.) When your handling the reject branch of each promise (in the .then call), there's no reason to ...


6

There are lots of things you can do: Use a parameterized function to create the XHR promises, since your three functions are identical other than what they're requesting Use Promise.all, since none of the requests relies on information from previous requests and they can run in parallel Move the JSON parsing into the functions retrieving the data, rather ...


6

You could move the door.locked check into the door.unlock() function (or into a new function) which would allow you return a promise for both cases: function unlockDoor(door) { return door.locked ? door.unlock() : Promise.resolve(); } } function openDoor(door) { unlockDoor(door).then(() => { door.handle.turn(...


5

Interesting, I actually really like your constructor, it is neat and shows in one place what the class does. I am not sure I would call it SignupDoctorPostRequest, since it is not a request at all but processes a request. I would consider DoctorSignupModel or since it seems to leverage several models DoctorSignupMetaModel. As for your eager validation, I ...


5

To me it looks like you should move the if (result) into each one of promptUserToPickFromDuplicatePersons, selectPersonFromDuplicatePersonLookup and addAsParticipant and add a method: function handleError(e( { console.log(e.stack); return self.createOdsPerson() .then(self.addAsParticipant.bind(self, redirect)); } Then you can rewrite ...


5

Solution After reviewing many answers, I believe I have two options: Make two queries, like I am doing now, Use a pagination plugin for mongoose My decision was the second one and so I am now using mongoose-paginate. Hope it helps.


5

Your code already looks pretty clean. Good work! There are only a few things I would recommend changing. Don't define timings in CSS that should be defined in JavaScript. In this specific case using a transition made it possible to simplify the code significantly, however in a more involved example it would become problematic. What happens if one progress ...


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) ]); ...


4

Overall it looks pretty good, but I do have some notes: You export new Foo but your Foo function isn't really a constructor. It's just a function that returns an object. This works just fine (one of JS' oddities), but personally I'd just call the function foo - lowercase, not a constructor - and use module.exports = foo();. You're skipping most semicolons ...


4

You have some magic strings: return amqp.connect('amqp://localhost') this.mongo.db = mongo('mongodb://127.0.0.1:27017/test', ['test']); These strings should be kept in a high scope variable, so that you can easily maintain them by changing out the strings in one place. Eventually you will have bigger applications where you use the connection in several ...


4

I like how you can call Resolve any number of times, but the listening objects will only be notified once. I also like how you can attach a listening object after Resolve has already been called, and the listening object will still get the notification. If you're programming in an uncertain multithreaded environment, it greatly reduces the amount of thinking ...


4

One idea would be to put the script tags at the end of the document's body - not in the head at all. It's a common trick in itself, often used to speed up load time, because it means no time is spent parsing scripts before moving on to the page's actual content. It also means that the scripts will have everything that's "before" them in the DOM ready for use....


4

I would just name the variable for Bluebird promises as Promise as it mostly acts like the standard Promise object in ES6. That way, if you happen to run an ES6-compatible Node.js, you can just remove the import, and you'll be set. In the promise constructor, you need not do a return. The code will also feel awkward to a new developer, thinking Promise needs ...


4

Let's start with weather.js. Use Node's querystring module to generate the query string from an object instead of concatenating strings. It's easier to read, plus you don't have to deal with escaping. is appid your API key? You shouldn't put any keys in the code. I suggest you move it out of your code and into an environment variable. That way, it's not ...


4

$.when(getPosition()) $.when is only used when listening to multiple promises. Since getPosition is just one and it already returns a promise, $.when isn't required and you can simply chain then to getPosition(). var getWeather = function(position) { return $.getJSON('https://api.forecast.io/forecast/{APIKEY}/' + position.coords.latitude + ',' + ...


4

You're almost there. But first, a few code smells: function getLocation(query, map, object, delay) { ... service.textSearch({ query: query }, function (results, status) { if (status == google.maps.places.PlacesServiceStatus.OK) { ... object.lang = loc.lng(); object.lat = loc.lat(); deferred....


4

Potential improvements generateToken(email, password) { let user, session; return module.exports.destroyInactive() .then(() => users.checkPassword(email, password, sharedSecret)) .then(_user => user = _user) .then(() => signedon.getByEmailAddressThisProcess(email)) .then(([rows]) => { if (rows.length > 0) { ...


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