Skip to content

Commit d78e903

Browse files
author
ABaldwinHunter
committed
Report issue for each instance of duplicated code
Previously, we just chose the first location where a duplication occurred, and report the others in `other_locations`.
1 parent bca74b3 commit d78e903

File tree

8 files changed

+109
-66
lines changed

8 files changed

+109
-66
lines changed

config/contents/duplicated_code.md.erb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ When you violate DRY, bugs and maintenance problems are sure to follow. Duplicat
99
## Issue Mass
1010

1111
Duplicated code has a calculated mass, which can be thought of as a measure of how much logic has been duplicated.
12-
This issue has a mass of `<%= issue.mass %>`: if you would like to change the minimum mass that will be reported as an issue, please see the details in [`codeclimate-duplication`'s documentation](https://github.com/codeclimate/codeclimate-duplication).
12+
This issue has a mass of `<%= mass %>`: if you would like to change the minimum mass that will be reported as an issue, please see the details in [`codeclimate-duplication`'s documentation](https://github.com/codeclimate/codeclimate-duplication).
1313

1414
## Refactorings
1515

lib/cc/engine/analyzers/reporter.rb

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,11 @@ def report
3838
flay.report(StringIO.new).each do |issue|
3939
violation = new_violation(issue)
4040

41-
unless reports.include?(violation.report_name)
42-
reports.add(violation.report_name)
43-
io.puts "#{violation.format.to_json}\0"
41+
violation.occurrences.each do |occurrence|
42+
unless reports.include?(occurrence.report_name)
43+
reports.add(occurrence.report_name)
44+
io.puts "#{occurrence.format.to_json}\0"
45+
end
4446
end
4547
end
4648
end

lib/cc/engine/analyzers/violation.rb

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,17 @@ module Analyzers
88
class Violation
99
attr_reader :issue
1010

11-
def initialize(language_strategy, issue, hashes)
11+
def initialize(language_strategy, issue, hashes, target_index = 0)
1212
@language_strategy = language_strategy
1313
@issue = issue
1414
@hashes = hashes
15+
@target_index = target_index
16+
end
17+
18+
def occurrences
19+
hashes.map.with_index do |_, i|
20+
Violation.new(language_strategy, issue, hashes, i)
21+
end
1522
end
1623

1724
def format
@@ -32,28 +39,34 @@ def report_name
3239
"#{current_sexp.file}-#{current_sexp.line}"
3340
end
3441

35-
def mass
36-
flay_score / occurrences # ABH Reverse engineer the actual mass.
42+
def mass # ABH Reverse engineer the actual mass from flay score.
43+
if issue.identical?
44+
flay_score / (occurrences.length**2)
45+
else
46+
flay_score / occurrences.length
47+
end
3748
end
3849

3950
private
4051

41-
attr_reader :language_strategy, :hashes
52+
attr_reader :language_strategy, :hashes, :target_index
4253

4354
def flay_score
4455
issue.mass
4556
end
4657

4758
def current_sexp
48-
@location ||= sorted_hashes.first
59+
@location ||= sorted_hashes[target_index]
4960
end
5061

5162
def sorted_hashes
5263
@_sorted_hashes ||= hashes.sort_by(&:file)
5364
end
5465

5566
def other_sexps
56-
@other_locations ||= sorted_hashes.drop(1)
67+
sorted = sorted_hashes
68+
sorted.delete_at(target_index)
69+
sorted
5770
end
5871

5972
def check_name
@@ -90,7 +103,7 @@ def format_sexp(sexp)
90103
end
91104

92105
def content_body
93-
@_content_body ||= { "body": ViolationReadUp.new(issue).contents }
106+
@_content_body ||= { "body": ViolationReadUp.new(mass).contents }
94107
end
95108

96109
def fingerprint
@@ -99,19 +112,15 @@ def fingerprint
99112
digest << "-"
100113
digest << current_sexp.mass.to_s
101114
digest << "-"
102-
digest << occurrences.to_s
115+
digest << occurrences.length.to_s
103116
digest.to_s
104117
end
105118

106119
def description
107-
description = "#{check_name} found in #{occurrences - 1} other location"
108-
description += "s" if occurrences > 2
120+
description = "#{check_name} found in #{(occurrences.length - 1)} other location"
121+
description += "s" if occurrences.length > 2
109122
description
110123
end
111-
112-
def occurrences
113-
other_sexps.count + 1
114-
end
115124
end
116125
end
117126
end

lib/cc/engine/analyzers/violation_read_up.rb

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,8 @@ module CC
44
module Engine
55
module Analyzers
66
class ViolationReadUp
7-
def initialize(issue)
8-
@issue = issue
7+
def initialize(mass)
8+
@mass = mass
99
end
1010

1111
def contents
@@ -14,7 +14,7 @@ def contents
1414

1515
private
1616

17-
attr_reader :issue
17+
attr_reader :mass
1818

1919
TEMPLATE_REL_PATH = "../../../../config/contents/duplicated_code.md.erb"
2020

spec/cc/engine/analyzers/javascript/main_spec.rb

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@
1616
console.log("hello JS!");
1717
EOJS
1818

19-
result = run_engine(engine_conf).strip
19+
issues = run_engine(engine_conf).strip.split("\0")
20+
result = issues.first.strip
21+
2022
json = JSON.parse(result)
2123

2224
expect(json["type"]).to eq("issue")
@@ -27,13 +29,13 @@
2729
"path" => "foo.js",
2830
"lines" => { "begin" => 1, "end" => 1 },
2931
})
30-
expect(json["remediation_points"]).to eq(297000)
32+
expect(json["remediation_points"]).to eq(33000)
3133
expect(json["other_locations"]).to eq([
3234
{"path" => "foo.js", "lines" => { "begin" => 2, "end" => 2} },
3335
{"path" => "foo.js", "lines" => { "begin" => 3, "end" => 3} }
3436
])
35-
expect(json["content"]["body"]).to match /This issue has a mass of `99`/
36-
expect(json["fingerprint"]).to eq("55ae5d0990647ef496e9e0d315f9727d")
37+
expect(json["content"]["body"]).to match /This issue has a mass of `11`/
38+
expect(json["fingerprint"]).to eq("d3e06d4d67aa65ea09a190bae018053e")
3739
end
3840

