Skip to content

Conversation

@toChaim
Copy link
Collaborator

@toChaim toChaim commented Aug 1, 2017

No description provided.

function replaceWith(str, target, replacement){
return str
.split('')
.map(function(v,i,a){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small thing, but if you're not using the other arguments i and a, you don't need to include them in the callback. your callback can just be a function of a single parameter.


function mergeObjects(...args) {
return args.reduce(function(t,v,i,a){
Object.keys(v).forEach(function(sv, si, sa){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above - your callbacks here only need to reference the variables you're actually using.

}

function mergeArrays(arr1, arr2) {
return arr1.concat(arr2).sort();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one gotcha to be aware of with sort - it won't sort numbers correctly! The default sort in JavaScript is alphabetical, so for example:

[23,1,4].sort() // [1, 23, 4]

The example doesn't catch this.

To fix this, you can pass in a comparator to the sort function telling it how it should sort. reference

Copy link
Contributor

@tigarcia tigarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your code looks good. Great job making it responsive.

Copy link
Contributor

@tigarcia tigarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you have some more work to do on hacker snooze. Let me know if you want more feedback on it.

return Promise.all(topstories.slice(0,50).map(function(v,i){
return $.get(
`https://hacker-news.firebaseio.com/v0/'
+ 'item/${topstories[i]}.json?print=pretty`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you mixed single quotes and back tics. I'm not sure what that does actually.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want to use two string templates:

`https://hacker-news.firebase.io.com/v0/`
+ `item/${topstories[i]}.json?print=pretty`

this.favoriteColor = favoriteColor;
this.favoriteNumber = favoriteNumber;
this.favoriteFoods = favoriteFoods;
this.fullName = function(){ return `${this.firstName} ${this.lastName}`;};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add this to the prototype instead of the object. That way you reduce your memory usage.

return this.family.length;
};

Array.prototype.map = function(fn, keepUndefined = false){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the second parameter to push is used to set the keyword this in the callback function. So it would be something like:

newArr.push(fn.call(thisArg, this[i], i, this));


}

Array.prototype.reduce = function(fn, init, keepUndefined = false){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think reduce has a 3rd parameter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original did not have 3 parameters. I had a use case that where I needed to fill in 'undefined' so I thought I would add it as a choice. Same with map. Because I gave it a default value it should not brake legacy code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants