-
Notifications
You must be signed in to change notification settings - Fork 269
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 our terms of service and privacy 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
Conversation
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 like yarn-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); | ||
| }; | ||
|
|
||
| const pairs = (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]); | ||
| } | ||
| } return newFilter; |
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 (let i = 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 = startingValue
Starting value can be anything a number, string, object attempting to set i (the index) to starting value is nonsensical. We went over this briefly last night where i should be dynamic dependent on if startingValue is undefined.
src/objects.js
Outdated
| // http://underscorejs.org/#mapObject | ||
| const myMap = {}; | ||
| for (let i = 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 | ||
| const myMap = {}; | ||
| for (let i = 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 of length on obj obj 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
PATRICK KENNEDY
Patrick Kennedy
# Conflicts: # README.md
I'm haven't quite figured it all out yet.