3941
it "prints an issue for similar code" do
@@ -43,7 +45,9 @@
4345
console.log("helllllllllllllllllo JS!");
4446
EOJS
4547

46-
result = run_engine(engine_conf).strip
48+
issues = run_engine(engine_conf).strip.split("\0")
49+
result = issues.first.strip
50+
4751
json = JSON.parse(result)
4852

4953
expect(json["type"]).to eq("issue")
@@ -54,13 +58,13 @@
5458
"path" => "foo.js",
5559
"lines" => { "begin" => 1, "end" => 1 },
5660
})
57-
expect(json["remediation_points"]).to eq(99000)
61+
expect(json["remediation_points"]).to eq(33000)
5862
expect(json["other_locations"]).to eq([
5963
{"path" => "foo.js", "lines" => { "begin" => 2, "end" => 2} },
6064
{"path" => "foo.js", "lines" => { "begin" => 3, "end" => 3} }
6165
])
62-
expect(json["content"]["body"]).to match /This issue has a mass of `33`/
63-
expect(json["fingerprint"]).to eq("55ae5d0990647ef496e9e0d315f9727d")
66+
expect(json["content"]["body"]).to match /This issue has a mass of `11`/
67+
expect(json["fingerprint"]).to eq("d3e06d4d67aa65ea09a190bae018053e")
6468
end
6569

6670
it "skips unparsable files" do
@@ -91,8 +95,8 @@
9195
<a className='button button-primary full' href='#' onClick={this.onSubmit.bind(this)}>Login</a>
9296
EOJSX
9397

94-
result = run_engine(engine_conf).strip
95-
issues = result.split("\0")
98+
issues = run_engine(engine_conf).strip.split("\0")
99+
96100
expect(issues.length).to eq 1
97101
end
98102

spec/cc/engine/analyzers/php/main_spec.rb

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,9 @@
2929
}
3030
EOPHP
3131

32-
result = run_engine(engine_conf).strip
32+
issues = run_engine(engine_conf).strip.split("\0")
33+
result = issues.first.strip
34+
3335
json = JSON.parse(result)
3436

