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

Improvement: Multiple detection and Remove an user #7

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

Conversation

pachirel
Copy link

Multiple detection

Now we can increment multiple users at one line!

For example, this command will increment taro and hanako:
taro++, hanako++ ,

This will add 3 points to taro:
taro++ taro++ taro++

And, this is just a joke:
taro-- taro++

Remove an user

When you mistyped name, now you can remove it.

tarp++
hubot scorekeeper remove tarp

@pachirel pachirel mentioned this pull request Aug 29, 2015
@yoshiori
Copy link
Owner

Add remove function is ok.

Multiple increment/decrement in one line is cool!
but IMO it lost readability.

Please change like this

robot.hear /(\w+)\+\+$/g, (msg) ->
  for str in msg.match
    user = userName(str)
    scorekeeper.increment user, (error, result) ->
      msg.send "incremented #{user} (#{result} pt)"

robot.hear /(\w+)\-\-$/g, (msg) ->
  for str in msg.match
    user = userName(str)
    scorekeeper.decrement user, (error, result) ->
      msg.send "decremented #{user} (#{result} pt)"

@pachirel
Copy link
Author

pachirel commented Sep 1, 2015

but IMO it lost readability.

Indeed.

But I can't work your example code correctly in my local environment.
Using global match, msg.match returns entire matching string instead of captured string.
e.g. )When foo++ is coming, msg.match doesn't return foo, but foo++.
And, when foo++ bar-- is passed, foo++ is ignored.

Anyway, I'm working to refactor my PR code better 💃

thanks.

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.

2 participants