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

Suggest "food for thought" prompts on submissions #3427

Merged
merged 6 commits into from Mar 25, 2017

Conversation

nilbus
Copy link

@nilbus nilbus commented Mar 24, 2017

Implement suggestion 1 in exercism/discussions#123 (comment).

screenshot

  • A thought-provoking prompt appears under the code submission.
  • A new comment button focuses the comment box—helpful when it's buried under a long list of comments, and because it connects what to do with your response.
  • A reload button supplies a new thought

Feature flag: prompts

Todo:

  • Layout design & implementation
  • Implement the Comment button
  • Replace the static meaning of life question with random prompts
  • Implement the ♻️ button

Initial Prompt Questions

  • Are there any complicated bits that deserve to be named/abstracted using a function or variable?
  • Are there any edge cases or unexpected inputs that aren't handled?
  • Are there any intermediate variables or functions that could be extracted to give a name to some concept, making the code easier to scan and understand its intent?
  • Could and should this code be simplified at all? What would you suggest?
  • Could this code be reorganized in a way that would increase its testability, beyond what is required for the given tests?
  • Did this solution use an appropriate amount of abstraction? What changes might make the code easier to understand?
  • Do you have any naming suggestions that could better express intent?
  • Do you understand why the author made the design choices that were made? In what situations might you want or not want to use this approach?
  • How reusable is this code, such that you could use parts of it in new and unexpected contexts?
  • How well might this code withstand future changes in requirements? Will the effort required to change it likely be proportional to the benefit of the change?
  • Is there another approach that could yield better results?
  • Is there anything in this submission that you don't understand or have questions about?
  • Is this code exemplary? Would you want to see more like it? What specifically would you like to see more or less of?
  • Is this code well-organized? What might make it better, and why?
  • Is this easy for you read and understand? What might make it easier? What aspects of this submission do you like and want to incorporate in your own code?
  • What could you learn from this submission?
  • What principles could the author learn and apply that would improve this solution?
  • What trade-offs can you identify being made in this submission? When would this be a good choice, and when would you want to try something different?
  • Where does this solution's formatting stray from community style guides? If any, do the variances matter?
  • Will this solution be performant at scale? In what cases might efficient performance matter?
  • Would you want to want to work in a codebase composed of code similar to this? What do you like and dislike about it?

@kotp
Copy link
Member

kotp commented Mar 24, 2017

The location of the food for thought may be unnoticed depending on the size of the solution and what is included. At the moment, once the code is read, or once your comment text area is hit, it seems like the need to scroll further down would be unnecessary. The presentation should probably be directly above the "new comment" area, rather than to the left where the space is not guaranteed to be viewed.

In a text only presentation, I would also expect the suggestions to be above my comment, not positioned near the code, above others comments.

@nilbus
Copy link
Author

nilbus commented Mar 24, 2017

As I thought about this, the best location of its placement wasn't obvious to me. Above the comment is a reasonable suggestion. My rationale for placing it below the code, is because considering the prompt is the next logical thing to do after reading the code. Reading the prompt is what might inspire you to make a comment, where you otherwise might have nothing to say. With nothing to say, you're not likely to scroll past any existing comments to find the comment box where the prompt is located. When you read the code and do have thoughts to comment on, then when you go to the comment box, the prompt isn't useful—you already have something to say.

This branch requires support for sample().
Since this optional feature is fully implemented in JS, hide it when JS
is disabled, rather than showing something non-functional.
@nilbus
Copy link
Author

nilbus commented Mar 25, 2017

@kytrinyx This is working and is ready for review.

target.fadeIn fadeDuration

randomPrompt: ->
_.sample(@prompts)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is the only place that underscore is used. I'm going to merge it in, but I'd like to consider whether it's worth replacing this with an implementation using Math.random

@prompts[Math.floor(Math.random() * @prompts.length) + 1]

Copy link
Author

Choose a reason for hiding this comment

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

That's interesting since the library was already present. I wonder if it was previously used and the code was removed, or if it is a dependency of a dependency. Git blame is probably telling.

Copy link
Member

Choose a reason for hiding this comment

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

oh, interesting. It is not a big deal. For the rewrite I'll be a lot more careful about dependencies.

@kytrinyx
Copy link
Member

This looks excellent. I'll get it into production in the next 30 minutes.

@kytrinyx kytrinyx merged commit b659a84 into exercism:master Mar 25, 2017
@nilbus nilbus deleted the prompts branch March 25, 2017 16:55
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