Skip navigation

Cost of technical debt calculation via Perl::Critic

Tudor Constantin asked about extending Perl::Critic to calculate the total cost of violations. You can do this yourself without much work without any code changes to Perl::Critic. Perl::Critic has a --statistics-only option which doesn’t emit any individual violations, but just some figures about the code that it looked at. The output looks like this:

perl -Ilib bin/perlcritic --force -1 --statistics-only bin lib
   194 files.
 1,468 subroutines/methods.
15,194 statements.

45,950 lines, consisting of:
     6,342 blank lines.
     3,833 comment lines.
     2,275 data lines.
    16,388 lines of Perl code.
    17,112 lines of POD.

Average McCabe score of subroutines was 2.94.

2,951 violations.
Violations per file was 15.211.
Violations per statement was 0.194.
Violations per line of code was 0.064.

   13 severity 5 violations.
   31 severity 4 violations.
   32 severity 3 violations.
2,364 severity 2 violations.
  511 severity 1 violations.

    6 violations of BuiltinFunctions::ProhibitStringyEval.
    1 violations of ClassHierarchies::ProhibitAutoloading.
    2 violations of CodeLayout::ProhibitParensWithBuiltins.
  192 violations of CodeLayout::RequireTidyCode.
    4 violations of ControlStructures::ProhibitCStyleForLoops.
    1 violations of ControlStructures::ProhibitCascadingIfElse.
    6 violations of ControlStructures::ProhibitPostfixControls.
  165 violations of Documentation::PodSpelling.
1,736 violations of Documentation::RequirePodSections.
    2 violations of ErrorHandling::RequireCarping.
    2 violations of ErrorHandling::RequireCheckingReturnValueOfEval.
    1 violations of InputOutput::ProhibitBacktickOperators.
    2 violations of InputOutput::ProhibitInteractiveTest.
    7 violations of InputOutput::RequireBriefOpen.
    1 violations of InputOutput::RequireCheckedSyscalls.
  579 violations of Miscellanea::RequireRcsKeywords.
    1 violations of NamingConventions::Capitalization.
    1 violations of RegularExpressions::ProhibitComplexRegexes.
   86 violations of RegularExpressions::ProhibitUnusualDelimiters.
   14 violations of RegularExpressions::RequireBracesForMultiline.
    4 violations of Subroutines::ProhibitExplicitReturnUndef.
    6 violations of Subroutines::ProhibitUnusedPrivateSubroutines.
   20 violations of Subroutines::RequireArgUnpacking.
    3 violations of Subroutines::RequireFinalReturn.
    1 violations of TestingAndDebugging::ProhibitNoStrict.
    1 violations of TestingAndDebugging::ProhibitNoWarnings.
    2 violations of ValuesAndExpressions::ProhibitMagicNumbers.
   37 violations of ValuesAndExpressions::ProhibitNoisyQuotes.
   50 violations of ValuesAndExpressions::RequireInterpolationOfMetachars.
   11 violations of Variables::ProhibitPackageVars.
    6 violations of Variables::ProhibitReusedNames.
    1 violations of Variables::ProtectPrivateVars.

Munging this output to fit Mr. Constantin’s desired output is left as an exercise for the reader.

Test Moose accessors? Hell, yes.

I’m sorry Dave, but you’re wrong, wrong, wrong! :]

The purpose of testing “attributes” is not to test that Moose did its thing. The purpose of testing attributes’ accessors is that they are part of your class’ interface. That an accessor is backed by a Moose attribute is an implementation detail. Just because get_foo() is an attribute accessor today doesn’t mean that the data won’t be stored elsewhere in the future or even be derived from something else.

What you shouldn’t be doing is using has_attribute_ok() from Test::Moose, again because where the data returned by the accessor is coming from shouldn’t matter to users of your class.

Don’t put Perl::Critic into source control pre-commit hooks.

