Skip to content

Check contents #17

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

Merged
merged 4 commits into from
Sep 9, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
source 'https://rubygems.org'

gem "activesupport", require: false
gem "rubocop", require: false
gem "rubocop-rspec", require: false
gem "pry", require: false
Expand Down
12 changes: 12 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
GEM
remote: https://rubygems.org/
specs:
activesupport (4.2.4)
i18n (~> 0.7)
json (~> 1.7, >= 1.7.7)
minitest (~> 5.1)
thread_safe (~> 0.3, >= 0.3.4)
tzinfo (~> 1.1)
ansi (1.5.0)
ast (2.1.0)
astrolabe (1.3.1)
parser (~> 2.2)
builder (3.2.2)
coderay (1.1.0)
i18n (0.7.0)
json (1.8.3)
metaclass (0.0.4)
method_source (0.8.2)
minitest (5.8.0)
Expand Down Expand Up @@ -35,11 +43,15 @@ GEM
rubocop-rspec (1.3.0)
ruby-progressbar (1.7.5)
slop (3.6.0)
thread_safe (0.3.5)
tzinfo (1.2.2)
thread_safe (~> 0.1)

PLATFORMS
ruby

DEPENDENCIES
activesupport
minitest
minitest-reporters
mocha
Expand Down
22 changes: 22 additions & 0 deletions config/contents/metrics/abc_size.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# ABC Size

ABC Size is a measure of complexity that counts the number of Assignments, Branches (method calls), and Conditionals in your code. It is related to Cyclomatic complexity, which has its origins in the earliest research on static analysis and remains a useful metric of complexity to this day. Cyclomatic complexity correlates the number of potential pathways through a given unit of code with complexity - a unit of code with a lot of potential pathways will have a high cyclomatic complexity score.

Branches -- code that causes decisions to be made -- are the central source of complexity according to cyclomatic complexity. `if` statements, `for` loops, and the predicates in these statements all create execution paths and increase complexity.

Functions with high cyclomatic complexity are harder to test and understand, leading to bugs and higher maintenance costs. Refactor at will.

#### Solutions

* Simplify any branching logic that isn't necessary in your function - collapse redundant branches.
* Extract blocks of logic to private methods where possible.
* Document and follow the pathways through the function and see if you can group functionality to ease in refactoring.

#### Further Reading

