-
Notifications
You must be signed in to change notification settings - Fork 83
finished tests; working on implementing functions #39
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?
Conversation
MiHHuynh
commented
Aug 1, 2017
- Wrote tests
- Working on implementing functions, not sure if I'll be able to finish tonight, but will try my best...
|
Recursion exercises: productOfArray, collectStrings, contains |
mmmaaatttttt
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.
couple of small comments on the testing exercises, but nice job!
testing_exercise/testing.js
Outdated
| @@ -1,0 +1,47 @@ | |||
| function replaceWith(str, replaceThisChar, replacementChar) { | |||
| var arrayOfChars = str.split(""); | |||
| arrayOfChars.forEach(function(val, index){ | |||
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.
nice work. can you think of a way to refactor this using a map?
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.
Just refactored it with a map. I think it looks cleaner now. :)
function replaceWith(str, replaceThisChar, replacementChar) {
return str.split("").map(function(val, index, array){
if (val === replaceThisChar) return replacementChar;
return val;
}).join("");
}
testing_exercise/testing.js
Outdated
| } | ||
|
|
||
| function expand(arr, num) { | ||
| if (num === 1) { |
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.
do you need this if statement?
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.
I was thinking that in the case of num === 1, or with no repeats, I would just return to save the work of going into the for loop, but I can see that it's just unnecessary code. I'll remove it.
elie
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.
Good job! Set your indentation to 2 spaces in your editor, keep it up!
lodash_exercise/lodash.js
Outdated
| function drop(arr, num=1){ | ||
| if (num === 1) { | ||
| return arr.slice(1); | ||
| } else { |
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.
Don't need the else here since you're returning in both cases.
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.
Removed.
lodash_exercise/lodash.js
Outdated
| function head(arr){ | ||
| if (arr.length === 0) { | ||
| return undefined; | ||
| } else { |
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.
same comment as above, don't need the else here.
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.
Removed.
lodash_exercise/lodash.js
Outdated
| function takeRight(arr, num=1){ | ||
| if (num === 0) { | ||
| return [] | ||
| } else { |
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 guessed it! Don't need the else here as well.
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.
Removed.
lodash_exercise/lodash.js
Outdated
| } | ||
| return false; | ||
| } | ||
| return (collection.slice(fromIndex).indexOf(val) !== -1) ? true : false; |
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.
Nice! You don't need () around the expression here
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.
Removed.
lodash_exercise/lodash.js
Outdated
| function flatten(){ | ||
|
|
||
| function flatten(arr){ | ||
| //NOT WORKING |
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.
Try to use helper method recursion to solve this
lodash_exercise/lodash.js
Outdated
| var result = []; | ||
| for (var i = 0; i < arr.length; i++) { | ||
| if (Array.isArray(arr[i])) { | ||
| flatten(arr[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.
If you wanted to use pure recursion you would have to reassign result with an expression like result = result.concat(flatten(arr[i]))
recursion_exercise/recursion.js
Outdated
| } | ||
|
|
||
| function binarySearch(arr, val) { | ||
| // sort the array |
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.
Try this one when you have some time - you're getting the hang of recursion!
|
|
||
| function acceptNumbersOnly() { | ||
| for (var i = 0; i < arguments.length; i++) { | ||
| if (typeof(arguments[i]) !== "number" || isNaN(arguments[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.
check out the method Number.isFinite - it's a useful way to avoid the logic here
|
Hi--everything I pushed in the past few days are ready for review. Thanks! |
tigarcia
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.
Good job! Let me know if I missed anything
| @@ -1,0 +1,55 @@ | |||
| function sumEvenArguments() { | |||
| var arr = [].slice.call(arguments); | |||
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.
I'm not sure if you had learned spread yet, but you can do
function sumEvenArguments(...arr) {
| } | ||
| } | ||
|
|
||
| function guessingGame(num) { |
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.
Good job here.
| sayHi: function(){ | ||
| return "This person's name is " + this.fullName | ||
| } | ||
| }.apply(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.
I think you want to use bind here:
var obj = {
fullName: "Harry Potter",
person: {
sayHi: function() {
return "This person's name is " + this.fullName;
}
}
}
obj.person.sayHi = obj.person.sayHi.bind(obj);
| if (randomShapeNumber === 1) { | ||
| expectedKey = "white0"; | ||
| ctx.fillStyle = "white"; | ||
| ctx.beginPath(); |
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.
Lots of duplicate code between these two if statements. I'd try to consolidate it and only change the color based on red or white triangle
| scoreSpan = document.getElementById("score-val"), | ||
| seconds = 3, | ||
| score = 0, | ||
| seconds = 30, // added a 0 to make it 30? |
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.
yep, I think I had it at 3 seconds when I was testing
| sayHi(){ | ||
| setTimeout(() => { | ||
| console.log("Your name is " + this.fullName) | ||
| }.bind(this),1000) |
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.
bind isn't needed because the arrow functions use the this from the surrounding area. Also, the curly braces aren't needed for a 1 line arrow function. So you can simplify to:
var person = {
fullName: "Harry Potter",
sayHi(){
setTimeout(() => console.log("Your name is " + this.fullName),1000)
}
}
| var temp = arr[0] | ||
| let temp = arr[0] | ||
| arr[0] = arr[1] | ||
| arr[1] = temp |
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.
this can be simplified to:
let arr = [1,2];
arr = [arr[1], arr[0]]
| // }); | ||
| // } | ||
|
|
||
| function double(arr){ |
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.
const double = arr => arr.map(v => v * 2);
| return result.join(""); | ||
| } | ||
|
|
||
| Function.prototype.bind = function(thisArg, ...outerArgs) { // anon func takes thisArg to reset the this, and other arguments if needed |
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.
Good job
| String.prototype.reverse = function() { | ||
| var result = []; | ||
| let temp = this.split(""); | ||
| for (let i = temp.length-1; i >= 0; 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.
Nice use of an array. You could probably just do this as well:
String.prototype.reverse = function() {
return this.split("").reverse().join("");
}
tigarcia
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.
I didn't see your hacker snooze ajax solution. Did you forget to submit it?
|
Hi Tim. My hack or snooze app is pretty bare bones at the moment. I pushed what I currently have, but I'm still working on it. Also I haven't started Tic Tac Toe, so no commits related to that for now. |