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 a specification for bang methods #122

Merged
merged 1 commit into from
Jan 4, 2014
Merged

Conversation

gylaz
Copy link
Contributor

@gylaz gylaz commented Oct 22, 2013

No description provided.

@@ -130,6 +130,8 @@ Ruby
instead to emphasize code branches.
* Avoid explicit return statements.
* Avoid using semicolons.
* Avoid appending bang (!) to method names, unless you a safe method with

Choose a reason for hiding this comment

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

Small typo

unless you *have* a safe method

Copy link
Contributor

Choose a reason for hiding this comment

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

Extraneous 'you' in the sentence

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me once the typo is fixed.

@calebhearth
Copy link
Contributor

👎 I don't think this is something we should provide a guideline for. It's very much a "does it feel right in this instance" sort of thing, which every conversation on the topic I've had has evolved into.

@jyurek
Copy link

jyurek commented Oct 22, 2013

"unless you a safe method" <-- should that be "unless you know a safe method"?

@calebhearth
Copy link
Contributor

Also my opinion about bang methods has changed, I don't know if any one else feels similarly. I feel like the bang method is there as an indication that something unexpected will happen and don't feel like that is mutually exclusive to a non-bang method existing.

I 👍 this, but still don't like having the guideline added.

@gylaz
Copy link
Contributor Author

gylaz commented Oct 22, 2013

@calebthompson I think a guideline is important, this will prevent discussions in pull requests about personal opinion on method naming.

@calebhearth
Copy link
Contributor

God forbid we have discussions or opinions.

Caleb Thompson
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Tuesday, October 22, 2013 at 1:45 PM, Greg Lazarev wrote:

@calebthompson (https://github.com/calebthompson) I think a guideline is important, this will prevent discussions in pull requests about personal opinion on method naming.


Reply to this email directly or view it on GitHub (#122 (comment)).

@jferris
Copy link
Contributor

jferris commented Oct 22, 2013

My general issue with bang methods that "feel right" is that they don't actually tell you anything. They end up seeming random, and as a reader of a method like 'invite!`, I have no idea if the author wants me to know that it's going to modify the user, might raise an exception, or something new and exotic. It's become a smell like "do_invite" or "execute_invite." It tells you nothing about what the method does in addition to its base name.

I'd rather not have bang methods at all than try to guess what the bang means.

@jferris
Copy link
Contributor

jferris commented Oct 22, 2013

indication that something unexpected will happen

I don't think we should have methods where something unexpected will happen. Can you give an example of a method with unexpected consequences that isn't just poorly named?

@calebhearth
Copy link
Contributor

I think that it tells you that you should probably check out what it is. Does the method invite modify the record, set an attribute, send an email? Bang methods are there to tell you to be careful before using them, and that maybe you should read it first.

Caleb Thompson
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Tuesday, October 22, 2013 at 1:47 PM, Joe Ferris wrote:

My general issue with bang methods that "feel right" is that they don't actually tell you anything. They end up seeming random, and as a reader of a method like 'invite!`, I have no idea if the author wants me to know that it's going to modify the user, might raise an exception, or something new and exotic. It's become a smell like "do_invite" or "execute_invite." It tells you nothing about what the method does in addition to its base name.
I'd rather not have bang methods at all than try to guess what the bang means.


Reply to this email directly or view it on GitHub (#122 (comment)).

@halogenandtoast
Copy link
Contributor

@jferris the point I feel is that you should look at what the bang method does and understand why you'd use it. The bang is there to signify that to you. As developers we shouldn't avoid reading the code.

@halogenandtoast
Copy link
Contributor

I'd rather not have bang methods at all than try to guess what the bang means.

I don't disagree with this.

@jferris
Copy link
Contributor

jferris commented Oct 22, 2013

I think we should merge the guidelines as-is; it's a guideline, and not a rule, and it makes sense for most cases.

If we find cases where the "feels right"/"unexpected effects" argument holds up, we can revise or remove.

Objections?

@calebhearth
Copy link
Contributor

I'd disagree that it makes sense for most cases. It just happens to align with the way ruby-lang and rails define their methods, not how developers use it in most cases. That we have it come up so often proves that.

@salbertson
Copy link
Contributor

I would be fine with avoiding bang methods completely. I think renaming a method to describe the unexpected behavior is more clear than using a bang.

@calebhearth
Copy link
Contributor

That seems like a good compromise to me as well.

"Avoid defining bang (!) methods; prefer descriptive names."

On Tue, Oct 22, 2013 at 6:31 PM, Scott Albertson [email protected]
wrote:

I would be fine with avoiding bang methods completely. I think renaming a method to describe the unexpected behavior would be more clear than using a bang.

Reply to this email directly or view it on GitHub:
#122 (comment)

@jferris
Copy link
Contributor

jferris commented Jan 3, 2014

@gylaz Looks good to merge.

@gylaz gylaz merged commit 79e43d7 into master Jan 4, 2014
@gylaz gylaz deleted the gl-clarify-bang-methods branch February 28, 2014 21:34
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.

8 participants