* [C2 Wiki: AbcMetric](http://c2.com/cgi/wiki?AbcMetric)
* [SOLID: Part 1 - The Single Responsibility Principle](http://code.tutsplus.com/tutorials/solid-part-1-the-single-responsibility-principle--net-36074)
* ["Clean Code: A Handbook of Agile Software Craftsmanship"](http://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882) - Robert C. Martin
* ["Refactoring: Improving the Design of Existing Code"](http://www.amazon.com/gp/product/0201485672/) - Martin Fowler et. al.
* ["The Single Responsibility Principle"](http://programmer.97things.oreilly.com/wiki/index.php/The_Single_Responsibility_Principle) - Robert C. Martin
* [Your code sucks, let’s fix it (Video)](https://www.youtube.com/watch?v=GtB5DAfOWMQ) — Rafael Dohms
27 changes: 27 additions & 0 deletions config/contents/metrics/class_length.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Class Length

The longer a class is, the harder it is to break down, test, and adequately express with a great name. Classes that are long will grow over time and become harder to manage, so it is usually a worthwhile investment to simplify classes by refactoring into smaller, more discrete units of functionality.

Classes that are too long tend to have too many responsibilities. Though there isn't always a direct correlation between *length* and *number of responsibilities*, it is still a good guideline to "keep it small". From "Clean Code" by Robert "Uncle Bob" Martin:

> "The Single Responsibility Principle (SRP) states that a class or module should have one,
and only one, reason to change. This principle gives us both a definition of responsibility,
and a guidelines for class size. Classes should have one responsibility—one reason to
change." - Robert "Uncle Bob" Martin, "Clean Code"

Seek to create classes with as close to one responsibility as possible and plan to work on those that don’t.

#### Solutions

* Document - What does this class do? Count its responsibilities
* Test - Make sure that your class is tested, and that all of the lines in the explanation above are accounted for.
* Refactor - Take small pieces of the class and pull them out into other collaborator classes. Simplify and clarify the intent of the class by repeating this process until the class has as few responsibilities as possible.
* As you go through this process, think deliberately about the names of your classes: do each of them indicate a clear, single responsibility?

#### Further Reading

* [SOLID: Part 1 - The Single Responsibility Principle](http://code.tutsplus.com/tutorials/solid-part-1-the-single-responsibility-principle--net-36074)
* ["Clean Code: A Handbook of Agile Software Craftsmanship"](http://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882) - Robert C. Martin
* ["Refactoring: Improving the Design of Existing Code"](http://www.amazon.com/gp/product/0201485672/) - Martin Fowler et. al.
* ["The Rule of 30: When is a method, class or subsystem too big?"](http://swreflections.blogspot.com/2012/12/rule-of-30-when-is-method-class-or.html) - Jim Bird
* ["The Single Responsibility Principle"](http://programmer.97things.oreilly.com/wiki/index.php/The_Single_Responsibility_Principle) - Robert C. Martin
27 changes: 27 additions & 0 deletions config/contents/metrics/module_length.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Module Length

The longer a module is, the harder it is to break down, test, and adequately express with a great name. MOdules that are long will grow over time and become harder to manage, so it is usually a worthwhile investment to simplify modules by refactoring into smaller, more discrete units of functionality.

MOdules that are too long tend to have too many responsibilities. Though there isn't always a direct correlation between *length* and *number of responsibilities*, it is still a good guideline to "keep it small". From "Clean Code" by Robert "Uncle Bob" Martin:

> "The Single Responsibility Principle (SRP) states that a class or module should have one,
and only one, reason to change. This principle gives us both a definition of responsibility,
and a guidelines for class size. Classes should have one responsibility—one reason to
change." - Robert "Uncle Bob" Martin, "Clean Code"

Seek to create modules with as close to one responsibility as possible and plan to work on those that don’t.

#### Solutions

* Document - What does this module do? Count its responsibilities
* Test - Make sure that your module is tested, and that all of the lines in the explanation above are accounted for.
* Refactor - Take small pieces of the module and pull them out into other collaborator modules. Simplify and clarify the intent of the module by repeating this process until the module has as few responsibilities as possible.
* As you go through this process, think deliberately about the names of your module: do each of them indicate a clear, single responsibility?

#### Further Reading

* [SOLID: Part 1 - The Single Responsibility Principle](http://code.tutsplus.com/tutorials/solid-part-1-the-single-responsibility-principle--net-36074)
* ["Clean Code: A Handbook of Agile Software Craftsmanship"](http://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882) - Robert C. Martin
* ["Refactoring: Improving the Design of Existing Code"](http://www.amazon.com/gp/product/0201485672/) - Martin Fowler et. al.
* ["The Rule of 30: When is a method, class or subsystem too big?"](http://swreflections.blogspot.com/2012/12/rule-of-30-when-is-method-class-or.html) - Jim Bird
* ["The Single Responsibility Principle"](http://programmer.97things.oreilly.com/wiki/index.php/The_Single_Responsibility_Principle) - Robert C. Martin
14 changes: 12 additions & 2 deletions lib/cc/engine/rubocop.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
require "rubocop"
require "rubocop/cop/method_complexity_patch"
require "cc/engine/category_parser"
require "active_support"
require "active_support/core_ext"

module CC
module Engine
Expand Down Expand Up @@ -83,7 +85,7 @@ def violation_positions(location)
end

def violation_json(violation, local_path)
{
violation_hash = {
type: "Issue",
check_name: "Rubocop/#{violation.cop_name}",
description: violation.message,
Expand All @@ -93,7 +95,15 @@ def violation_json(violation, local_path)
path: local_path,
positions: violation_positions(violation.location),
},
}.to_json
}
body = content_body(violation.cop_name)
violation_hash.merge!(content: { body: body }) if body.present?
violation_hash.to_json
end

def content_body(cop_name)
path = File.expand_path("../../../../config/contents/#{cop_name.underscore}.md", __FILE__)
File.exist?(path) ? File.read(path) : nil
end
end
end
Expand Down
21 changes: 19 additions & 2 deletions spec/cc/engine/rubocop_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -201,10 +201,27 @@ def method(a,b,c,d,e,f,g)
assert_equal location, result["location"]
end

it "includes issue content when available" do
lines = " test\n" * 101
create_source_file("klass.rb", "class Klass\n#{lines}end")

output = run_engine

assert includes_content_for?(output, "Metrics/ClassLength")
end

def includes_check?(output, cop_name)
issues = output.split("\0").map { |x| JSON.parse(x) }
!!issues(output).detect { |i| i["check_name"] =~ /#{cop_name}$/ }
end

def includes_content_for?(output, cop_name)
issue = issues(output).detect { |i| i["check_name"] =~ /#{cop_name}$/ }

!!issues.detect { |i| i["check_name"] =~ /#{cop_name}$/ }
issue["content"]["body"].present?
end

def issues(output)
issues = output.split("\0").map { |x| JSON.parse(x) }
end

def create_source_file(path, content)
Expand Down