MethodLength should not consider Heredocs

[fabiopelosin]

Any attempt of refactoring would end up in splitting the heredoc decreasing readability.

[mikegee]

I avoid this problem by moving the heredoc its own constant or method.

[bbatsov]

The same problem applies for huge literals of any kind - long hashes, array, etc. Usually they can be extracted elsewhere as pointed by @mikegee.

[ixti]

Although I found "bright side" in extraction of a long hash out of main class into separate file, that makes me feel absolutely terrible. It makes declaration of a single class splitted across multiple files without any good actually. So I vote for counting heredocs, hashes and arrays like a single LOC.

For example I had to extract declaration of HTTP statuses into separate file, but first of all class is inherited from Delegator, secondly it makes reading of class pretty annoying (REASONS hash is used in class).

[bbatsov]

Method length is a line metric. If we start implementing such suggestions it definitely won't be. If you don't find it useful simply disable it. I understand your reasoning, but this should be counting lines, not literal assignments.

[ixti]

I disagree :D

I'm not proposing to count literal assignments. I propose to count statements. You don't count blank lines and comment lines. And although I understand that these are separate questions, I would like to emphasize my point that elements of plain objects should be treated as a single line of code:

FOO = {
  :some => :value,
  :another => :value
}

# IMHO should be counted same as:

BAR = { :some => :value, :another => :value }

After all, extracting array/hash with long list of elements into separate file will not affect "real" amount of LOCs - it will only fool rubocop.

[bbatsov]

I'm not saying there isn't value in such a metric, but I'm saying you can't call such a metric MethodLength or ClassLength. And there are many implications to consider - what are we supposed to do for heavily nested ifs for instance? We already have a cyclomatic complexity check, perhaps this is what you're looking for?

I understand the problem all too well, but I don't think it has a clear-cut universal solution.

[yujinakayama]

I agree with @bbatsov.

We know MethodLength and ClassLength are not so smart, but the simple line count metric allows you to clearly recognize how many line you need to reduce.

If we introduce special case handling into the two cops, I'm pretty sure we'll see many subjective demands “This should be special”.

If you feel they are strict, relax the max line count or disable them. The combination of the line count cops and CyclomaticComplexity is the realistic solution.

[ixti]

Found another workaround. Instead of extracting plain structs into separate files, one can use DATA const with YAML:

# stdlib
require "yaml"

class Status
  # ...

  REASONS = YAML.load(DATA)

  # ...
end

__END__

100: Continue
101: Switching Protocols
102: Processing
200: OK
201: Created
202: Accepted
203: Non-Authoritative Information
204: No Content
205: Reset Content
206: Partial Content
207: Multi-Status
226: IM Used
300: Multiple Choices
301: Moved Permanently
302: Found
303: See Other
304: Not Modified
305: Use Proxy
306: Reserved
307: Temporary Redirect
308: Permanent Redirect
400: Bad Request
401: Unauthorized
402: Payment Required
403: Forbidden
404: Not Found
405: Method Not Allowed
406: Not Acceptable
407: Proxy Authentication Required
408: Request Timeout
409: Conflict
410: Gone
411: Length Required
412: Precondition Failed
413: Request Entity Too Large
414: Request-URI Too Long
415: Unsupported Media Type
416: Requested Range Not Satisfiable
417: Expectation Failed
418: I'm a Teapot
422: Unprocessable Entity
423: Locked
424: Failed Dependency
426: Upgrade Required
500: Internal Server Error
501: Not Implemented
502: Bad Gateway
503: Service Unavailable
504: Gateway Timeout
505: HTTP Version Not Supported
506: Variant Also Negotiates
507: Insufficient Storage
510: Not Extended

So I really think that decision on counting lines as is for literals is a bad idea. Actually, I think same goes for string literals written with \. Or we need a new cops called MehtodStatementsLength and Class StatementsLength, similar to MethodLength and ClassLength but counting statement lines not just lines...

[ixti]

Correction to the sample above. It will work only if DATA defined in the executed file :((

[ixti]

Instead of DATA one can implement __DATA__ method that mimics Perl's magic constant with same name: https://gist.github.com/ixti/21b291cdb80574bf29a9

[jfelchner]

I just ran into this, opened #1361 and @bbatsov pointed me to this issue.

I understand where everyone is coming from. And I understand that this could be construed as a "slippery slope", but honestly I don't think that anyone can make a case for anything other than:

• Blanks
• Comments
• Hashes
• Method Parameters
• Chaining

If options are implemented for those (as they already are for blanks and comments), that will catch almost all of the situations that people are having issues with.

Those last three are cases where a different style is almost always introduced for readability only.

Yes, you could technically say that you could do this:

def my_method; if some_var_name == some_other_var_name; puts 'yeah do true stuff'; else puts 'Awww not true'; end; end

But I don't think that a majority of people would consider it reasonable to call that "one line" of code.

Then again, maybe there just needs to be an MethodExpressionCount cop that counts the number of expressions and make it completely separate from the MethodLength one.

[jonas054]

👍 for MethodExpressionCount or something like that. There's one size metric called ABC that we could consider.

[jfelchner]

@jonas054 that's awesome. That seems exponentially more valuable than "lines of code" as a metric.

[JeanMertz]

👍 for a MethodExpressionCount cop, using the ABC Metric

[jonas054]

I'll get started on an implementation.

[jfelchner]

@jonas054 I was wanting to implement a couple changes and additional cops. I'd like to learn how it's done so if you want to pair on this, let me know. You can email me at the email that's on my profile or you can DM me on Twitter. I just started following you.

[schmijos]

As of #8159 you can configure heredocs to be counted as one line (CountAsOne).