3537
expect(json["type"]).to eq("issue")
@@ -40,17 +42,18 @@
4042
"path" => "foo.php",
4143
"lines" => { "begin" => 2, "end" => 6 },
4244
})
43-
expect(json["remediation_points"]).to eq(176000)
45+
expect(json["remediation_points"]).to eq(44_000)
4446
expect(json["other_locations"]).to eq([
4547
{"path" => "foo.php", "lines" => { "begin" => 10, "end" => 14} },
4648
])
47-
expect(json["content"]["body"]).to match /This issue has a mass of `44`/
48-
expect(json["fingerprint"]).to eq("667da0e2bab866aa2fe9d014a65d57d9")
49+
expect(json["content"]["body"]).to match /This issue has a mass of `11`/
50+
expect(json["fingerprint"]).to eq("5ede2c52b49dc6542f62cfaa7efcebf1")
4951
end
5052

5153
it "runs against complex files" do
5254
FileUtils.cp(fixture_path("symfony_configuration.php"), File.join(@code, "configuration.php"))
53-
result = run_engine(engine_conf).strip
55+
issues = run_engine(engine_conf).strip.split("\0")
56+
result = issues.first.strip
5457

5558
expect(result).to match "\"type\":\"issue\""
5659
end
@@ -91,11 +94,6 @@
9194
end
9295
end
9396

94-
def printed_issue
95-
issue = {"type":"issue","check_name":"Identical code","description":"Similar code found in 1 other location","categories":["Duplication"],"location":{"path":"foo.php","lines":{"begin":2,"end":6}},"remediation_points":176000,"other_locations":[{"path":"foo.php","lines":{"begin":10,"end":14}}],"content":{"body": read_up}}
96-
issue.to_json + "\0\n"
97-
end
98-
9997
def engine_conf
10098
CC::Engine::Analyzers::EngineConfig.new({
10199
'config' => {

spec/cc/engine/analyzers/python/main_spec.rb

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@
1616
print("Hello", "python")
1717
EOJS
1818

19-
result = run_engine(engine_conf).strip
19+
issues = run_engine(engine_conf).strip.split("\0")
20+
result = issues.first.strip
21+
2022
json = JSON.parse(result)
2123

2224
expect(json["type"]).to eq("issue")
@@ -27,13 +29,13 @@
2729
"path" => "foo.py",
2830
"lines" => { "begin" => 1, "end" => 1 },
2931
})
30-
expect(json["remediation_points"]).to eq(3_000_000)
32+
expect(json["remediation_points"]).to eq(1_600_000)
3133
expect(json["other_locations"]).to eq([
3234
{"path" => "foo.py", "lines" => { "begin" => 2, "end" => 2} },
3335
{"path" => "foo.py", "lines" => { "begin" => 3, "end" => 3} }
3436
])
35-
expect(json["content"]["body"]).to match /This issue has a mass of `54`/
36-
expect(json["fingerprint"]).to eq("42b832387c997f54a2012efb2159aefc")
37+
expect(json["content"]["body"]).to match /This issue has a mass of `6`/
38+
expect(json["fingerprint"]).to eq("cc22115aa7c7d115b799fcdf668195d6")
3739
end
3840

3941
it "prints an issue for similar code" do
@@ -43,7 +45,9 @@
4345
print("Hello from the other side", "python")
4446
EOJS
4547

46-
result = run_engine(engine_conf).strip
48+
issues = run_engine(engine_conf).strip.split("\0")
49+
result = issues.first.strip
50+
4751
json = JSON.parse(result)
4852

4953
expect(json["type"]).to eq("issue")
@@ -54,13 +58,13 @@
5458
"path" => "foo.py",
5559
"lines" => { "begin" => 1, "end" => 1 },
5660
})
57-
expect(json["remediation_points"]).to eq(1_920_000)
61+
expect(json["remediation_points"]).to eq(1_600_000)
5862
expect(json["other_locations"]).to eq([
5963
{"path" => "foo.py", "lines" => { "begin" => 2, "end" => 2} },
6064
{"path" => "foo.py", "lines" => { "begin" => 3, "end" => 3} }
6165
])
62-
expect(json["content"]["body"]).to match /This issue has a mass of `18`/
63-
expect(json["fingerprint"]).to eq("42b832387c997f54a2012efb2159aefc")
66+
expect(json["content"]["body"]).to match /This issue has a mass of `6`/
67+
expect(json["fingerprint"]).to eq("cc22115aa7c7d115b799fcdf668195d6")
6468
end
6569

6670

0 commit comments

Comments
 (0)