monterail / guidelines

[DEPRECATED] We are Ruby on Rails experts from Poland. Think hussars. Solid & winged. These are our guidelines.
71 stars 17 forks source link

Don't use one letter variables unless it is very short block. #138

Closed sheerun closed 11 years ago

sheerun commented 11 years ago

So only following are acceptable:

[1,2,3].map{ |e| e + 1 }

And that doesn't mean you can use array.map{ |e| ... } or hash.map{ |k, v| ... } everywhere. Only if the body is one, max two lines long.

Of course there are exceptions, like e in rescue block, but we can use them only because there is no way variable in rescue is not an error / exception...

If you feel there are other cases where you want to use one letter variables, please comment about it here. I'd like to not see them anywhere else.

teamon commented 11 years ago

Ok, let's go:

array.inject({}) do |h, e|
  h[foo] = e.bar
  h
end
if a = variant.address
  [a.country_code, a.state || a.city].join("-")
...
find {|h| h["term"] == "T" }
user.tap do |u|
  u.uid   = user_hash['uid']
  u.email = user_hash['email']
  u.name  = user_hash['name']
  u.save!
end

" there is no way variable in rescue is not an error / exception.." - wrong e in rescue block sucks, use ex at least.

szajbus commented 11 years ago

@teamon why you don't like e in rescue? I prefer ex as well, but for no specific reason...

All above examples are valid uses of one-letter vars imho. I'd add:

array.sort do |a, b|
  # multi-line sort logic
end

By the way I also like to use arr for "anonymoys" arrays.

sheerun commented 11 years ago

@teamon You usually want to rescue Errors, not Exceptions. Exceptions should blow up application.

sheerun commented 11 years ago

All of them except user.tap do |u| are very short blocks.

I'd agree for the user.tap do |u| because we can easily mentally remember that u stands for user (there is only one short variable, and instructions are very similar), so there is no need for duplication.

I don't agree with the array.inject({}) do |h, e|. What is h? What is e? It's really not clear.. Of course they stand for hash and element, but what exactly they are? users? projects? No way.

The same for naming variable array.

jandudulski commented 11 years ago

Oh lord, I agree with @sheerun

sheerun commented 11 years ago

There's nothing wrong to agree with me ;)

jandudulski commented 11 years ago

You usually want to rescue Errors, not Exceptions. Exceptions should blow up application.

@sheerun can you explain?

sheerun commented 11 years ago

Exceptions mean there's something wrong in your application, something went terribly wrong. It should be reported by Airbrake or other tool immediately, and there should be 500. Errors are thrown for example on validation. It's nothing wrong with Error.

sheerun commented 11 years ago

@jandudulski, @chytreg, @teamon, @szajbus, @Ostrzy Any other exceptions or should I prepare pull request?

teamon commented 11 years ago

You do want to catche exceptions. Like Faraday's HTTP errors etc. Some of us do not agree with all @sheerun's examples - imho such guideline will be good, but it needs to be more relaxed.

sheerun commented 11 years ago

Faraday's HTTP errors

Ostrzy commented 11 years ago

Agree with @teamon, Something like "Do not overuse one letter variables" (but phrased in way better way) would be good.

jandudulski commented 11 years ago

agree with @Ostrzy

teamon commented 11 years ago

@sheerun Please make PR appropriate to discussion