Skip to content

Encourage improvements after submission #262

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.

Sign up for GitHub

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

Merged
kytrinyx merged 3 commits into from
Dec 27, 2015
Merged

Encourage improvements after submission #262

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2d7208d
Add extra hints to submit on first iteration
eToThePiIPower Dec 16, 2015
7620e47
Add some variety to extra hints on submit
eToThePiIPower Dec 17, 2015
ac8c049
Fix formatting and wording of tips in cmd/submit
eToThePiIPower Dec 20, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions cmd/submit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"log"
"os"
"path/filepath"
"math/rand"
"time"
Copy link
Copy Markdown
Member

@kytrinyx kytrinyx Dec 19, 2015

Choose a reason for hiding this comment

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

Would you make sure this is run through gofmt, please? goimports, would be good, too.


"github.com/codegangsta/cli"
"github.com/exercism/cli/api"
Expand Down Expand Up @@ -97,6 +99,28 @@ Your submission can be found online at %s
if submission.Iteration == 1 {
msg += `
To get the next exercise, run "exercism fetch" again.

`
rand.Seed( time.Now().UTC().UnixNano() )
phrases := []string{
"For bonus points",
"Don't stop now: The fun's just begun",
"Some tips to continue",
}
msg += fmt.Sprintf("## %s", phrases[rand.Intn(len(phrases))])
msg += `

Did you get the tests passing and the code clean? If you want to, these are some
additional things you could try:

* Remove as much duplication as you possibly can.
* Optimize for readability, even if it means introducing duplication.
* If you've removed all the duplication, do you have a lot of conditionals? Try
replacing the conditionals with polymorphism, if it applies in this language.
Copy link
Copy Markdown
Member

@kytrinyx kytrinyx Dec 19, 2015

Choose a reason for hiding this comment

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

Let's not mention polymorphism. How about suggesting trying to get rid of conditionals without suggesting a tactic?

How readable is it?

Then please share your thoughts in a comment on the submission. Did this
experiment make the code better? Worse? Did you learn anything from it?
`
Copy link
Copy Markdown
Member

@kytrinyx kytrinyx Dec 19, 2015

Choose a reason for hiding this comment

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

Should we use a text/template for this?

Copy link
Copy Markdown
Contributor Author

@eToThePiIPower eToThePiIPower Dec 20, 2015

Choose a reason for hiding this comment

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

Looking into text/template, it seems like it'll make the code less readable overall. It either:

  • Adds overhead for creating the template plus a data structure (struct/map), while still requiring all the text in the source file, or
  • Creates a run-time dependency on an external file using ParseFiles, which doesn't really make sense.

If anything it makes sense to define the static text as a const definite at the end of the file, eg:

    msg += fmt.Sprintf("## %s", phrases[rand.Intn(len(phrases))])
    msg += tips
...
const tips = `
(tips here)
`

Copy link
Copy Markdown
Member

@kytrinyx kytrinyx Dec 20, 2015

Choose a reason for hiding this comment

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

Yes, I think you're right. Defining it as a const would be better.

}

Expand Down