Some people seem to like to put Perl::Critic tests into their VCS pre-commit hooks in order to prevent code with violations being checked in. I consider this to be a really bad idea. You should always be able to check your code in, if only for backup purposes. It’s a matter of preserving your code, even if it’s got some stupid PodSpelling violation. You’ve got a production emergency bug fix and you can’t check in because you’ve got a ProhibitEscapedMetacharacters problem? I certainly wouldn’t want that.

No Devel::Declare. Ever.

I think Devel::Declare is evil for the same reasons that Adam does.

Even if PPI supports it, Perl::Critic will not be supporting any syntax that’s not part of the language core. Not only because it would be necessary to figure out semantics, but because I oppose balkanization of the language.

To quote myself: “Please add a ‘class. keyword. Would that break MooseX::Declare? Yup. Tough cookies.“

If it’s broken, fix it — dammit.

I was reminded of the epigram “If it’s broken, don’t fix it.”. My problem with this is that people don’t apply the contrapositive. So many just leave things that don’t work around, and that plain sucks.

Don’t suffer brokenness. Fix it.

Explicitly running author tests.

There are tests that you want to have around, but not be part of the standard test run prior to installation of a distribution. Probably the biggest set of these in publicly available Perl distributions is “author” tests (manifest checks, code and POD coverage, etc.), but there are other things such as live database tests that should not block installation. The most common way of disabling tests is via checking an environment variable in a test file:


1 if ( not $ENV{TEST_AUTHOR}) {
2     plan skip_all => ‘Author test. Set $ENV{TEST_AUTHOR} to a true value to run.‘;
3 }

On the Perl::Critic project, we ran into problems with this. We got failures when people had the environment variable set for their own purposes. The code that checked for enabling author tests also turned them on if there was a .svn directory around, so, of course, we got a complaint when someone grabbed the source from the Perl::Critic repository. *sigh*

The proper way to enable author tests is to require the user to explicitly say that she wants them to be run.

The first thing to do is to segregate the non-standard tests from the regular ones so that the standard testing tools don’t run them by default. At the 2008 QA hackathon in Oslo it was decided that these tests belong under the “xt” directory in a distribution; there was no mandate about substructure. I’ve been using “xt/author” for my author tests.

The second step is to create a “authortest” build target. Since I use Module::Build as my build tool, this example will use that. The way to create a custom target in Module::Build is to create a method on a Module::Build subclass with a name that starts with “ACTION_”. Here’s my standard “authortest” code:


 1 sub ACTION_authortest {
 2     my ($self) = @_;
 3
 4     $self->depends_on(‘build‘);
 5     $self->depends_on(‘manifest‘);
 6     $self->depends_on(‘distmeta‘);
 7
 8     $self->test_files( qw< t xt/author > );
 9     $self->recursive_test_files(1);
10
11     $self->depends_on(‘test‘);
12
13     return;
14 }

First this asks Module::Build to ensure that the build, manifest, and distmeta actions have been run because some of the tests depend upon the MANIFEST and META.yml files being around and I don’t check those into source control. Line 8 tells Module::Build to look in the standard “t” directory plus “xt/author” for tests. Line 9 says that the test files may not be directly in these subdirectories. Finally, it asks for the regular test action to be run.

Using this, running the standard “./Build test” command doesn’t run author tests, no matter what the user’s environment is. And simply running “./Build authortest” causes all the tests to run. Simple, yet explicit.

To ensure that nothing gets released without the author tests being run, I change the distdir action to require the author tests be run:


1 sub ACTION_distdir {
2     my ($self) = @_;
3
4     $self->depends_on(‘authortest‘);
5
6     return $self->SUPER::ACTION_distdir();
7 }

How to structure Perl programs.

