From a0dd44a74a6e6cf268509bf8662476fa3715ca1f Mon Sep 17 00:00:00 2001
From: Andrew Yates <ayates@ebi.ac.uk>
Date: Tue, 28 May 2013 10:24:55 +0000
Subject: [PATCH] [ENSCORESW-478] and [ENSCORESW-467]. Brought in a
 Perl::Critic test being run on the lowest level. Tests are only run when we
 have the ENV TEST_AUTHOR is set to a true value. Otherwise it does not impact
 on the current test cases. Also had to fix a number of issues associcated
 with these flags. Mostly were due to not using warnings or strict. Anything I
 felt was ok or not my call has been annotated with a no critic flag.

---
 modules/Bio/EnsEMBL/BaseAlignFeature.pm       |  2 +-
 modules/Bio/EnsEMBL/ChainedAssemblyMapper.pm  |  9 ++--
 .../Bio/EnsEMBL/DB/ExternalFeatureFactoryI.pm |  4 ++
 modules/Bio/EnsEMBL/DBFile/FileAdaptor.pm     |  3 +-
 modules/Bio/EnsEMBL/DBSQL/BaseAdaptor.pm      |  2 +-
 modules/Bio/EnsEMBL/DBSQL/GeneAdaptor.pm      | 27 ++++++------
 modules/Bio/EnsEMBL/External/BlastAdaptor.pm  |  4 +-
 .../Bio/EnsEMBL/IdMapping/ExonScoreBuilder.pm |  7 +--
 .../StableIdGenerator/AnophelesGambiae.pm     |  2 +-
 .../Bio/EnsEMBL/IdMapping/SyntenyFramework.pm |  2 +-
 modules/Bio/EnsEMBL/IndividualSlice.pm        |  9 ++--
 modules/Bio/EnsEMBL/PaddedSlice.pm            | 11 +++--
 .../Bio/EnsEMBL/Pipeline/ChecksumGenerator.pm |  2 +-
 .../Bio/EnsEMBL/Pipeline/Flatfile/FindDirs.pm |  2 +-
 .../EnsEMBL/Pipeline/Production/PepStats.pm   | 12 +++---
 modules/Bio/EnsEMBL/ProjectionSegment.pm      |  3 ++
 modules/Bio/EnsEMBL/Registry.pm               | 12 +++---
 modules/Bio/EnsEMBL/StrainSlice.pm            | 22 ++++++----
 modules/Bio/EnsEMBL/SubSlicedFeature.pm       |  3 ++
 modules/Bio/EnsEMBL/Utils/Collector.pm        |  5 ++-
 modules/Bio/EnsEMBL/Utils/ConfParser.pm       | 11 ++---
 .../Bio/EnsEMBL/Utils/ConversionSupport.pm    | 19 ++++----
 modules/Bio/EnsEMBL/Utils/Converter.pm        |  2 +-
 .../Utils/Converter/ens_bio_featurePair.pm    |  2 +-
 modules/Bio/EnsEMBL/Utils/Exception.pm        |  2 +
 modules/Bio/EnsEMBL/Utils/Proxy.pm            |  8 +++-
 modules/Bio/EnsEMBL/Utils/SchemaConversion.pm |  6 +--
 modules/Bio/EnsEMBL/Utils/ScriptUtils.pm      |  4 +-
 modules/t/externalFeatureAdaptor.t            |  5 ++-
 modules/t/fullIdCaching.t                     |  2 +-
 modules/t/gffSerialiser.t                     |  3 ++
 modules/t/housekeeping_perlCritic.t           | 43 +++++++++++++++++++
 modules/t/lruIdCaching.t                      |  4 +-
 modules/t/mapper.t                            | 11 +++--
 modules/t/proteinAlignFeatureAdaptor.t        |  4 +-
 modules/t/seqEdit.t                           |  2 +
 modules/t/utilsScalar.t                       |  2 +-
 perlcriticrc                                  |  4 ++
 38 files changed, 184 insertions(+), 93 deletions(-)
 create mode 100644 modules/t/housekeeping_perlCritic.t
 create mode 100644 perlcriticrc

