Every repository with this icon (
Every repository with this icon (
| Description: | Code smell detector for Ruby edit |
-
Currently smell warnings describe the code element in which the smell sits, but this can be ambiguous or vague (for example, nested iterators in a large method can be hard to identify). Reek should offer %f (filename) and %l (line number) in smell reports.
Comments
-
3 comments Created 7 months ago by kevinrutherfordInline exception configurationfeaturexWould be nice to be able to disable smells for specific bits of code instead of using the config file. for example...
def my_method #:nosmell[utility_function] ... end[Raised by S. Brent Faulkner]
Comments
kevinrutherford
Thu Apr 23 09:04:42 -0700 2009
| link
The latest version of ruby_parser provides access to comments immediately preceeding methods etc. Which means configuration could be done like this:
# :reek: exclude from UtilityFunction def my_method ... endI haven't verified this technique yet, and clearly I also need to work on the syntax. But the idea definitely has promise!
kevinrutherford
Thu Apr 23 09:05:25 -0700 2009
| link
And of course, this idea is predicated on fixing #4 first.
ashleymoran
Thu Apr 23 12:31:42 -0700 2009
| link
This would mean that you couldn't use
MyClass.reek?and inline smell exclusions at the same time though, right? Seems like this feature would obsolete ParseTree compatibility. -
(This is a special case of Dead Code.)
Comments
-
0 comments Created 6 months ago by ashleymoranClass methods defined in "class << self; end" not recognised as suchdefectxFor example, the following method reports Feature Envy (I think - I'd have to check). But Utility Function is the most common one that gets reported.
class ProposedQuestion class << self def new_for_facts_with_format(times_table_facts, question_format) times_table_facts.map { |fact| ProposedQuestion.new(:times_table_fact => fact, :question_format => question_format) } end end endComments
-
New smell detector.
Comments
-
The name Large Class is a hangover from Martin Fowler's original Java smells; in Ruby it should probably be renamed to Large Module.
Comments
-
0 comments Created 7 months ago by kevinrutherfordoption to execute a single smell detectorfeaturexAdd a command-line option that disables all but the named smell. This should be a rake option too.
Comments
-
0 comments Created 7 months ago by kevinrutherfordConfigure Nested Iterators on nesting depthsmellxCurrently the default behaviour is to complain about any block that's nested inside another. Add a config option to allow the user to raise this threshhold above 1.
Comments
-
0 comments Created 7 months ago by kevinrutherfordFeature Envy for blocks and code fragmentssmellxFeature Envy currently applies only to whole methods. It would be great to find a way to have it report a block, or the else-clause of a conditional, or any branch of a switch.
Comments
-
0 comments Created 7 months ago by kevinrutherfordAdapters are different from domain codesmellxIn a Hexagonal Architectural view of an app, the code style in the Adapters is special. So any name matching /Adapter/ should be exempted from certain smells (particularly those related to conditional code).
Comments
-
1 comment Created 7 months ago by kevinrutherfordCan't be a UtilityFunction when there's no enclosing moduledefectxWhen I just define a method at the top level in a script, it has no "instance state" to ignore, and will therefore always be a UtilityFunction. Doesn't make sense.
Comments
kevinrutherford
Fri May 08 12:14:57 -0700 2009
| link
Closely related to #27.
-
A class has a field that is not set in the constructor, but is used in some method that doesn't also set it.
Or a class provides an attr_writer.
Comments
ashleymoran
Mon Sep 28 10:49:34 -0700 2009
| link
Discussion about this started here: http://groups.google.com/group/ruby-reek/browse_thread/thread/e6f9568d1f3957c4
-
2 comments Created 7 months ago by kevinrutherfordFail on unnecessary exclusionsfeaturex[Raised by Ashley Moran]
Say you have the following config file:
--- FeatureEnvy: exclude: - viewbut the view method that gets analysed does not have Feature Envy. Should this be considered an error? It seems that exclusions could persist after the code smells have been removed, silently ignoring the smell being introduced.
I've done a few refactorings now that removed code smells, and had to remember to manually take them out of the configs.
Similar idea to code coverage failing if your coverage percentage increases on the basis that you want to maintain any improvements you make.Possibly the sanest behaviour would be to fail if the config is in the same directory as the file affected, so that global excludes don't raise loads of errors.
Comments
kevinrutherford
Thu Apr 23 12:29:08 -0700 2009
| link
So this would raise a smell warning if it is checking code that has no smells, but which would have been excluded even if it did have smells?
Yes, I can see the thinking here.
kevinrutherford
Sun Jun 14 12:22:55 -0700 2009
| link
How about an option (eg --config-report) that doesn't report the smells, but instead lists the number of smells found by each line of each config file that was used?
-
1 comment Created 7 months ago by kevinrutherfordLarge Class should not count added methods from validations / associations in Railsfeaturex[Raised originally by dbalatero]
Given an ActiveRecord model with a number of validations and associations added, and no extra methods, the method count can balloon to larger than the max count really fast.
I believe this is because validates*of-type methods in Rails add methods to the class itself, so they don't get seen as separate modules / inherited methods.
Would there be a way to whitelist some of these generated methods?
Comments
kevinrutherford
Thu Apr 23 12:32:13 -0700 2009
| link
There's no way to whitelist individual methods so they don't contribute to the class size. There's also no way to increase the size threshold on a per-class basis. But you can use a .reek file to whitelist the class against the LargeClass smell; see the wiki page at http://wiki.github.com/kevinruth... for hints.
-
4 comments Created 6 months ago by ashleymoranmodule_function does not disable Utility FunctiondefectxA bit like a method on a class, the following should presumably not trigger Utility Function?
module MyModule def reset! # ... end module_function :reset! endComments
kevinrutherford
Fri May 08 12:14:31 -0700 2009
| link
Yes; this is closely related to #21.
ashleymoran
Fri May 08 12:44:34 -0700 2009
| link
...and also related to this:
class C class << self def m; end end end?
Sure I submitted a ticket for this when it was on Lighthouse, but I can't find it here.
kevinrutherford
Fri May 08 12:46:40 -0700 2009
| link
Not sure that this is the same thing, but I'll check.
You emailed me, I think. Anyroad, it's here now :)
kevinrutherford
Thu May 21 02:36:44 -0700 2009
| link
And now it's also #29 here too.
-
Tools such as perlcritic categorise the smells they report with a "severity". Good idea.
Comments
-
The plug-in should mark up source files in the editor view, showing where the smells are.
Comments
kevinrutherford
Thu Apr 23 09:07:45 -0700 2009
| link
This is almost certainly dependent on #3 to get line numbers.
-
The plug-in should mark up source files in the editor view, showing where the smells are.
Comments
kevinrutherford
Thu Apr 23 09:08:16 -0700 2009
| link
This is almost certainly dependent on #3 to get line numbers.
-
0 comments Created 5 months ago by kevinrutherfordFeatureEnvy can miss envious instance variablesdefectxThe following method is envious
def func @other.a @other.b @nother.c endbecause it could be moved to @other, creating a method like tthis:
def other.func(me) a b me.nother_c endBut FeatureEnvy doesn't pick this up.
Comments
-
0 comments Created 5 months ago by kevinrutherfordLong Method should count nested assignmentssmellxThe following code includes 3 nested assignments, yet none of these contribute to the method's "length":
def parse(arg, argv, &error) if !(val = arg) and (argv.empty? or /\A-/ =~ (val = argv[0])) return nil, block, nil end opt = (val = parse_arg(val, &error))[1] val = conv_arg(*val) if opt and !arg argv.shift else val[0] = nil end val endThe metho'ds length is reported as 6, but should be 9.
Comments
-
Add an optino to write a YAML report into an output folder.
(Could look at metric_fu for the report's format?)Comments
-
0 comments Created 4 months ago by kevinrutherfordNeed a way to set different thresholds for the same smellfeaturexI'd like to be able to configure Reek so that, for example, some methods are allowed to have upto 5 parameters, while the rest must keep within the standard 3.
Comments
-
1 comment Created 4 months ago by ashleymoranConfigure reek per-file, not per-smellfeaturexThis came up today. We have a file of ugly monkey-patching hacks to workaround framework bugs, and we wanted to turn reek off for this. But we couldn't control the smells on a per-file basis, so we had to put them in one by one.
Being able to configure smells per-file or per-class/module would have made this easier.
WDYT?
Comments
kevinrutherford
Tue Jul 14 13:53:44 -0700 2009
| link
Or by placing comments in the source code?
-
1 comment Created 4 months ago by kevinrutherfordUncommunicativeName: 'accept' should be called 'allow'featurex -
0 comments Created 4 months ago by kevinrutherfordValidate contents of config filesfeaturexFor example, it should be a fatal error if the max_allowed_calls setting for Duplication is set to < 2.
Comments
-
Every class and module should have a brief description of its responsibilities in the comments just ahead of its definition
Comments
-
Attempt to detect when a method has been added to a core class.
Comments
kevinrutherford
Thu Sep 03 11:54:57 -0700 2009
| link
The rationale here is that a core extension is dangerous (it may clash with another); so instead of extending a core class, create a new one of your own.
-
New smell: More than 8 calls to
requirein a single source file.Comments
-
0 comments Created 2 months ago by kevinrutherfordProvide YAML output for ease of parsingfeaturexTools such as metric_fu currently parse Reek's text output. It would be easier for them to load a YAML file produced by Reek. So I propose a new option --format taking one of (initially) three values:
none,yaml, ortext; the last would produce the current output, and would be the default.Comments
-
2 comments Created about 1 month ago by ashleymoranBang method without non-bang methodsmellxI've had some of this in my code. Good Ruby style says that in the following code,
class Thing def do_something end def do_something! end endThe second, ! method should only exist if the first also does. Is this in the scope of reek? I propose PrimaDonnaMethod as the smell name if so :)
Comments
kevinrutherford
Fri Oct 02 05:06:41 -0700 2009
| link
Mmm; I agree, but is this a smell or a style issue?
ashleymoran
Fri Oct 02 16:06:23 -0700 2009
| link
Possibly a style issue. But maybe it's comparable to Uncommunicative Name?
-
I'm now convinced that the this code pattern is also code smell. Is it something Reek could detect? (If you believe it should be a code smell?)
Comments
kevinrutherford
Fri Oct 02 05:08:33 -0700 2009
| link
It's right up there at the very extreme end of the smell spectrum! I guess it could be detected as Implementation Inheritance...?
ashleymoran
Fri Oct 02 16:08:46 -0700 2009
| link
How would you do that, look for local method calls that are defined in a superclass?
ashleymoran
Fri Oct 02 16:36:55 -0700 2009
| link
Oh no wait, isn't Template Method is the other way round, local method calls that are only defined (or overridden) in subclasses?
ashleymoran
Fri Oct 02 16:38:02 -0700 2009
| link
Actually shouldn't overriding a method be a smell too, as a violation of the LSP? (Having said that, I've done it myself this week as it was a convenient solution to a problem...)
kevinrutherford
Sat Oct 03 05:42:29 -0700 2009
| link
...which is why I say it's right at the extreme end of the spectrum :)
Sometimes inheritance is just the best way to go; and sometimes Template Method is a good temporary solution to a problem. Reek needs to admit a little more subjectivity before I'll add these as smells -- see #47 for that.
-
1 comment Created about 1 month ago by ashleymoranFeature Envy reported even when it is reporting Utility FunctiondefectxFor example:
class Level1Generator def classes(row, col, value) (row == 1 || col == 1) ? %w[ header ] : [ ] end endGets both smells. Surely it's not informative to report Feature Envy in this case?
Comments
kevinrutherford
Fri Oct 02 05:04:45 -0700 2009
| link
Yes, and it was this very thing that led me to begin questioning the value of Feature Envy and Utility Function.
-
2 comments Created about 1 month ago by ashleymoranDataClump not reported on closely related classesfeaturexNow please try and resist tearing too much into the other smells here :) But I believe this deserves flagging as a Data Clump?
module CellClassGenerator class Level1Generator def initialize(multiplicand, multiplier) @multiplicand = multiplicand @multiplier = multiplier end def classes(row, col, value) (row == 1 || col == 1) ? %w[ header ] : [ ] end end # TODO figure out why this isn't reported as a data clump class Level2Generator < Level1Generator def classes(row, col, value) classes = super classes << "highlighted" if col == @multiplicand classes end end class Level3Generator < Level2Generator def classes(row, col, value) classes = super classes << "highlighted" if row == @multiplier classes << "answer" if value == @multiplicand * @multiplier && col == @multiplicand classes.uniq end end end(You can't see it here, but if we had
CellReference, I'm pretty sure some related code in this HTML table generator module would clean up too.)Comments
kevinrutherford
Sat Oct 03 05:38:46 -0700 2009
| link
Agree. Right now Reek only looks at the methods in a single class; clearly it could be extended. However, bear in mind that this is only a check on the parameters' names, because Reek can't intuit the parameter types. So extending it might yield false positives; is that a price worth paying?
ashleymoran
Sat Oct 03 05:53:36 -0700 2009
| link
Hmmm, maybe it's worth an experiment? You could start increasing the extent of DataClumps that reeks complains about until reek users start complaining about reek :)
Two obvious starting points are
Classes in the same hierarchy should be grouped Classes nested at the same level in an explicit module or class should be groupedEither would make reek bitch about the code above.
Also, I'd be interested to know if warnings generated bet extending the scope of DataClump throw up new forms of UncommunicativeName.
-
Module methods seperated by :: opposed to #
0 comments Created about 13 hours ago by danmayerIn the Reek output it normally separates class with ::, and denotes methods with # or . Inside a module it doesn't do that correctly
reports like so http://assets.devver.net/github_crnixon_wordbadger/6d6a19064e7b51bfd2507117aee41e216379dd5d/reek.html
'ApplicationHelper::section_link' which should be 'ApplicationHelper#section_link'
This makes it hard to tell the difference between a class and a method... Obviously lower case names can't be classes, but it seems using the # is more the standard.
Comments












This would be dependent on ruby_parser offering line numbers, which it doesn't yet do.