Skip to content
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

add solutions to Objects and Classes chapter #164

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cRAN-cg
Copy link
Contributor

@cRAN-cg cRAN-cg commented Oct 2, 2019

Hi @gvwilson @MayaGans ! Could you please review

Also could you please advise the format for answers to questions under Active Expressions

@cRAN-cg
Copy link
Contributor Author

cRAN-cg commented Oct 8, 2019

Hi @MayaGans could you please review

Copy link
Collaborator

@gvwilson gvwilson left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this - feedback inline.

docs/answers.md Outdated

```{js}
class Delay {
constructor(initialValue){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use a space between the method name and the opening parenthesis, and between the closing parenthesis and curly brace:

constructor (initialValue) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review and have acknowledged the required changes to be made 👍

docs/answers.md Outdated
this.nextValue = initialValue
}

call(nextValue) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Spacing as above please.

}

call(inputValue){
return this.filterValues.some((value) => value === inputValue) ? null : inputValue
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is probably more efficient to use indexOf:

return this.filterValues.indexOf(value) === -1 ? inputValue : null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @gvwilson,
I performed some tests out of curiosity to learn more about your suggestion.
I did run a performance test. The results showed me that both are comparatively same.
Please advise.

docs/answers.md Outdated

```{js}
class Filter {
constructor(...values){
Copy link
Collaborator

Choose a reason for hiding this comment

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

spacing as above

docs/answers.md Outdated
this.filterValues = values
}

call(inputValue){
Copy link
Collaborator

Choose a reason for hiding this comment

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

spacing as above

docs/answers.md Outdated

```{js}
class Pipeline {
constructor(...pipes){
Copy link
Collaborator

Choose a reason for hiding this comment

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

spacing as above

docs/answers.md Outdated
this.pipes = pipes
}

call(inputValue){
Copy link
Collaborator

Choose a reason for hiding this comment

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

spacing as above

docs/answers.md Outdated
}

call(inputValue){
let returnValue
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no need to introduce another variable: you can update inputValue repeatedly with inputValue = pipe.call(inputValue). (Although if you're going to do this, it should probably just be called value.)

docs/answers.md Outdated
call(inputValue){
let returnValue
for (let pipe of this.pipes){
returnValue = (() => pipe.call(inputValue))()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why introduce the anonymous function call? Why not simply value = pipe.call(value)? (Assuming you unify the two variable inputValue and returnValue as described above.)

docs/answers.md Outdated
returnValue = (() => pipe.call(inputValue))()
if (!returnValue) break
inputValue = returnValue
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have we introduced break before this point in the text? If not, can you find a solution that does not use it?

@gvwilson gvwilson removed the request for review from MayaGans October 12, 2019 10:40
@gvwilson gvwilson assigned gvwilson and unassigned MayaGans Oct 12, 2019
@cRAN-cg cRAN-cg requested a review from gvwilson October 12, 2019 14:40
@gvwilson gvwilson requested review from MayaGans and removed request for gvwilson November 9, 2019 12:24
@gvwilson gvwilson assigned MayaGans and unassigned gvwilson Nov 9, 2019
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.

None yet

3 participants