Skip to content

Conversation

@breyana
Copy link
Collaborator

@breyana breyana commented Mar 4, 2017

No description provided.

expect(handshake.commands()).toEqual(['wink']);
});

xit('10 is a double blink', function() {
Copy link

Choose a reason for hiding this comment

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

when I looked it up, it appears that the xit() function skips the test? I assume you were working on this when the week ended :) Your code is looking good! I'd recommend putting up the code into github without the xit() even if the tests are failing.

expect(result).toEqual('Whatever.');
});

xit('shouting', function() {
Copy link

Choose a reason for hiding this comment

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

I cloned your project so I could test it and give detailed feedback. It's looking great, with clean code, though it's difficult for me to test since many of the tests have been skipped.

var Hamming = function() {};

Hamming.prototype.checkForSameLength = function(firstStrand, secondStrand) {
return firstStrand.length === secondStrand.length ? true : false
Copy link

Choose a reason for hiding this comment

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

I think this is a great use of the ? operator! Clean code, succinctly written.

Hamming.prototype.compute = function(firstStrand, secondStrand) {
if (this.checkForSameLength(firstStrand, secondStrand)) {
return firstStrand.split('').reduce( function(accumulator, current, index){
return current !== secondStrand[index] ? accumulator + 1 : accumulator
Copy link

Choose a reason for hiding this comment

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

Another place of terse well written code that's easy to understand. Nice!

@@ -7,31 +7,31 @@ describe('Hamming', function () {
expect(hamming.compute('A', 'A')).toEqual(0);
Copy link

Choose a reason for hiding this comment

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

Unsure of the @@ code before this expect function in your test. Perhaps the describe() was supposed to be on the next line?

'Ileana', 'Joseph', 'Kincaid', 'Larry'
]) {
this.diagram = diagram.split('\n')
this.students = students.sort()
Copy link

Choose a reason for hiding this comment

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

This must be because I'm new or something, but what kind of sort() is happening here? I feel a comment might be helpful :)

@@ -0,0 +1,30 @@
var Series = function(inputNumber) {
if( /[^0-9]/g.test(inputNumber) ) {
Copy link

Choose a reason for hiding this comment

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

I've noticed this nice errors catchers in all the right places in the code. Great! Thanks for teaching me this.

return largestProduct
};

module.exports = Series;
Copy link

Choose a reason for hiding this comment

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

This is a nicely coded module!

if (givenNth < 1) {
throw new Error('Prime is not possible')
}
//count up towards infinity
Copy link

Choose a reason for hiding this comment

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

Great comment. Shows me your thoughtflow, which I feel is very useful.

Copy link

@bairbanu bairbanu left a comment

Choose a reason for hiding this comment

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

Clean code, easy to read and follow, comments placed where they're most needed, was fun to review. Suggestion for future would be to enable tests so reviewer can run them and make more detailed suggestions :)

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