Don’t put the implementation of your program in your program; put it in a module instead. There are at least three reasons for doing things this way:

  • Your code is more testable. You can feed in arbitrary command lines and check the exit code.
  • Your code is reusable. If you need to use your program from another Perl program, there’s no need to use system, backticks, or IPC::System::Simple, just use your module and call the implementing subroutine.
  • Your programs can be better managed by the standard Perl toolchain. Program versions are not checked by CPAN, et. al. so if you create a new version of an existing program, it won’t be upgraded. If all of your code is in a module, then version changes will cause updates to be installed.

A simple way of doing this would be to have a module that looks like


 1 package ProgramImplementation;
 2
 3 use utf8;
 4 use 5.010;
 5
 6 use strict;
 7 use warnings;
 8
 9 use Exporter qw< import >;
10
11 our @EXPORT_OK = qw< run >;
12
13
14 sub run {
15     my (@argv) = @_;
16
17     …
18
19     return 0;
20 }
21
22 1;

and then use that like this:


 1 use utf8;
 2 use 5.010;
 3
 4 use strict;
 5 use warnings;
 6
 7 use ProgramImplementation qw< run >;
 8
 9 return 1 if caller;
10 exit run(@ARGV);

Having the “return 1 if caller” in there means that the program can be required without causing the requiring program to exit. (Whether you ought to actually do that is another matter.)

If you want things to be even more reusable, make your program implementation an object with attributes for the standard file handles:


 1 package ObjectProgramImplementation;
 2
 3 use utf8;
 4 use 5.010;
 5
 6 use Moose;      # Or your favorite object module.
 7
 8 has stdout => (
 9     isa => ‘FileHandle‘,
10     …
11     default => sub { \*STDOUT },
12 );
13
14 has stderr => (
15     isa => ‘FileHandle‘,
16     …
17     default => sub { \*STDERR },
18 );
19
20
21 sub run {
22     my ($self, @argv) = @_;
23
24     say { $self->stdout() } ‘Hello there!‘;
25     …
26
27     return 0;
28 }
29
30 1;

Your regular program would then look like this:


 1 use utf8;
 2 use 5.010;
 3
 4 use strict;
 5 use warnings;
 6
 7 use ObjectProgramImplementation qw< >;
 8
 9 return 1 if caller;
10 exit ObjectProgramImplementation->new()->run(@ARGV);

But you can have other uses like


 1 my $stdout;
 2 my $stderr;
 3 open my $stdout_handle, ‘>‘, \$stdout;
 4 open my $stderr_handle, ‘>‘, \$stderr;
 5
 6 my $program =
 7     ObjectProgramImplementation->new(stdout => $stdout, stderr => $stderr);
 8 my $exit_code = $program->run(@ARGV);
 9
10 # Do something with $stdout/$stderr

Thank you Tom Wyant.

From the Perl::Critic 1.104 Changes file:

This release is dedicated to Tom Wyant in appreciation of the amount of effort he put into the enhancements and bug fixes in this release, for having the patience to wait for the amount of time that it took to get them out, and for overall awesomeness. Thank you, Tom.

Deprecation.

In the source repository, there are a number of changes to Perl::Critic that include deprecation of existing functions/methods. One Policy author has asked us to not do the deprecation because it will result in warnings being emitted whenever his Policy is being used.

There have been a lot of discussion recently in the Perl community about what deprecation means. For me, deprecation means that the interface or functionality will be removed and not sit around in a limbo state for years.

So how do I decide whether to deprecate something? If something is merely suboptimal, I’m not going to deprecate it; I’ll merely change the documentation to discourage its use. In Perl::Critic, there was the issue of Policy constructors. For a long time, there was no requirements for the new() method for a given Policy other than it returning a blessed hash. There are some Policys [sic] outside of the core distribution with new() methods that don’t call up to the new() method in Perl::Critic::Policy. There was a point when we really wanted to add a lot of functionality to the constructor, but we couldn’t because there were Policys that didn’t call it. We worked around this by doing what we wanted after the Policy object was constructed. If you look at the developer documentation for Perl::Critic, you’ll note that it discourages creation of a new() method.

