- Notifications
You must be signed in to change notification settings - Fork269
JS-I&II Push 1#299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
JS-I&II Push 1#299
Uh oh!
There was an error while loading.Please reload this page.
Conversation
indifferentghost left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's generally a good idea to remove unneeded logs likeyarn-error.log. They're very useful information for you; however, provide unneeded bloat on the repo. Just something to think about.
Great
You seem to have a good idea of callbacks and looping over arrays. Your formatting is awesome.
Requested Improvements
I think you should start breaking these functions down with pseudocode. Think through each step and if it's pseudocode you're not staring at a blank screen when writing actual code.
Questions
I unfortunately don't have a timestamp on when this was submitted, was this before our brief 1:1 on the 3/20?
Rating: {1-3}
1.5
| cb(myMap); | ||
| }; | ||
| constpairs=(obj)=>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Was pairs forgotten about?
| if(cb(elements[i])===true){ | ||
| newFilter.push(elements[i]); | ||
| } | ||
| }returnnewFilter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
While this is fine, it's generally suggested that you separate these. It gets hard to debug if you've forgotten something small in this scenario.
}return newFilter;src/arrays.js Outdated
| if(startingValue===undefined){ | ||
| startingValue=elements[0]; | ||
| } | ||
| for(leti=startingValue;i<elements.length;i++){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
let i = startingValueStarting value can be anything anumber,string,object attempting to seti (theindex) to starting value is nonsensical. We went over this briefly last night wherei should be dynamic dependent on ifstartingValue is undefined.
src/objects.js Outdated
| // http://underscorejs.org/#mapObject | ||
| constmyMap={}; | ||
| for(leti=0;i<obj.length;i++){ | ||
| myMap.push(obj[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You can't push into an object.
src/objects.js Outdated
| // Like map for arrays, but for objects. Transform the value of each property in turn. | ||
| // http://underscorejs.org/#mapObject | ||
| constmyMap={}; | ||
| for(leti=0;i<obj.length;i++){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
While you could add a property oflength onobjobj doesn't inherently have a length. You can access it's keys; however, to get a length.
- Updated Readme- Updated objects.js- Updated arrays.js
- match header style for Stretch tasks from last week's curriculum.- console.logging assignment decription
- line 14 last sentence- line 22 period
# Conflicts:#README.md
I'm haven't quite figured it all out yet.