Count lines in private methods

[lasseebert]

I'm not sure if this is a bug or by design.

Lets say I have this class in foo.rb:

class Foo
  def foo
    1
    2
    3
    4
    5
    6
  end

  def bar
    1
    2
    3
    4
    5
    6
  end
end

sandi_meter -d will produce the expected result:

$ sandi_meter -d
1. 100% of classes are under 100 lines.
2. 0% of methods are under 5 lines.
3. No method calls to analyze.
4. No controllers to analyze.

Methods with 5+ lines
  Class name  | Method name  | Size  | Path     
  Foo         | foo          | 6     | ./foo.rb 
  Foo         | bar          | 6     | ./foo.rb 

If I make the bar method private like this:

class Foo
  def foo
    1
    2
    3
    4
    5
    6
  end

  private

  def bar
    1
    2
    3
    4
    5
    6
  end
end

The bar method is not counted:

$ sandi_meter -d
1. 100% of classes are under 100 lines.
2. 0% of methods are under 5 lines.
3. No method calls to analyze.
4. No controllers to analyze.

Methods with 5+ lines
  Class name  | Method name  | Size  | Path     
  Foo         | foo          | 6     | ./foo.rb 

Shouldn't the bar method be counted even though it's private?

This example was mase with ruby 2.1.0

[lasseebert]

Ok, I have found the place in the analyzer that only analyzes a method if it's not protected or private (https://github.com/makaroni4/sandi_meter/blob/master/lib/sandi_meter/analyzer.rb#L167).

Is this intended? AFAIK the rules does not say that private methods can be longer than five lines.

[lasseebert]

By the way: Thanks for a great tool! I love this! 😄

[agileapplications]

+1 I am pretty sure there should be no exception to this rule for private or protected methods.

[makaroni4]

How about option for including private methods for consideration? This way I am afraid sandi_meter will look like Boing 777 ✈️ 😄

@mgrecar, @bf4 what do you think?

[agileapplications]

An option would be great, even though I prefer this to be default. Why should you break the rules in private methods? In the worst case, you copy a big chunk of code to a private method and just call it from the public method -> fully conforming now to sandi's rules ;-)

[mgrecar]

@makaroni4, it definitely seems like it was intentional to ignore private methods, was there a reason for that? From the perspective of the rules as they appear here, it makes no special exception for private methods. From a practical perspective, I don't see a particularly compelling reason to skip them. The developer reading and understanding the code will not be shielded from a mess that gets hidden in a private method, in the scenario @agileapplications points out, they'd just be 'gaming the system'. So, I agree that private methods should be included in the rules, as well.

As far as if it should be an option or the default behavior, I would say it should be default behavior. I actually didn't realize it was doing this until now, and I find the behavior surprising, so that's a strong indicator to me that it should be the default. I suppose an option could be included to skip private methods, but I'm not sure that provides value.

[lasseebert]

Thanks for taking this discussion :)

I agree that it should include private methods. Also, I don't see much value in a configurable option to not include private methods.

[mgrecar]

@makaroni4, I created a new feature branch on my fork to test out the change. In making the change, it appears 3 other existing tests fail, which were explicitly testing that private and protected methods were not found by the Analyzer. This would be simple for me to address, but I want to understand why private and protected methods were expected to be ignored, before I fix the tests and issue a PR. Also, while making this change doesn't change the public API, it would change the results of running the gem on a project... so, that would be a patch level release (ex. 1.1.7 -> 1.1.8), right? I can bang this out in an hour, if everyone's in agreement on this.

[andersennl]

Is there any update on this? @makaroni4 @mgrecar
Thanks for the great gem!