{"thread":{"id":"19412","subject":"gitweb: Comments on perlcritic suggestions with severity 3 (harsh)","startedAt":"2009-05-19T08:40:13Z","lastAt":"2009-05-19T08:40:13Z","messageCount":1,"participants":["Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"114241","messageId":"200905191040.15155.jnareb@gmail.com","threadId":"19412","inReplyTo":null,"subject":"gitweb: Comments on perlcritic suggestions with severity 3 (harsh)","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-05-19T08:40:13Z","receivedAt":"2009-05-19T08:40:13Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Below there are my comments on perlcritic (Perl::Critic) suggestions\nfor gitweb. As suggestions with severity 5 and 4 were covered in cover\nletter for cleanup series:\n  [PATCH 0/5] gitweb: Some code cleanups (up to perlcritic --stern)\n  Message-Id: <200905100203.51744.jnareb@gmail.com>\n  http://thread.gmane.org/gmane.comp.version-control.git/118690\nthis post is about severity 3 (harsh) suggestions only. I have tried\nto exclude issues which were addressed in second cleanup series:\n  [PATCH 0/4] gitweb: Some code cleanups, part 2 (low hanging fruit)\n  Message-ID: <20090511173025.15152.94215.stgit@localhost.localdomain>\n  http://thread.gmane.org/gmane.comp.version-control.git/118812\nBoth of those series are already in 'next' as merge (or merge of early\nparts) from 'jn/gitweb-cleanup' feature branch.\n\nI am sending those comments as I do not know where I would have time\nto work on issues mentioned here. But perhaps that would help\nsomebody...\n\nThis post was inspired by similar series of patches for \ngit-send-email by Bill Pemberton:\n  Subject: [PATCH 0/6] cleanups for git-send-email\n  Msg-Id: <1241010743-7020-1-git-send-email-wfp5p@virginia.edu>\n  URL: http://thread.gmane.org/gmane.comp.version-control.git/117881\n\nThe comments are based on documentation of policy modules for perlcritic\n(Perl::Critic).  Most policy modules are in turn based on Damian\nConway's book \"Perl Best Practices\" (PBP).\n\nI have used online version of perlcritic at http://perlcritic.com, which\nis running Perl-Critic version 1.096).\n\ngitweb: Some code cleanups\n\nThe following are some code cleanups for gitweb.perl.  They're based on\nsuggestions by perlcritic (Perl::Critic).  Most policy modules are in turn\nbased on Damian Conway's book \"Perl Best Practices\" (PBP).\n\nThis series was inspired by similar series of patches for git-send-email by\nBill Pemberton:\n  Subject: [PATCH 0/6] cleanups for git-send-email\n  Msg-Id: <1241010743-7020-1-git-send-email-wfp5p@virginia.edu>\n  URL: http://thread.gmane.org/gmane.comp.version-control.git/117881\n\nNot all suggestions by perlcritic (or, to be more exact, by its online\nversion at http://perlcritic.com, which is running Perl-Critic version\n1.096) were implemented.  Below there is list of perlcritic suggestions,\nsorted by severity (gentle, stern, harsh, cruel, brutal): first list of\npatches in series which applied perlcritic suggestions, then suggestions\nnot applied, with short explanation why they were not used.\n\n......................................................................\nSeverity: harsh (3)\n\nMain code has high complexity score (54) at line 1, column 1.\nConsider refactoring.  Severity: 3\n\nNOTE: Yeah, right.  The code is '#!/usr/bin/perl'\n\n\nPackage variable declared or used at line 27, column 1.\nSee pages 73,75 of PBP.  Severity: 3\nPerl::Critic::Policy::Variables::ProhibitPackageVars\n## no critic (ProhibitPackageVars)\n\n - Eliminate globals declared with our or use vars.\n\n  Conway suggests avoiding package variables completely, because they expose\n  your internals to other packages. Never use a package variable when a\n  lexical variable will suffice. If your package needs to keep some dynamic\n  state, consider using an object or closures to keep the state private.\n\n  This policy assumes that you're using strict vars so that naked variable\n  declarations are not package variables by default. Thus, it complains you\n  declare a variable with our or use vars, or if you make reference to\n  variable with a fully-qualified package name.\n\n* PBP 5.2. Package Variables (Don't use package variables in your own\n  development.)\n\nNOTE: This is about 'our $base_url = $my_url;' etc., i.e. using 'our'\ninstead of 'my' to declare variables.  Unfortunately this is required to be\nable to use gitweb from mod_perl, see commit 4a87b43e by Dennis Stosberg:\n\n    gitweb: Declare global variables with \"our\"\n    \n    Variables declared with \"my\" in the file scope cannot be accessed from\n    subroutines with mod_perl.\n\n\nRegular expression without \"/x\" flag at line XXX, column XX.\nSee page 236 of PBP.  Severity: 3\nPerl::Critic::Policy::RegularExpressions::RequireExtendedFormatting\n## no critic (RequireExtendedFormatting) <-- perhaps in some cases...\n\n - Always use the /x modifier with regular expressions.\n\n  Extended regular expression formatting allows you mix whitespace and\n  comments into the pattern, thus making them much more readable.\n\n  You might find that putting a /x on short regular expressions to be\n  excessive.  [The minimum_regex_length_to_complain_about defaults to 0]\n\n* PBP 12.1. Extended Formatting (Always use the /x flag.)\n\nNOTE: This is a matter of taste.\n\n\n\"warn\" used instead of \"carp\" at line 381, column 3.\nSee page 283 of PBP.  Severity: 3\nPerl::Critic::Policy::ErrorHandling::RequireCarping\n\n - Use functions from Carp instead of warn or die.\n\n  The die and warn functions both report the file and line number where the\n  exception occurred. But if someone else is using your subroutine, they\n  usually don't care where _your_ code blew up. Instead, they want to know\n  where _their_ code invoked the subroutine. The Carp module provides\n  alternative methods that report the exception from the caller's file and\n  line number.\n\n  By default, this policy will not complain about die or warn, if it can\n  determine that the message will always result in a terminal newline. Since\n  perl suppresses file names and line numbers in this situation, it is\n  assumed that no stack traces are desired either and none of the Carp\n  functions are necessary.\n\n* PBP 13.6. Reporting Failure (Have exceptions report from the caller's\n  location, not from the place where they were thrown.)\n\nNOTE: This is about use of 'warn' in gitweb_get_feature. Probably should\njust add end of line character (sic!), which suppresses adding file and\nline number info; information which we don't really need in this case\n(I have not found where this behavior of \"warn\" is documented).\n\n\nBacktick operator used at line 476, column 20.\nUse IPC::Open3 instead.  Severity: 3\nPerl::Critic::Policy::InputOutput::ProhibitBacktickOperators\n## no critic (ProhibitBacktickOperators) <-- TODO anyway\n\n - Discourage stuff like @files = `ls $directory`.\n\n  Backticks are super-convenient, especially for CGI programs, but I find\n  that they make a lot of noise by filling up STDERR with messages when they\n  fail. I think its better to use IPC::Open3 to trap all the output and let\n  the application decide what to do with it.\n\nNOTE: IPC::Open3 is overkill for setting $git_version.  On the other hand we\nshould perhaps fail early here if $GIT is not found.\n\n\nSubroutine \"some_sub\" with high complexity score (nn) at line XXX, column XX.\nConsider refactoring.  Severity: 3\nPerl::Critic::Policy::Subroutines::ProhibitExcessComplexity\n## no critic (ProhibitExcessComplexity) <-- left for later\n\n - Minimize complexity by factoring code into smaller subroutines.\n\n  All else being equal, complicated code is more error-prone and more\n  expensive to maintain than simpler code. The first step towards managing\n  complexity is to establish formal complexity metrics. One such metric is\n  the McCabe score, which describes the number of possible paths through a\n  subroutine. This Policy approximates the McCabe score by summing the\n  number of conditional statements and operators within a\n  subroutine. Research has shown that a McCabe score higher than 20 is a\n  sign of high-risk, potentially untestable code. See\n  http://www.sei.cmu.edu/str/descriptions/cyclomatic_body.html for some\n  discussion about the McCabe number and other complexity metrics.\n\n  The usual prescription for reducing complexity is to refactor code into\n  smaller subroutines. Mark Dominus book \"Higher Order Perl\" also describes\n  callbacks, recursion, memoization, iterators, and other techniques that\n  help create simple and extensible Perl code.\n\n  [...]\n\n  Complexity is subjective, but formal complexity metrics are still\n  incredibly valuable. Every problem has an inherent level of complexity, so\n  it is not necessarily optimal to minimize the McCabe number. So don't get\n  offended if your code triggers this Policy. Just consider if there might\n  be a simpler way to get the job done.\n\nFOOTNOTE: The link mentioned above is available only through The Internet\nArchive (Wayback Machine) although the information is also available in PDF\nfile (supposedly for limited time); you can try 'Cyclomatic complexity'\narticle on Wikipedia (and links therein).\n\nNOTE: The following subroutines were marked as having high complexity score,\n evaluate_path_info (44), href (31), format_diff_line (29),\n git_get_projects_list (21), parse_commit_text (27), git_header_html (33),\n git_difftree_body (54), git_patchset_body (24), git_project_list_body (34),\n git_summary (22), git_blame (23), git_blob (22), git_tree (25),\n git_blobdiff (39), git_commitdiff (56), git_search (51), git_feed (56).\n\nBelow they are sorted with greater complexity measure first:\n  \n  Subroutine                   complexity score \n  name                         (McCabe number)\n  ==============================================\n  git_commitdiff               56\n  git_feed                     56\n  git_difftree_body            54\n  git_search                   51\n  evaluate_path_info           44\n  git_blobdiff                 39\n  git_project_list_body        34\n  git_header_html              33\n  href                         31\n  format_diff_line             29\n  parse_commit_text            27\n  git_tree                     25\n  git_patchset_body            24\n  git_blame                    23\n  git_summary                  22\n  git_blob                     22\n  git_get_projects_list        21\n\n\nCapture variable used outside conditional at line XXX, column XX.\nSee page 253 of PBP.  Severity: 3\nPerl::Critic::Policy::RegularExpressions::ProhibitCaptureWithoutTest\n\n - Capture variable used outside conditional.\n\n  If a regexp match fails, then any capture variables ($1, $2, ...) will be\n  undefined. Therefore it's important to check the return value of a match\n  before using those variables.\n\n  This policy checks that capture variables are inside a conditional and do\n  not follow an regexps.\n\n  BUGS: Needs to allow 'next unless /$regexp/' construct.\n\n* PBP 12.15. Captured Values (Use the numeric capture variables only when\n  you're sure that the preceding match succeeded.)\n\n   Pattern matches that fail never assign anything to $1, $2, etc., nor do\n   they leave those variables undefined. After an unsuccessful pattern\n   match, the numeric capture variables remain exactly as they were before\n   the match was attempted. Often, that means that they retain whatever\n   values some earlier successful pattern match gave them.\n\nNOTE: Below there are different kind of cases, with different severity:\n* line 697 in evaluate_path_info (very cryptic, if it does what I think\n  it does, and can be simplified avoiding this warning), \n* lines 1199, 1208, 1210, 1220, 1221 in chop_str (might be an error if it\n  doesn't match),\n* line 2021 in git_get_hash_by_path, and line 2628 in parse_ls_tree_line,\n  and lines 5820, 5821 in git_object (which assumes specified output\n  format); git_object shouldn't perhaps do parsing by itself.\n* line 2375 in parse_date (which assumes specific format of timezone info in\n  time info; we should protect against errors as it was freeform and some\n  older git didn't set it up correctly),\n* line 2823 in mimetype_guess_file assumes that filename ends in extension\n  which is assured by caller, but perhaps that should be also used in this\n  subroutine to return early, just in case.\n\n\nReused variable name in lexical scope: $var at line XXX, column XX.\nInvent unique variable names.  Severity: 3\nPerl::Critic::Policy::Variables::ProhibitReusedNames \n\n - Do not reuse a variable name in a lexical scope\n\n  It's really hard on future maintenance programmers if you reuse a variable\n  name in a lexical scope. The programmer is at risk of confusing which\n  variable is which. And, worse, the programmer could accidentally remove\n  the inner declaration, thus silently changing the meaning of the inner\n  code to use the outer variable.\n\nNOTE: This suggestion occurs in the following places:\n* In project_in_list(), which is a very short subroutine; the name $project\n  is quite natural here, and we do not use global variable $project.\n  Perhaps to be named differently.\n* Line 1208 in chop_str, where we (perhaps by accident) redeclare $str\n  variable. To be fixed!\n* Line 1757 at beginning of format_snapshot_links, where we override $hash\n  with value we get as parameter, shadowing $hash with local variable of the\n  same name; while we shouldn't overwrite global variable perhaps parameter\n  name COULD be (SHOULD be) changed to something else other than $hash.\n* Line 1848 at beginning of git_get_head_hash, where we reuse $project for\n  name of function argument; perhaps it should be named $path of $repository\n  or something, or use '$project ||= shift'.\n* Line 1867 at beginning of git_get_type, where name of function argument is\n  $hash (same as global variable) and perhaps it should be $object, or\n  $object_hash...\n* Line 1883 at beginning of hash_set_multi (an utility function), where\n  argument should perhaps be named $hashref rather than $hash.\n* %config is both name of global variable storing parsed commit, and the\n  name of local variable in (utility) subroutine git_parse_project_config,\n  which is returned on exit, and which result is set by caller of this\n  subroutine to global variable %config.  To be replaced in the future by\n  Git::Config.\n* git_get_path_by_hash reuses $hash for the name of parameter.\n* git_get_project_owner reuses $project.\n* git_get_rev_name_tags passes $hash by parameter (and reuses $hash\n  for parameter name).\n* git_get_heads_list reuses $hash for local variable in line 2703.\n* blob_contenttype reuses $file_name for local variable in line 2871.\n* git_header_html reuses $action in line 3023 to mean -action parameter form\n  $cgi->startform method (\"action\" attribute in <form> element); better name\n  it $action_uri, or $form_action, or $form_uri.\n* format_paging_nav passes $action, $hash, $page via arguments instead of\n  using global variables; same with git_print_header_div and $action, $hash,\n  $hash_base.\n* git_get_link_target reuses $hash for parameter name ($object_hash or\n  $symlink_object_hash).\n* normalize_link_target reuses $hash_base for parameter name (or reuses\n  $hash_base for parameter name)... but current lazy (no lookup)\n  implementation doesn't use it at all. TODO !!! (TO BE CORRECTED).\n* git_print_tree_entry reuses (I think) $hash_base for parameter name;\n  I guess it is $basedir_hash or $hash_basedir.\n* git_difftree_body passes $hash as parameter instead of using global\n  variable\n* git_difftree_body uses $hash_parent as local variable in line 3586\n  (in a loop over all parents in combined diff).\n* git_patchset_body passes $hash as parameter and reuses $hash_parent as a\n  local variable (it used to pass $hash_parent as parameter, but now it\n  supports also combined diff with multiple parents).\n* git_history_body passes $hash_base as parameter.\n\n\nAmbiguously named variable \"nnnn\" at line XXX, column XX.\nSee page 48 of PBP.  Severity: 3\nPerl::Critic::Policy::NamingConventions::ProhibitAmbiguousNames\n## no critic (ProhibitAmbiguousNames) <-- very subjective\n\n - Don't use vague variable or subroutine names like 'last' or 'record'.\n\n  Conway lists a collection of English words which are highly ambiguous as\n  variable or subroutine names. For example, $last can mean previous or\n  final.\n\n  This policy tests against a list of ambiguous words for variable names.\n\n  The default list of forbidden words is:\n    abstract bases close contract last left no record right second set\n\n* PBP 3.9. Ambiguous Names (Avoid using inherently ambiguous words in\n  names.)\n\n\nCascading if-elsif chain at line XXXX, column X.\nSee pages 117,118 of PBP.  Severity: 3\nPerl::Critic::Policy::ControlStructures::ProhibitCascadingIfElse\n## no critic (ProhibitCascadingIfElse) <-- can't use 'switch', \n                                           can't simplify otherwise\n\n - Don't write long \"if-elsif-elsif-elsif-elsif...else\" chains.\n\n  Long if-elsif chains are hard to digest, especially if they are longer\n  than a single page or screen. If testing for equality, use a hash lookup\n  instead. See 'Switch' for another approach.\n\n  The default maximum number of elsif alternatives to allow is 2.\n\n* PBP 6.15. Multipart Selections (Avoid cascading an if.)\n\n   Avoid cascades of if-elsif-elsif-else statements wherever possible. They\n   tend to produce code with poor readability that is also expensive to\n   execute.\n\nNOTE: I'd rather avoid using 'Switch' module / 'switch' command.\n\nLong if-elsif chain is also used for \"range\" testing in age_string, and for\nmode testing using S_SOMETHING subroutines in mode_str, file_type and\nfile_type_long.\n\nLong if-elsif chain is also used in format_diff_line subroutine, where\nperhaps in first case it could be avoided (NOTE strange code using\ne.g. tr/+/+/ to count number of occurences, when the fact that there was\noccurence is enough), in second case it could be replaced by hash lookup.\n\nLong if-elsif chain is used in parse_tag and parse_commit_text subroutines,\nwhere we match using regexps against known headers (capturing and saving\nrelevant information).\n\nLong if-elsif chain is used in blob_mimetype; main part is checking using\nregexp if filename ends in specified known extension.\n\nLong if-elsif chain is used git_difftree_body to check against possible\nsingle letter status (this part should be simplified, the chain might be\nreplaced by hash lookup).\n\n\nHard tabs used at line 1389, column 11.\nSee page 20 of PBP.  Severity: 3\nPerl::Critic::Policy::CodeLayout::ProhibitHardTabs \n\n - Use spaces instead of tabs.\n\n  Putting hard tabs in your source code (or POD) [...] anywhere other than a\n  leading position.\n\n* PBP 2.11. Tabs (Indent with spaces, not tabs.)\n* PBP 4.3. Single-Character Strings (Don't write one-character strings in\n  visually ambiguous ways.) <-- this includes literal tabs in strings.\n\nNOTE: It is about: \n* using tabs in replacement part of 's{...}{...}eg;' regexp in\n  format_log_line_html subroutine.\n\n-- \nJakub Narebski\nPoland\n"}]}