diff --git a/modules/Bio/EnsEMBL/BaseAlignFeature.pm b/modules/Bio/EnsEMBL/BaseAlignFeature.pm
index 335e527045..16aa70c9ad 100644
--- a/modules/Bio/EnsEMBL/BaseAlignFeature.pm
+++ b/modules/Bio/EnsEMBL/BaseAlignFeature.pm
@@ -623,7 +623,7 @@ sub _parse_features {
   my $hstrand     = $f[0]->hstrand;
   my $slice       = $f[0]->slice();
   my $hslice       = $f[0]->hslice();
-  my $name        = $slice->name() if($slice);
+  my $name        = $slice ? $slice->name() : undef;
   my $hname       = $f[0]->hseqname;
   my $score       = $f[0]->score;
   my $percent     = $f[0]->percent_id;
diff --git a/modules/Bio/EnsEMBL/ChainedAssemblyMapper.pm b/modules/Bio/EnsEMBL/ChainedAssemblyMapper.pm
index b673bb1d36..119f2d6c9e 100644
--- a/modules/Bio/EnsEMBL/ChainedAssemblyMapper.pm
+++ b/modules/Bio/EnsEMBL/ChainedAssemblyMapper.pm
@@ -70,11 +70,6 @@ normal assembly mapper.
 
 =cut
 
-
-my $FIRST = 'first';
-my $MIDDLE = 'middle';
-my $LAST  = 'last';
-
 package Bio::EnsEMBL::ChainedAssemblyMapper;
 
 use strict;
@@ -86,6 +81,10 @@ use Bio::EnsEMBL::Mapper::RangeRegistry;
 use Bio::EnsEMBL::Utils::Exception qw(throw deprecate);
 use Scalar::Util qw(weaken);
 
+my $FIRST = 'first';
+my $MIDDLE = 'middle';
+my $LAST  = 'last';
+
 #2^20 = approx 10^6
 my $CHUNKFACTOR = 20;
 
diff --git a/modules/Bio/EnsEMBL/DB/ExternalFeatureFactoryI.pm b/modules/Bio/EnsEMBL/DB/ExternalFeatureFactoryI.pm
index 74481cfd65..8c549dd6c6 100755
--- a/modules/Bio/EnsEMBL/DB/ExternalFeatureFactoryI.pm
+++ b/modules/Bio/EnsEMBL/DB/ExternalFeatureFactoryI.pm
@@ -131,6 +131,10 @@ above.
 =cut
 
 package Bio::EnsEMBL::DB::ExternalFeatureFactoryI;
+
+use strict;
+use warnings;
+
 use Bio::EnsEMBL::External::ExternalFeatureAdaptor;
 use vars qw(@ISA);
 
diff --git a/modules/Bio/EnsEMBL/DBFile/FileAdaptor.pm b/modules/Bio/EnsEMBL/DBFile/FileAdaptor.pm
index 89b40066b5..fe9a1aab29 100755
--- a/modules/Bio/EnsEMBL/DBFile/FileAdaptor.pm
+++ b/modules/Bio/EnsEMBL/DBFile/FileAdaptor.pm
@@ -126,8 +126,7 @@ sub open_file{
 	throw("Cannot perform open with unsupported operator:\t${file_op}${filepath}");
   }
 
-  my $fh;
-  my $success = open($fh, "${file_op}${filepath}");
+  my $success = open my $fh, $file_op, $filepath;
   #$fh will be still be GLOB on fail
   
   #These warn instead of throw/die to allow
diff --git a/modules/Bio/EnsEMBL/DBSQL/BaseAdaptor.pm b/modules/Bio/EnsEMBL/DBSQL/BaseAdaptor.pm
index d2f514093b..634dac750e 100755
--- a/modules/Bio/EnsEMBL/DBSQL/BaseAdaptor.pm
+++ b/modules/Bio/EnsEMBL/DBSQL/BaseAdaptor.pm
@@ -1003,7 +1003,7 @@ sub get_dumped_data {
     my $data = shift;
 
     $data =~ s/\n|\r|\f|\\//g;
-    return eval ($data);
+    return eval ($data); ## no critic
 }
 
 
diff --git a/modules/Bio/EnsEMBL/DBSQL/GeneAdaptor.pm b/modules/Bio/EnsEMBL/DBSQL/GeneAdaptor.pm
index 2d390514b7..c1c67ea1b8 100644
--- a/modules/Bio/EnsEMBL/DBSQL/GeneAdaptor.pm
+++ b/modules/Bio/EnsEMBL/DBSQL/GeneAdaptor.pm
@@ -515,12 +515,11 @@ sub fetch_all_by_Slice_and_external_dbname_link {
   my $dbe_adaptor = $self->db()->get_DBEntryAdaptor();
 
   my %linked_genes;
-  foreach $external_db_id (@external_db_ids) {
-	my @linked_genes = $dbe_adaptor->list_gene_ids_by_external_db_id($external_db_id);
-
-	foreach my $gene_id (@linked_genes) {
-	  $linked_genes{$gene_id} = 1;
-	}
+  foreach my $local_external_db_id (@external_db_ids) {
+    my @linked_genes = $dbe_adaptor->list_gene_ids_by_external_db_id($local_external_db_id);
+    foreach my $gene_id (@linked_genes) {
+      $linked_genes{$gene_id} = 1;
+    }
   }
 
   # Get all the genes on the slice.
@@ -1878,9 +1877,11 @@ sub fetch_all_by_exon_supporting_evidence {
 	throw("feature type must be dna_align_feature or protein_align_feature");
   }
 
-  my $anal_from = ", analysis a " if ($analysis);
-  my $anal_where = "AND a.analysis_id = f.analysis_id AND a.analysis_id=? "
-	if ($analysis);
+  my ($anal_from, $anal_where);
+  if($analysis) {
+    $anal_from = ", analysis a ";
+    $anal_where = "AND a.analysis_id = f.analysis_id AND a.analysis_id=? ";
+  }
 
   my $sql = qq(
       SELECT DISTINCT(g.gene_id)
@@ -1943,9 +1944,11 @@ sub fetch_all_by_transcript_supporting_evidence {
 	throw("feature type must be dna_align_feature or protein_align_feature");
   }
 
-  my $anal_from = ", analysis a " if ($analysis);
-  my $anal_where = "AND a.analysis_id = f.analysis_id AND a.analysis_id=? "
-	if ($analysis);
+  my ($anal_from, $anal_where);
+  if($analysis) {
+    $anal_from = ", analysis a ";
+    $anal_where = "AND a.analysis_id = f.analysis_id AND a.analysis_id=? ";
+  }
 
   my $sql = qq(
       SELECT DISTINCT(g.gene_id)
diff --git a/modules/Bio/EnsEMBL/External/BlastAdaptor.pm b/modules/Bio/EnsEMBL/External/BlastAdaptor.pm
index 7288ccc5d1..1f44c7c865 100644
--- a/modules/Bio/EnsEMBL/External/BlastAdaptor.pm
+++ b/modules/Bio/EnsEMBL/External/BlastAdaptor.pm
@@ -847,9 +847,9 @@ sub get_all_SearchFeatures {
 sub dynamic_use {
   my( $self, $classname ) = @_;
   my( $parent_namespace, $module ) = $classname =~/^(.*::)(.*?)$/;
-  no strict 'refs';
+  no strict 'refs'; ## no critic
   return 1 if $parent_namespace->{$module.'::'}; # return if already used
-  eval "require $classname";
+  eval "require $classname"; ## no critic
   if($@) {
     warn "DrawableContainer: failed to use $classname\nDrawableContainer: $@";
     return 0;
diff --git a/modules/Bio/EnsEMBL/IdMapping/ExonScoreBuilder.pm b/modules/Bio/EnsEMBL/IdMapping/ExonScoreBuilder.pm
index e35a31432e..96d96b5bca 100644
--- a/modules/Bio/EnsEMBL/IdMapping/ExonScoreBuilder.pm
+++ b/modules/Bio/EnsEMBL/IdMapping/ExonScoreBuilder.pm
@@ -343,6 +343,7 @@ sub sort_exons {
   my $self  = shift;
   my $exons = shift;
 
+  ## no critic 
   return sort {
     ( $a->[0]->common_sr_name cmp $b->[0]->common_sr_name ) ||
       ( $a->[1] <=> $b->[1] )
@@ -497,7 +498,7 @@ sub run_exonerate {
                $self->conf()->param('lsf_opt_exonerate') );
 
   local *BSUB;
-  open( BSUB, $bsub_cmd )
+  open( BSUB, $bsub_cmd ) ## no critic
     or $self->logger->error("Could not open open pipe to bsub: $!\n");
 
   print BSUB $exonerate_job;
@@ -688,11 +689,11 @@ sub parse_exonerate_results {
 
     $num_files++;
 
-    open( F, '<', "$dump_path/$file" );
+    open( my $fh, '<', "$dump_path/$file" ); 
 
     my $threshold = $self->conf->param('exonerate_threshold') || 0.5;
 
-    while (<F>) {
+    while (<$fh>) {
       $num_lines++;
       chomp;
 
diff --git a/modules/Bio/EnsEMBL/IdMapping/StableIdGenerator/AnophelesGambiae.pm b/modules/Bio/EnsEMBL/IdMapping/StableIdGenerator/AnophelesGambiae.pm
index d0ed7ab172..6f86f0b600 100644
--- a/modules/Bio/EnsEMBL/IdMapping/StableIdGenerator/AnophelesGambiae.pm
+++ b/modules/Bio/EnsEMBL/IdMapping/StableIdGenerator/AnophelesGambiae.pm
@@ -18,7 +18,7 @@
 
 =cut
 
-package Bio::EnsEMBL::IdMapping::StableIdGenerator::AedesAegypti;
+package Bio::EnsEMBL::IdMapping::StableIdGenerator::AnophelesGambiae;
 
 # Package that implements incrementing and verification of Aedes aegypti
 # stable IDs as used by the VectorBase project.
diff --git a/modules/Bio/EnsEMBL/IdMapping/SyntenyFramework.pm b/modules/Bio/EnsEMBL/IdMapping/SyntenyFramework.pm
index 24c2461e57..659b683e97 100644
--- a/modules/Bio/EnsEMBL/IdMapping/SyntenyFramework.pm
+++ b/modules/Bio/EnsEMBL/IdMapping/SyntenyFramework.pm
@@ -390,7 +390,7 @@ sub rescore_gene_matrix_lsf {
   $self->logger->debug("$cmd\n\n");
 
   local *BSUB;
-  open( BSUB, $bsub_cmd )
+  open( BSUB, $bsub_cmd ) ## no critic
     or $self->logger->error("Could not open open pipe to bsub: $!\n");
 
   print BSUB $cmd;
diff --git a/modules/Bio/EnsEMBL/IndividualSlice.pm b/modules/Bio/EnsEMBL/IndividualSlice.pm
index 8a08a2433c..7d17892d79 100644
--- a/modules/Bio/EnsEMBL/IndividualSlice.pm
+++ b/modules/Bio/EnsEMBL/IndividualSlice.pm
@@ -169,7 +169,8 @@ sub seq {
 
     # sort edits in reverse order to remove complication of
     # adjusting downstream edits
-    my @allele_features_ordered = sort {$b->start() <=> $a->start() || $b->end() <=> $a->end()} @{$self->{'alleleFeatures'}} if (defined $self->{'alleleFeatures'});
+    my @allele_features_ordered;
+    @allele_features_ordered = sort {$b->start() <=> $a->start() || $b->end() <=> $a->end()} @{$self->{'alleleFeatures'}} if (defined $self->{'alleleFeatures'});
 
     foreach my $af (@allele_features_ordered){
 	$af->apply_edit($reference_sequence); #change, in the reference sequence, the af
@@ -343,7 +344,8 @@ sub mapper{
 	my $mapper = Bio::EnsEMBL::Mapper->new('Slice','IndividualSlice');
 	#align with Slice
 	#get all the VariationFeatures in the Individual Slice, from start to end in the Slice
-	my @allele_features_ordered = sort {$a->start() <=> $b->start() || $b->end() <=> $a->end()} @{$self->{'alleleFeatures'}} if (defined $self->{'alleleFeatures'});
+  my @allele_features_ordered;
+	@allele_features_ordered = sort {$a->start() <=> $b->start() || $b->end() <=> $a->end()} @{$self->{'alleleFeatures'}} if (defined $self->{'alleleFeatures'});
 	
 	my $start_slice = 1;
 	my $end_slice;
@@ -485,7 +487,8 @@ sub subseq {
     #apply all differences to the reference sequence
     # sort edits in reverse order to remove complication of
     # adjusting downstream edits
-    my @allele_features_ordered = sort {$b->start() <=> $a->start() || $b->end() <=> $a->end()} @{$self->{'alleleFeatures'}} if (defined $self->{'alleleFeatures'});
+    my @allele_features_ordered;
+    @allele_features_ordered = sort {$b->start() <=> $a->start() || $b->end() <=> $a->end()} @{$self->{'alleleFeatures'}} if (defined $self->{'alleleFeatures'});
     my $af_start;
     my $af_end;
     foreach my $af (@allele_features_ordered){
diff --git a/modules/Bio/EnsEMBL/PaddedSlice.pm b/modules/Bio/EnsEMBL/PaddedSlice.pm
index 60307851ad..747f869f4f 100644
--- a/modules/Bio/EnsEMBL/PaddedSlice.pm
+++ b/modules/Bio/EnsEMBL/PaddedSlice.pm
@@ -36,6 +36,9 @@ is carried out using C<subseq()> calls.
 
 package Bio::EnsEMBL::PaddedSlice;
 
+use strict;
+use warnings;
+
 use Bio::EnsEMBL::Utils::Argument qw/rearrange/;
 use Bio::EnsEMBL::Utils::Scalar qw/assert_ref assert_strand/;
 use base qw/Bio::EnsEMBL::Utils::Proxy/;
@@ -162,11 +165,11 @@ sub subseq {
   
   #Return if we were upstream of overlap
   if($start < $parent_start && $end < $parent_start) {
-    return N x (( $end - $start )+1);
+    return 'N' x (( $end - $start )+1);
   }
   #Return if we were downstream of overlap
   if($start > $parent_end && $end > $parent_end) {
-    return N x (( $end - $start )+1);
+    return 'N' x (( $end - $start )+1);
   }
   
   my $prefix  = '';
@@ -174,11 +177,11 @@ sub subseq {
   my $subslice_start = ($start - $parent_start)+1;
   my $subslice_end = ($end - $parent_start) + 1;
   if($start < $parent_start) {
-    $prefix = N x ($parent_start - $start);
+    $prefix = 'N' x ($parent_start - $start);
     $subslice_start = 1;
   }
   if($end > $parent_end) {
-    $suffix = N x ($end - $parent_end);
+    $suffix = 'N' x ($end - $parent_end);
     $subslice_end = (($parent_end - $parent_start)+1);
   }
   
diff --git a/modules/Bio/EnsEMBL/Pipeline/ChecksumGenerator.pm b/modules/Bio/EnsEMBL/Pipeline/ChecksumGenerator.pm
index 430a905e87..133a2e4957 100644
--- a/modules/Bio/EnsEMBL/Pipeline/ChecksumGenerator.pm
+++ b/modules/Bio/EnsEMBL/Pipeline/ChecksumGenerator.pm
@@ -135,7 +135,7 @@ sub checksum {
 
 sub permissions {
   my ($self, $file) = @_;
-  my $mode = 0666;
+  my $mode = 0666; ## no critic
   chmod($mode, $file) or $self->throw("Cannot perform the chmod to mode $mode for file $file");
   return;
 }
diff --git a/modules/Bio/EnsEMBL/Pipeline/Flatfile/FindDirs.pm b/modules/Bio/EnsEMBL/Pipeline/Flatfile/FindDirs.pm
index d68e72f9e7..d71bd68c3a 100644
--- a/modules/Bio/EnsEMBL/Pipeline/Flatfile/FindDirs.pm
+++ b/modules/Bio/EnsEMBL/Pipeline/Flatfile/FindDirs.pm
@@ -37,7 +37,7 @@ Allowed parameters are:
 
 =cut
 
-package Bio::EnsEMBL::Pipeline::FASTA::FindDirs;
+package Bio::EnsEMBL::Pipeline::Flatfile::FindDirs;
 
 use strict;
 use warnings;
diff --git a/modules/Bio/EnsEMBL/Pipeline/Production/PepStats.pm b/modules/Bio/EnsEMBL/Pipeline/Production/PepStats.pm
index d4bfc50936..3177908f0c 100644
--- a/modules/Bio/EnsEMBL/Pipeline/Production/PepStats.pm
+++ b/modules/Bio/EnsEMBL/Pipeline/Production/PepStats.pm
@@ -71,11 +71,11 @@ sub store_attrib {
 sub run_pepstats {
   my ($self, $tmpfile) = @_;
   my $PEPSTATS = $self->param('binpath') . '/bin/pepstats';
-  open(OUT, "$PEPSTATS -filter < $tmpfile 2>&1 |");
-  my @lines   = <OUT>;
+  open(my $fh, "$PEPSTATS -filter < $tmpfile 2>&1 |"); ## no critic
+  my @lines   = <$fh>;
   my $attribs = {};
   my $tid;
-  close(OUT);
+  close($fh);
   foreach my $line (@lines) {
 
 	if ($line =~ /PEPSTATS of ([^ ]+)/) {
@@ -130,7 +130,7 @@ sub dump_translation {
   my $helper = $dba->dbc()->sql_helper();
   my $dbtype = $self->param('dbtype');
   my $ta     = Bio::EnsEMBL::Registry->get_adaptor($self->param('species'), $dbtype, 'translation');
-  open(TMP, "> $tmpfile");
+  open(my $fh, '>', $tmpfile);
   my $sql = q{
     SELECT tl.translation_id
     FROM translation tl, transcript tr, seq_region s, coord_system cs
@@ -146,9 +146,9 @@ sub dump_translation {
 	if ($peptide_seq !~ /\n$/) {
 	  $peptide_seq .= "\n";
 	}
-	print TMP ">$dbid\n$peptide_seq";
+	print $fh ">$dbid\n$peptide_seq";
   }
-  close(TMP);
+  close($fh);
 } ## end sub dump_translation
 
 1;
diff --git a/modules/Bio/EnsEMBL/ProjectionSegment.pm b/modules/Bio/EnsEMBL/ProjectionSegment.pm
index e7c90a7e94..205717c052 100644
--- a/modules/Bio/EnsEMBL/ProjectionSegment.pm
+++ b/modules/Bio/EnsEMBL/ProjectionSegment.pm
@@ -50,6 +50,9 @@ $segment->from_start(), $segement->from_end(), $segment->to_Slice().
 
 package Bio::EnsEMBL::ProjectionSegment;
 
+use strict;
+use warnings;
+
 #
 # WARNING: THIS CLASS IS REPRESENTED BY A BLESSED ARRAY REFERENCE
 #  NOT A HASH REFERENCE
diff --git a/modules/Bio/EnsEMBL/Registry.pm b/modules/Bio/EnsEMBL/Registry.pm
index 19ff9a40ce..b7a0d35c27 100644
--- a/modules/Bio/EnsEMBL/Registry.pm
+++ b/modules/Bio/EnsEMBL/Registry.pm
@@ -355,7 +355,7 @@ sub load_all {
                             $adaptor, $section );
                 }
 
-                my $test_eval = eval "require $adaptor";
+                my $test_eval = eval "require $adaptor"; ## no critic
                 if ($@ or (!$test_eval)) { die($@) }
 
                 $adaptor->new(%adaptor_args);
@@ -370,7 +370,7 @@ sub load_all {
             if($NEW_EVAL) {
               require Bio::EnsEMBL::Utils::IO;
               my $contents = Bio::EnsEMBL::Utils::IO::slurp($config_file);
-              $test_eval = eval $contents;
+              $test_eval = eval $contents; ## no critic
             }
             else {
               $test_eval = eval { require($config_file) };
@@ -1024,7 +1024,7 @@ sub get_adaptor {
   my $dba = $registry_register{_SPECIES}{$species}{ lc($group) }{'_DB'};
   my $module = $ret;
 
-  my $test_eval = eval "require $module";
+  my $test_eval = eval "require $module"; ## no critic
   if ($@ or (!$test_eval)) {
     warning("'$module' cannot be found.\nException $@\n");
     return;
@@ -1856,7 +1856,7 @@ sub load_registry_from_db {
 
   # Variation
 
-  my $test_eval = eval "require Bio::EnsEMBL::Variation::DBSQL::DBAdaptor";
+  my $test_eval = eval "require Bio::EnsEMBL::Variation::DBSQL::DBAdaptor"; ## no critic
   if ($@or (!$test_eval)) {
     # Ignore variations as code required not there for this
     if ($verbose) {
@@ -1938,7 +1938,7 @@ sub load_registry_from_db {
     } ## end foreach my $multidb (@variation_multidbs)
   }
 
-  my $func_eval = eval "require Bio::EnsEMBL::Funcgen::DBSQL::DBAdaptor";
+  my $func_eval = eval "require Bio::EnsEMBL::Funcgen::DBSQL::DBAdaptor"; ## no critic
   if ($@ or (!$func_eval)) {
     if ($verbose) {
       # Ignore funcgen DBs as code required not there for this
@@ -2023,7 +2023,7 @@ sub load_registry_from_db {
   my @compara_dbs = grep { /^ensembl_compara/ } @dbnames;
 
   if (@compara_dbs) {
-    my $comp_eval = eval "require Bio::EnsEMBL::Compara::DBSQL::DBAdaptor";
+    my $comp_eval = eval "require Bio::EnsEMBL::Compara::DBSQL::DBAdaptor"; ## no critic
     if ($@ or (!$comp_eval)) {
       # Ignore Compara as code required not there for this
       if ($verbose) {
diff --git a/modules/Bio/EnsEMBL/StrainSlice.pm b/modules/Bio/EnsEMBL/StrainSlice.pm
index a34efdb27d..724b6dcb02 100644
--- a/modules/Bio/EnsEMBL/StrainSlice.pm
+++ b/modules/Bio/EnsEMBL/StrainSlice.pm
@@ -264,23 +264,25 @@ sub seq {
 	
     #apply all differences to the reference sequence
     #first, in case there are any indels, create the new sequence (containing the '-' bases)
-   # sort edits in reverse order to remove complication of
+    # sort edits in reverse order to remove complication of
     # adjusting downstream edits
-    my @indels_ordered = sort {$b->start() <=> $a->start()} @{$self->{'alignIndels'}} if (defined $self->{'alignIndels'});
+    my @indels_ordered;
+    @indels_ordered = sort {$b->start() <=> $a->start()} @{$self->{'alignIndels'}} if (defined $self->{'alignIndels'});
 
     foreach my $vf (@indels_ordered){
-	$vf->apply_edit($reference_sequence); #change, in the reference sequence, the vf
+      $vf->apply_edit($reference_sequence); #change, in the reference sequence, the vf
     }
 	
     #need to find coverage information if diffe
     # sort edits in reverse order to remove complication of
     # adjusting downstream edits
-    my @variation_features_ordered = sort {$b->start() <=> $a->start()} @{$self->{'alleleFeatures'}} if (defined $self->{'alleleFeatures'});
+    my @variation_features_ordered;
+    @variation_features_ordered = sort {$b->start() <=> $a->start()} @{$self->{'alleleFeatures'}} if (defined $self->{'alleleFeatures'});
 
     foreach my $vf (@variation_features_ordered){
-	$vf->apply_edit($reference_sequence); #change, in the reference sequence, the vf
+      $vf->apply_edit($reference_sequence); #change, in the reference sequence, the vf
     }
-	
+
     #need to find coverage information if different from reference
     my $indAdaptor = $self->adaptor->db->get_db_adaptor('variation')->get_IndividualAdaptor;
     my $ref_strain = $indAdaptor->get_reference_strain_name;
@@ -293,7 +295,7 @@ sub seq {
   return 'N' x $self->length();
 }
 
-sub expanded_length() {
+sub expanded_length {
 	my $self = shift;
 	
 	my $length = $self->SUPER::length();
@@ -335,7 +337,8 @@ sub _add_coverage_information{
 	# and unmasks seq where there is read coverage
 	
 	# get all length-changing vars
-	my @indels_ordered = sort {$a->start() <=> $b->start()} @{$self->{'alignIndels'}} if (defined $self->{'alignIndels'});
+  my @indels_ordered;
+	@indels_ordered = sort {$a->start() <=> $b->start()} @{$self->{'alignIndels'}} if (defined $self->{'alignIndels'});
 	
 	my $masked_seq = '~' x length($$reference_sequence);
 	
@@ -724,7 +727,8 @@ sub mapper{
 	my $mapper = Bio::EnsEMBL::Mapper->new('Slice','StrainSlice');
 	#align with Slice
 	#get all the VariationFeatures in the strain Slice, from start to end in the Slice
-	my @variation_features_ordered = sort {$a->start() <=> $b->start()} @{$self->{'alleleFeatures'}} if (defined $self->{'alleleFeatures'});
+  my @variation_features_ordered;
+	@variation_features_ordered = sort {$a->start() <=> $b->start()} @{$self->{'alleleFeatures'}} if (defined $self->{'alleleFeatures'});
 	
 	my $start_slice = 1;
 	my $end_slice;
diff --git a/modules/Bio/EnsEMBL/SubSlicedFeature.pm b/modules/Bio/EnsEMBL/SubSlicedFeature.pm
index 99059a829b..8cb07118db 100644
--- a/modules/Bio/EnsEMBL/SubSlicedFeature.pm
+++ b/modules/Bio/EnsEMBL/SubSlicedFeature.pm
@@ -43,6 +43,9 @@ my $transcripts = $truncated_gene->get_all_Transcripts();
 
 package Bio::EnsEMBL::SubSlicedFeature;
 
+use strict;
+use warnings;
+
 use Bio::EnsEMBL::Utils::Argument qw/rearrange/;
 use base qw/Bio::EnsEMBL::Utils::Proxy/;
 
diff --git a/modules/Bio/EnsEMBL/Utils/Collector.pm b/modules/Bio/EnsEMBL/Utils/Collector.pm
index c09e1d0c96..50a7db826e 100644
--- a/modules/Bio/EnsEMBL/Utils/Collector.pm
+++ b/modules/Bio/EnsEMBL/Utils/Collector.pm
@@ -85,11 +85,12 @@ bypassing object creation via the related BaseFeatureAdaptor method.
 
 package Bio::EnsEMBL::Utils::Collector;
 
-use Bio::EnsEMBL::Utils::Argument  ('rearrange');
-use Bio::EnsEMBL::Utils::Exception ('throw');
 use strict;
 use warnings;
 
+use Bio::EnsEMBL::Utils::Argument  ('rearrange');
+use Bio::EnsEMBL::Utils::Exception ('throw');
+
 ### Global package config vars
 
 # Defaults
diff --git a/modules/Bio/EnsEMBL/Utils/ConfParser.pm b/modules/Bio/EnsEMBL/Utils/ConfParser.pm
index 0553b3374a..7dc2aeaa6e 100644
--- a/modules/Bio/EnsEMBL/Utils/ConfParser.pm
+++ b/modules/Bio/EnsEMBL/Utils/ConfParser.pm
@@ -157,13 +157,13 @@ sub parse_options {
   $conffile = abs_path($conffile);
 
   if (-e $conffile) {
-    open(CONF, $conffile) or throw( 
+    open(my $fh, '<', $conffile) or throw( 
         "Unable to open configuration file $conffile for reading: $!");
 
     my $serverroot = $self->serverroot;
     my $last;
 
-    while (my $line = <CONF>) {
+    while (my $line = <$fh>) {
       chomp $line;
       
       # remove leading and trailing whitespace
@@ -200,6 +200,7 @@ sub parse_options {
       }
       $self->param($name, $val);
     }
+    close($fh);
 
     $self->param('conffile', $conffile);
   }
@@ -556,14 +557,14 @@ sub list_or_file {
     # we didn't get a list of values, but a file to read values from
     @vals = ();
     
-    open(IN, $firstval) or throw("Cannot open $firstval for reading: $!");
+    open(my $fh, '<', $firstval) or throw("Cannot open $firstval for reading: $!");
     
-    while(<IN>){
+    while(<$fh>){
       chomp;
       push(@vals, $_);
     }
     
-    close(IN);
+    close($fh);
     
     $self->param($param, @vals);
   }
diff --git a/modules/Bio/EnsEMBL/Utils/ConversionSupport.pm b/modules/Bio/EnsEMBL/Utils/ConversionSupport.pm
index 488288c85d..4c16938044 100644
--- a/modules/Bio/EnsEMBL/Utils/ConversionSupport.pm
+++ b/modules/Bio/EnsEMBL/Utils/ConversionSupport.pm
@@ -135,10 +135,10 @@ sub parse_common_options {
   my $conffile = $h{'conffile'} || $self->serverroot . "/sanger-plugins/vega/conf/ini-files/Conversion.ini";
   $conffile = abs_path($conffile);
   if (-e $conffile) {
-    open(CONF, $conffile) or throw( 
+    open(my $fh, '<', $conffile) or throw( 
       "Unable to open configuration file $conffile for reading: $!");
     my $serverroot = $self->serverroot;
-    while (<CONF>) {
+    while (<$fh>) {
       chomp;
 
       # remove comments
@@ -155,6 +155,7 @@ sub parse_common_options {
       }
       $self->param($name, $val);
     }
+    close $fh;
     $self->param('conffile', $conffile);
   }
   elsif ($conffile) {
@@ -616,12 +617,12 @@ sub list_or_file {
   if (scalar(@vals) == 1 && -e $firstval) {
     # we didn't get a list of values, but a file to read values from
     @vals = ();
-    open(IN, $firstval) or throw("Cannot open $firstval for reading: $!");
-    while(<IN>){
+    open(my $fh, '<', $firstval) or throw("Cannot open $firstval for reading: $!");
+    while(<$fh>){
       chomp;
       push(@vals, $_);
     }
-    close(IN);
+    close($fh);
     $self->param($param, @vals);
   }
   $self->comma_to_list($param);
@@ -881,11 +882,11 @@ sub dynamic_use {
   my ($self, $classname) = @_;
   my ($parent_namespace, $module) = $classname =~/^(.*::)(.*)$/ ? ($1,$2) : ('::', $classname);
 
-  no strict 'refs';
+  no strict 'refs'; ## no critic
   # return if module has already been imported
   return 1 if $parent_namespace->{$module.'::'} && %{ $parent_namespace->{$module.'::'}||{} };
 
-  eval "require $classname";
+  eval "require $classname"; ## no critic
   throw("Failed to require $classname: $@") if ($@);
   $classname->import();
 
@@ -1278,7 +1279,7 @@ sub log {
 
 sub lock_log {
   my ($self) = @_;
-  
+  ## no critic
   my $fh = $self->{'_log_filehandle'};
   return if -t $fh or -p $fh; # Shouldn't lock such things
   flock($self->{'_log_filehandle'},LOCK_EX) || return 0;
@@ -1294,7 +1295,7 @@ sub lock_log {
 
 sub unlock_log {
   my ($self) = @_;
-
+  ## no critic
   my $fh = $self->{'_log_filehandle'};
   return if -t $fh or -p $fh; # We don't lock such things
   # flush is implicit in flock
diff --git a/modules/Bio/EnsEMBL/Utils/Converter.pm b/modules/Bio/EnsEMBL/Utils/Converter.pm
index 652e01e84c..a939e6041c 100644
--- a/modules/Bio/EnsEMBL/Utils/Converter.pm
+++ b/modules/Bio/EnsEMBL/Utils/Converter.pm
@@ -171,7 +171,7 @@ sub _convert_single{
 
 foreach my $field (qw(in out)){
     my $slot=__PACKAGE__ ."::$field";
-    no strict 'refs';
+    no strict 'refs'; ## no critic
     *$field=sub{
         my $self=shift;
         $self->{$slot}=shift if @_;
diff --git a/modules/Bio/EnsEMBL/Utils/Converter/ens_bio_featurePair.pm b/modules/Bio/EnsEMBL/Utils/Converter/ens_bio_featurePair.pm
index 21ab6e6281..2d0d8b16d7 100644
--- a/modules/Bio/EnsEMBL/Utils/Converter/ens_bio_featurePair.pm
+++ b/modules/Bio/EnsEMBL/Utils/Converter/ens_bio_featurePair.pm
@@ -83,7 +83,7 @@ sub _convert_single_repeatFeature {
     );
     
     my $output_module = $self->out;
-    require "$output_module";
+    eval "require $output_module"; ## no critic
     return new Bio::SeqFeature::FeaturePair(
         -feature1 => $feature1,
         -feature2 => $feature2
diff --git a/modules/Bio/EnsEMBL/Utils/Exception.pm b/modules/Bio/EnsEMBL/Utils/Exception.pm
index 077dee25ff..f8b06dc516 100644
--- a/modules/Bio/EnsEMBL/Utils/Exception.pm
+++ b/modules/Bio/EnsEMBL/Utils/Exception.pm
@@ -519,6 +519,7 @@ sub deprecate {
 
 =cut
 
+## no critic
 sub try (&$) {
   my ($try, $catch) = @_;
   eval { &$try };
@@ -529,6 +530,7 @@ sub try (&$) {
   }
 }
 
+## no critic
 sub catch (&) {
   shift;
 }
diff --git a/modules/Bio/EnsEMBL/Utils/Proxy.pm b/modules/Bio/EnsEMBL/Utils/Proxy.pm
index 559782893b..ef1cc49e19 100644
--- a/modules/Bio/EnsEMBL/Utils/Proxy.pm
+++ b/modules/Bio/EnsEMBL/Utils/Proxy.pm
@@ -58,6 +58,9 @@ most classes.
 
 package Bio::EnsEMBL::Utils::Proxy;
 
+use strict;
+use warnings;
+
 use Bio::EnsEMBL::Utils::Exception qw/throw/;
 
 use vars '$AUTOLOAD';
@@ -179,7 +182,10 @@ sub AUTOLOAD {
     my $type = ref $self ? 'object' : 'class';
     throw qq{Can't locate $type method "$method_name" via package "$package_name". No subroutine was generated};
   }
-  *$AUTOLOAD = $sub;
+  {
+    no strict 'refs'; ## no critic ProhibitNoStrict
+    *{$AUTOLOAD} = $sub;
+  }
   goto &$sub;
 }
 
diff --git a/modules/Bio/EnsEMBL/Utils/SchemaConversion.pm b/modules/Bio/EnsEMBL/Utils/SchemaConversion.pm
index bce988747a..ba0224ee0e 100644
--- a/modules/Bio/EnsEMBL/Utils/SchemaConversion.pm
+++ b/modules/Bio/EnsEMBL/Utils/SchemaConversion.pm
@@ -48,10 +48,10 @@ Bio::EnsEMBL::Utils::ConversionSupport
 
 package Bio::EnsEMBL::Utils::SchemaConversion;
 
-use  Bio::EnsEMBL::Utils::ConversionSupport;
 use strict;
 use warnings;
 
+use Bio::EnsEMBL::Utils::ConversionSupport;
 use Data::Dumper;
 
 =head2 new
@@ -184,7 +184,7 @@ sub choose_conversion_type {
     $species = $self->species_alias($self->conv_support->param('source_db'));
     if ($self->conv_support->param('do_vega_sc')) {
         $species = "vega::".$species;
-        eval "require SeqStoreConverter::$species";
+        eval "require SeqStoreConverter::$species"; ## no critic
         if($@) {
             warn("Could not require conversion module SeqStoreConverter::$species\ for vega conversion\n" .
                     "Using SeqStoreConverter::BasicConverter instead:\n$@");
@@ -196,7 +196,7 @@ sub choose_conversion_type {
         }
     }
     else {
-        eval "require SeqStoreConverter::$species";
+        eval "require SeqStoreConverter::$species"; ## no critic
         if($@) {
             warn("Could not require conversion module SeqStoreConverter::$species for Ensembl conversion\n" .
                     "Using SeqStoreConverter::BasicConverter instead:\n$@");
diff --git a/modules/Bio/EnsEMBL/Utils/ScriptUtils.pm b/modules/Bio/EnsEMBL/Utils/ScriptUtils.pm
index 8a63aa28c6..4fcd444380 100644
--- a/modules/Bio/EnsEMBL/Utils/ScriptUtils.pm
+++ b/modules/Bio/EnsEMBL/Utils/ScriptUtils.pm
@@ -243,12 +243,12 @@ sub inject {
   my $classname = shift;
   my ($parent_namespace, $module) = $classname =~/^(.*::)(.*)$/ ?
                                       ($1,$2) : ('::', $classname);
-  no strict 'refs';
+  no strict 'refs'; ## no critic
 
   # return if module has already been imported
   return 1 if $parent_namespace->{$module.'::'};
   
-  eval "require $classname";
+  eval "require $classname"; ## no critic
   die("Failed to require $classname: $@") if ($@);
 
   $classname->import();
diff --git a/modules/t/externalFeatureAdaptor.t b/modules/t/externalFeatureAdaptor.t
index 09dbe45d58..0cb5d4ea5c 100644
--- a/modules/t/externalFeatureAdaptor.t
+++ b/modules/t/externalFeatureAdaptor.t
@@ -1,4 +1,7 @@
+## no critic (RequireFilenameMatchesPackage)
+
 use strict;
+use warnings;
 
 our $verbose = 0;
 
@@ -75,7 +78,7 @@ package ExternalFF2;
 
 
 
-package Test;
+package main;
 
 use Bio::EnsEMBL::Test::TestUtils;
 
diff --git a/modules/t/fullIdCaching.t b/modules/t/fullIdCaching.t
index 262a72def5..764b7819b0 100644
--- a/modules/t/fullIdCaching.t
+++ b/modules/t/fullIdCaching.t
@@ -18,7 +18,7 @@ my $gene_ids = [18256, 18257, 18258];
 my $genes = $gene_adaptor->fetch_all_by_dbID_list($gene_ids);
 
 sub BEGIN {
-  no strict 'refs';
+  no strict 'refs'; ## no critic
   *Bio::EnsEMBL::DBSQL::GeneAdaptor::_build_id_cache = sub {
     my ($self) = @_;
     return Bio::EnsEMBL::DBSQL::Support::FullIdCache->new($self);
diff --git a/modules/t/gffSerialiser.t b/modules/t/gffSerialiser.t
index 22369d157b..ad888cfcc3 100644
--- a/modules/t/gffSerialiser.t
+++ b/modules/t/gffSerialiser.t
@@ -1,4 +1,7 @@
+## no critic (RequireFilenameMatchesPackage)
 package Test::SO::Term;
+use strict;
+use warnings;
 
 sub new {
   my ($class) = @_;
diff --git a/modules/t/housekeeping_perlCritic.t b/modules/t/housekeeping_perlCritic.t
new file mode 100644
index 0000000000..715e3ec5f2
--- /dev/null
+++ b/modules/t/housekeeping_perlCritic.t
@@ -0,0 +1,43 @@
+use strict;
+use warnings;
+
+use Cwd;
+use File::Spec;
+use File::Basename qw/dirname/;
+use Test::More;
+
+if ( not $ENV{TEST_AUTHOR} ) {
+  my $msg = 'Author test. Set $ENV{TEST_AUTHOR} to a true value to run.';
+  plan( skip_all => $msg );
+}
+
+eval {
+  require Test::Perl::Critic;
+  require Perl::Critic::Utils;
+};
+if($@) {
+  plan( skip_all => 'Test::Perl::Critic required.' );
+  note $@;
+}
+
+#chdir into the file's target & request cwd() which should be fully resolved now.
+#then go back
+my $file_dir = dirname(__FILE__);
+my $original_dir = cwd();
+chdir($file_dir);
+my $cur_dir = cwd();
+chdir($original_dir);
+my $root = File::Spec->catdir($cur_dir, File::Spec->updir(),File::Spec->updir());
+
+# Configure critic
+Test::Perl::Critic->import(-profile => File::Spec->catfile($root, 'perlcriticrc'), -severity => 5, -verbose => 8);
+
+#Find all files & run
+my @perl_files = Perl::Critic::Utils::all_perl_files(
+  File::Spec->catdir($root, 'modules')
+);
+foreach my $perl (@perl_files) {
+  critic_ok($perl);
+}
+
+done_testing();
\ No newline at end of file
diff --git a/modules/t/lruIdCaching.t b/modules/t/lruIdCaching.t
index 94242b9ae6..152f46d27a 100644
--- a/modules/t/lruIdCaching.t
+++ b/modules/t/lruIdCaching.t
@@ -1,3 +1,5 @@
+## no critic (RequireFilenameMatchesPackage)
+
 package main;
 
 use strict;
@@ -18,7 +20,7 @@ my $gene_ids = [18256, 18257, 18258];
 my $genes = $gene_adaptor->fetch_all_by_dbID_list($gene_ids);
 
 sub BEGIN {
-  no strict 'refs';
+  no strict 'refs'; ##no critic
   *Bio::EnsEMBL::DBSQL::GeneAdaptor::_build_id_cache = sub {
     my ($self) = @_;
     return Bio::EnsEMBL::DBSQL::Support::LruIdCache->new($self, 3);
diff --git a/modules/t/mapper.t b/modules/t/mapper.t
index b9f6fda819..a603db4d23 100644
--- a/modules/t/mapper.t
+++ b/modules/t/mapper.t
@@ -1,8 +1,5 @@
 ## Bioperl Test Harness Script for Modules
 ##
-# CVS Version
-# $Id$
-
 
 # Before `make install' is performed this script should be runnable with
 # `make test'. After `make install' it should work as `perl test.t'
@@ -20,6 +17,8 @@
 #-----------------------------------------------------------------------
 
 use Test::More;
+use strict;
+use warnings;
 
 # This test script heavily edited by ihh@fruitfly.org
 
@@ -37,7 +36,7 @@ ok( 1 );
 use Bio::EnsEMBL::Mapper;
 
 
-$mapper = Bio::EnsEMBL::Mapper->new( "rawcontig", "virtualcontig" );
+my $mapper = Bio::EnsEMBL::Mapper->new( "rawcontig", "virtualcontig" );
 load_sgp_dump( $mapper, undef );
 
 # loading done successfully
@@ -335,10 +334,10 @@ chr1	625359	1214016	1216330	1	2315	1
 	@sgp_dump = reverse (@sgp_dump) if defined $reverse;   # test the auto-sorting feature
 
 	my $first = 1;
-	for $line ( @sgp_dump ) {
+	for my $local_line ( @sgp_dump ) {
 	  if( $first ) { $first = 0; next; }
 	  my ( $chr_name, $contig_id, $chr_start, 
-	       $chr_end, $contig_start, $contig_end, $contig_ori ) = split ( /\t/, $line );
+	       $chr_end, $contig_start, $contig_end, $contig_ori ) = split ( /\t/, $local_line );
 
 # new argument order:
 	  $map->add_map_coordinates( $contig_id, $contig_start, $contig_end, $contig_ori,  
diff --git a/modules/t/proteinAlignFeatureAdaptor.t b/modules/t/proteinAlignFeatureAdaptor.t
index dbb254e0c3..965d2f6427 100644
--- a/modules/t/proteinAlignFeatureAdaptor.t
+++ b/modules/t/proteinAlignFeatureAdaptor.t
@@ -1,6 +1,8 @@
-
 use Test::More;
 
+use strict;
+use warnings;
+
 use Bio::EnsEMBL::Test::MultiTestDB;
 use Bio::EnsEMBL::Test::TestUtils;
 
diff --git a/modules/t/seqEdit.t b/modules/t/seqEdit.t
index 471aa36f2c..85f2c478c9 100644
--- a/modules/t/seqEdit.t
+++ b/modules/t/seqEdit.t
@@ -1,5 +1,7 @@
 
 use Test::More;
+use strict;
+use warnings;
 
 use Bio::EnsEMBL::SeqEdit;
 use Bio::EnsEMBL::Attribute;
diff --git a/modules/t/utilsScalar.t b/modules/t/utilsScalar.t
index f651be67b9..18547fb0d1 100644
--- a/modules/t/utilsScalar.t
+++ b/modules/t/utilsScalar.t
@@ -117,7 +117,7 @@ my $scalar;
 my $other_scalar;
 open my $scalar_fh, '>', \$scalar;
 open my $other_scalar_fh, '>', \$other_scalar;
-bless($other_scalar_fh);
+bless($other_scalar_fh, __PACKAGE__);
 my $io_handle = IO::Handle->new(); # no need to close as it isn't opened yet just created
 throws_ok { assert_file_handle(undef) } qr/undefined/, 'Passing in undefined scalar means death';
 dies_ok { assert_file_handle(bless(1, 'Brian'), 'met')} 'Passing in a blessed scalar means death';
diff --git a/perlcriticrc b/perlcriticrc
new file mode 100644
index 0000000000..eea328b4fc
--- /dev/null
+++ b/perlcriticrc
@@ -0,0 +1,4 @@
+severity  = 5
+
+[Subroutines::ProhibitExplicitReturnUndef]
+severity=4
\ No newline at end of file
-- 
GitLab