On the other hand, if something is incorrectly designed or is holding up future changes, I’m going to change it or remove it. I’m not going to just work around the issue forever. I won’t change it without notifying anyone, however.

I made a mistake with the design of Perl::Critic::Utils::PPI::get_constant_name_element_from_declaring_statement(). I didn’t realize that you can actually create multiple values using the constant pragma; Tom Wyant corrected my misconception. The function returns a single value, so it can’t correctly handle single statements that declare multiple constants. Its interface is wrong. It will be removed in the future. A suggestion was made to immediately remove it, but we’ll go through a deprecation cycle first. We won’t change things without giving notice.

I think the Policy author’s question about the warnings comes from the practice of a number of projects of “deprecating” something, but never actually removing it. Sun is a particularly egregious example of this with Java functionality that has been deprecated for more than a decade but still hangs around, allowing people to write new code against an interface that is broken. If I deprecate something, it will be removed. The warnings that are emitted allow others to deal with the change before it happens. If, in the interim, this annoys some users, too bad. It would be worse if the code just stopped working.

If you have a look at my code page, among the epigrams that you find there is “If it’s broken, fix it.”. While there has long been the saying of not fixing something that isn’t broken, I think people don’t do the opposite enough. Don’t suffer breakage. Don’t put up with things that are wrong. Change things that need changing, even if it causes some short-term pain.

How I use Perl::Critic.

Under regular development conditions, I rarely use perlcritic. Most of my Perl::Critic usage is via Test::Perl::Critic for new code and Test::Perl::Critic::Progressive for existing code.

For code that I release to the CPAN, the Perl::Critic tests are in xt/author so that they don’t get run as part of normal tests that someone installing the modules runs. On someone else’s system, I don’t know what Perl::Critic version or add-on sets of policies they have, so those tests may fail there through no fault of my own.

For non-public code, the Perl::Critic tests are regular tests that get run with all the rest of the tests. I can do this because I control the specific version of Perl::Critic that I’m running and which add-on policies that I’ve got around.

Perl::Critic supports the concept of severity levels for policies and, by default, only applies the most severe ones (severity 5). I effectively ignore the severities by turning Perl::Critic’s strictness all the way to 1. Of course, there are policies that I don’t agree with or cannot use for practical reasons, so I disable those explicitly.

You may have noticed that above I used the word “tests” and not “test”. This is because I run Perl::Critic on my tests as well as my “real” code. The set of policies that I use on tests is different than I use on the rest of my code. Tests have fewer restrictions on them compared to regular code. So I have a test that applies to the contents of lib and bin using one perlcriticrc file and a seperate test that runs on the content of the t and xt directories with a different perlcriticrc. For tests, I disable the POD policies and Subroutines::ProtectPrivateSubs (tests are allowed to peek). Sometimes I also disable ValuesAndExpressions::ProhibitMagicNumbers, though I prefer not to.

(Perl::Critic actually uses three self-compliance tests: one for the regular code, one for the tests, and one that makes an additional pass over the Perl::Critic::Policy subclasses that places some additional requirements on them.)

The sets of add-on policies that I generally use are Perl::Critic::More, Perl::Critic::Bangs, Perl::Critic::Swift, and Perl::Critic::Moose. I’m thinking about starting to use Perl::Critic::Pulp, but I’d have to disable about half the policies in there.

There are a couple of cases in which I do use perlcritic.

First, if I’m surveying a codebase that is new to me, I will run perlcritic --statistics-only over it in order to get an idea of the state of affairs.

Second, presented with a set of code that has a large number of problems that need fixing, after running perlcritic --statistics-only on it, I’ll pick an individual policy that I want to fix violations of and run perlcritic -s policy name on the code. (The long form of -s is --single-policy.) I find it easier to fix one kind of problem across all of the files before moving on to another kind of problem than to fix all the problems in one file before moving on to another file.