https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23084 --- Comment #5 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 90823 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90823 Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} Review of attachment 90823: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=23084&attachment=90823) ----------------------------------------------------------------- I only thing that really stood out was the C4/Search.pm "Why is the \Q and \E missing?"? Explain that, I may sign this off. Everything else is a result of me seeing tcohen wanting to improve readability of code. "unless grep" is better read as "if none". ::: C4/ImportBatch.pm @@ +1114,4 @@

my $dbh = C4::Context->dbh;

my $order_by = $parameters->{order_by} || 'import_record_id'; + ( $order_by ) = grep( { $_ eq $order_by } qw( import_record_id title status overlay_status ) ) ? $order_by : 'import_record_id';

Wouldn't 'any' be semantically easier to read? It also has the added benefit of removing the whole need for () around $order_by, because $order_by is a string. $order_by = any { $_ eq $order_by } qw( import_record_id title status overlay_status ) ) ? $order_by : 'import_record_id'; Actually, the whole point of the code is even clearer with 'none': $order_by = none { $_ eq $order_by } qw( import_record_id title status overlay_status ) ) ? 'import_record_id' : $order_by; ::: C4/Installer/PerlModules.pm @@ +76,4 @@

no warnings ; # perl throws warns for invalid $VERSION numbers which some modules use my ( $self, $module ) = @_; + return -1 unless grep { $_ eq $module } keys(%$PERL_DEPS);

any is better, since it is boolean. ::: C4/Items.pm @@ +2101,4 @@

push @columns, Koha::Database->new()->schema()->resultset('Biblioitem')->result_source->columns; my @operators = qw(= != > < >= <= like); my $field = $filter->{field}; + if ( (0 < grep { $_ eq $field } @columns) or (substr($field, 0, 5) eq 'marc:') ) {

any is better, since it checking if the field is in the list of columns. @@ +2105,4 @@

my $op = $filter->{operator}; my $query = $filter->{query};

+ if (!$op or (0 == grep { $_ eq $op } @operators)) {

none would be better than a '0 ==' check. @@ +2601,5 @@

|; for my $condition ( @$conditions ) { if ( + grep { $_ eq $condition->{field} } @item_columns + or grep { $_ eq $condition->{field} } @biblioitem_columns

any's would be nicer. ::: C4/OAI/Sets.pm @@ +500,4 @@

my $value = $mapping->{'marcvalue'}; my @subfield_values = $record->subfield($field, $subfield); if ($operator eq 'notequal') { + if(0 == grep { $_ eq $value } @subfield_values) {

none. @@ +505,5 @@

last; } } else { + if(0 < grep { $_ eq $value } @subfield_values) {

any. ::: C4/Overdues.pm @@ +766,4 @@

# Put 'print' in first if exists # It avoid to sent a print notice with an email or sms template is no email or sms is defined @mtts = uniq( 'print', @mtts ) + if grep { $_ eq 'print' } @mtts;

any. ::: C4/Search.pm @@ +731,4 @@

my $data = $field->as_string( $subfield_letters, $facet->{ sep } );

+ unless ( grep { $_ eq $data } @used_datas ) {

Why is the \Q and \E missing? @@ +1490,4 @@

# this happens when selecting a subject on the opac-detail page @limits = grep {!/^$/} @limits; my $original_q = $q; # without available part + unless ( grep { $_ eq 'available' } @limits ) {

unless is harder to read construction, if none would be clearer, I think. @@ +1494,5 @@

$q =~ s| and \( \( allrecords,AlwaysMatches:'' not onloan,AlwaysMatches:''\) and \(lost,st-numeric=0\) \)||; $original_q = $q; } if ( @limits ) { + if ( grep { $_ eq 'available' } @limits ) {

any. ::: C4/Serials.pm @@ +1579,4 @@

### Would use substr and index But be careful to previous presence of () $recievedlist .= "; $serialseq" unless ( index( $recievedlist, $serialseq ) > 0 ); } + if ( grep { $_ eq $status } (MISSING_STATUSES) ) {

any. ::: C4/Stats.pm @@ +108,4 @@

} my @invalid_params = (); for my $myparam (keys %$params ) { + push @invalid_params, $myparam unless grep { $_ eq $myparam } @allowed_keys;

if none would be easier to read. ::: C4/Tags.pm @@ +192,4 @@

carp "Empty argument key to get_tag_rows: ignoring!"; next; } + unless (1 == scalar grep { $_ eq $key } @ok_fields) {

if none would be easier to read. @@ +233,4 @@

carp "Empty argument key to get_tags: ignoring!"; next; } + unless (1 == scalar grep { $_ eq $key } @ok_fields) {

if none would be easier to read. @@ +302,4 @@

carp "Empty argument key to get_approval_rows: ignoring!"; next; } + unless (1 == scalar grep { $_ eq $key } @ok_fields) {

if none would be easier to read. ::: C4/Utils/DataTables/Members.pm @@ +38,4 @@

# Do that after iTotalQuery! if ( defined $branchcode and $branchcode ) { @restricted_branchcodes = @restricted_branchcodes + ? grep { $_ eq $branchcode } @restricted_branchcodes

@restricted_branchcodes = ((scalar @restricted_branchcodes > 0) && (none { $_ eq $branchcode } @restricted_branchcodes)) ? (undef) : ($branchcode); Nested ternary's are harder to read. ::: Koha/Object.pm @@ +236,4 @@

my @columns = @{$self->_columns()};

foreach my $p ( keys %$properties ) { + unless ( grep { $_ eq $p } @columns ) {

if none. @@ +442,4 @@

my @columns = @{$self->_columns()}; # Using direct setter/getter like $item->barcode() or $item->barcode($barcode); + if ( grep { $_ eq $method } @columns ) {

if any. @@ +457,4 @@

Koha::Exceptions::Object::MethodNotCoveredByTests->throw( error => sprintf("The method %s->%s is not covered by tests!", ref($self), $method), show_trace => 1 + ) unless grep { $_ eq $method } @known_methods;

if none. ::: Koha/Objects.pm @@ +384,4 @@

$method =~ s/.*:://;

+ unless ( grep { $_ eq $method } @known_methods ) {

if none. ::: acqui/duplicate_orders.pl @@ +86,4 @@

if ( $op eq 'select' ) { @result_order_loop = map { my $order = $_; + ( grep {$_ eq $order->{ordernumber}} @ordernumbers ) ? () : $order

Too ugly to suggest something. @@ +132,4 @@

for my $field ( qw(currency budget_id order_internalnote order_vendornote sort1 sort2 )) { + next if grep { $_ eq $field } @fields_to_copy;

any. ::: admin/preferences.pl @@ +117,4 @@

{ text => $options{multiple}->{$option_value}, value => $option_value, + selected => (grep { $_ eq $option_value } @values) ? 1 : 0,

any without the ternary? ::: admin/searchengine/elasticsearch/mappings.pl @@ +124,4 @@

my $mapping_suggestible = $mapping_suggestible[$i]; my $mapping_sort = $mapping_sort[$i]; $mapping_sort = undef if $mapping_sort eq 'undef'; + $mapping_facet = ( grep { $_ eq $search_field_name } @facetable_field_names ) ? $mapping_facet : 0;

$mapping_facet = 0 if none { $_ eq $search_field_name } @facetable_field_names; @@ +200,4 @@

sort => $s->get_column('sort') // 'undef', # To avoid warnings "Use of uninitialized value in lc" suggestible => $s->get_column('suggestible'), facet => $s->get_column('facet'), + is_facetable => ( grep { $_ eq $name } @facetable_field_names ) ? 1 : 0,

any without the ternary? ::: catalogue/ISBDdetail.pl @@ +152,4 @@

my $basket = $myorder->{'basketno'}; if ((defined $myorder->{'datecancellationprinted'}) and ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){ push @deletedorders_using_biblio, $myorder; + unless (grep{ $_ eq $basket } @baskets_deletedorders){

if none. @@ +157,5 @@

} } else { push @orders_using_biblio, $myorder; + unless (grep{ $_ eq $basket } @baskets_orders){

if none, and fix the brace indentation below. ::: catalogue/MARCdetail.pl @@ +328,4 @@

my $basket = $myorder->{'basketno'}; if ((defined $myorder->{'datecancellationprinted'}) and ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){ push @deletedorders_using_biblio, $myorder; + unless (grep{ $_ eq $basket } @baskets_deletedorders){

if none @@ +333,5 @@

} } else { push @orders_using_biblio, $myorder; + unless (grep { $_ eq $basket } @baskets_orders){

if none with brace indentation fix below. ::: catalogue/detail.pl @@ +522,4 @@

my $basket = $myorder->{'basketno'}; if ((defined $myorder->{'datecancellationprinted'}) and ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){ push @deletedorders_using_biblio, $myorder; + unless (grep{ $_ eq $basket } @baskets_deletedorders){

if none @@ +527,5 @@

} } else { push @orders_using_biblio, $myorder; + unless (grep{ $_ eq $basket } @baskets_orders){

if none with brace indentation below. ::: catalogue/imageviewer.pl @@ +89,4 @@

my $basket = $myorder->{'basketno'}; if ((defined $myorder->{'datecancellationprinted'}) and ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){ push @deletedorders_using_biblio, $myorder; + unless (grep{ $_ eq $basket } @baskets_deletedorders){

if none @@ +99,1 @@

push @baskets_orders,$myorder->{'basketno'};

if none with brace indentation below. ::: catalogue/itemsearch.pl @@ +63,4 @@

push @f, $columns[$i]; push @c, 'and';

+ if ( grep { $_ eq $columns[$i] } qw( ccode homebranch holdingbranch location itype notforloan itemlost ) ) {

any would be better. ::: catalogue/labeledMARCdetail.pl @@ +135,4 @@

my $basket = $myorder->{'basketno'}; if ((defined $myorder->{'datecancellationprinted'}) and ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){ push @deletedorders_using_biblio, $myorder; + unless (grep { $_ eq $basket } @baskets_deletedorders){

if none @@ +140,5 @@

} } else { push @orders_using_biblio, $myorder; + unless (grep{ $_ eq $basket } @baskets_orders){

if none with brace indentation fix below. ::: catalogue/moredetail.pl @@ +235,4 @@

my $basket = $myorder->{'basketno'}; if ((defined $myorder->{'datecancellationprinted'}) and ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){ push @deletedorders_using_biblio, $myorder; + unless (grep{ $_ eq $basket } @baskets_deletedorders){

if none @@ +240,5 @@

} } else { push @orders_using_biblio, $myorder; + unless (grep { $_ eq $basket } @baskets_orders){

if none with brace fix indentation below. ::: circ/circulation.pl @@ +126,4 @@

$template_name = q|circ/circulation_batch_checkouts.tt|; my @batch_category_codes = split '\|', C4::Context->preference('BatchCheckoutsValidCategories'); my $categorycode = $patron->categorycode; + if ( $categorycode && grep { $_ eq $categorycode } @batch_category_codes ) {

any Though, I wonder if this $batch_allowed and $barcodes[] could be optimized a bit with the any logic. ::: clubs/templates-add-modify.pl @@ +92,4 @@

? Koha::Club::Template::Fields->find($field_id) : Koha::Club::Template::Field->new();

+ if ( grep { $_ eq $field_id } @field_delete ) {

any @@ +126,4 @@

? Koha::Club::Template::EnrollmentFields->find($field_id) : Koha::Club::Template::EnrollmentField->new();

+ if ( grep { $_ eq $field_id } @field_delete ) {

any ::: misc/cronjobs/gather_print_notices.pl @@ +91,4 @@

@all_messages = map { my $letter_code = $_->{letter_code}; ( + grep { $_ eq $letter_code } @letter_codes

ugly code map code. no patently obvious suggestions. ::: misc/cronjobs/longoverdue.pl @@ +301,4 @@

$skip_borrower_category = [ map { uc $_} @$skip_borrower_category ]; my %category_to_process; for my $cat ( @$borrower_category ) { + unless ( grep { $_ eq $cat } @available_categories ) {

if none @@ +311,4 @@

} if ( @$skip_borrower_category ) { for my $cat ( @$skip_borrower_category ) { + unless ( grep { $_ eq $cat } @available_categories ) {

if none @@ +329,4 @@

$skip_itemtype = [ map { uc $_} @$skip_itemtype ]; my %itemtype_to_process; for my $it ( @$itemtype ) { + unless ( grep { $_ eq $it } @available_itemtypes ) {

if none @@ +339,4 @@

} if ( @$skip_itemtype ) { for my $it ( @$skip_itemtype ) { + unless ( grep { $_ eq $it } @available_itemtypes ) {

if none ::: misc/migration_tools/rebuild_zebra.pl @@ +146,4 @@

}

our @tables_allowed_for_select = ( 'biblioitems', 'items', 'biblio', 'biblio_metadata' ); +unless ( grep { $_ eq $table } @tables_allowed_for_select ) {

if none @@ +476,4 @@

sub select_all_biblios { $table = 'biblioitems' + unless grep { $_ eq $table } @tables_allowed_for_select;

if none ::: misc/translator/LangInstaller.pm @@ +634,4 @@

next if $entry =~ /^\./; my $relentry = File::Spec->catfile($dir, $entry); my $abspath = File::Spec->catfile($basedir, $relentry); + if (-d $abspath and not grep { $_ eq $relentry } @blacklist) {

none ::: opac/opac-detail.pl @@ +720,4 @@

if ( C4::Context->preference('OPACAcquisitionDetails') ) { $itm->{on_order} = 1 + if grep { $_ eq $itm->{itemnumber} } @itemnumbers_on_order;

any. possible optimization? ::: opac/opac-memberentry.pl @@ +77,4 @@

my @libraries = Koha::Libraries->search; if ( my @libraries_to_display = split '\|', C4::Context->preference('PatronSelfRegistrationLibraryList') ) { + @libraries = map { my $b = $_; my $branchcode = $_->branchcode; grep { $_ eq $branchcode } @libraries_to_display ? $b : () } @libraries;

ugly code! no suggestions. ::: opac/opac-search-history.pl @@ +70,4 @@

@searches = map { $_->{type} ne $type ? $_ : () } @searches; } if ( @id ) { + @searches = map { my $search = $_; ( grep { $_ eq $search->{id} } @id ) ? () : $_ } @searches;

so many ugly map{ grep {...}}'s. ::: opac/opac-search.pl @@ +431,4 @@

@sort_by = $cgi->multi_param('sort_by'); $sort_by[0] = $default_sort_by if !$sort_by[0] && defined($default_sort_by); foreach my $sort (@sort_by) { + if ( grep { $_ eq $sort } @allowed_sortby ) {

any ::: opac/opac-shelves.pl @@ +114,3 @@

if ( $shelf ) { $op = $referer; my $sortfield = $query->param('sortfield');

if $sortfield is undef, next line will explode. // 'title'; would be good. @@ +114,4 @@

if ( $shelf ) { $op = $referer; my $sortfield = $query->param('sortfield'); + $sortfield = 'title' unless grep { $_ eq $sortfield } qw( title author copyrightdate itemcallnumber dateadded );

if none. ::: reports/borrowers_stats.pl @@ +162,4 @@

my $attribute_type = $1; return unless (grep {$attribute_type eq $_->{code}} @attribute_types); } else { + return unless (grep { $_ eq $line } @valid_names);

if none @@ +167,5 @@

if ($column =~ /^patron_attr\.(.*)/) { my $attribute_type = $1; return unless (grep {$attribute_type eq $_->{code}} @attribute_types); } else { + return unless (grep { $_ eq $column } @valid_names);

if none @@ +172,3 @@

} return if ($digits and $digits !~ /^\d+$/); + return if ($status and (grep { $_ eq $status } qw(debarred gonenoaddress lost)) == 0);

none @@ +172,4 @@

} return if ($digits and $digits !~ /^\d+$/); + return if ($status and (grep { $_ eq $status } qw(debarred gonenoaddress lost)) == 0); + return if ($activity and (grep { $_ eq $activity } qw(active nonactive)) == 0);

none ::: tools/export.pl @@ +75,4 @@

if ( $filename ) { my $mimetype = $query->uploadInfo($filename)->{'Content-Type'}; my @valid_mimetypes = qw( application/octet-stream text/csv text/plain application/vnd.ms-excel ); + unless ( grep { $_ eq $mimetype } @valid_mimetypes ) {

if none ::: tools/letter.pl @@ +263,4 @@

my $preview_is_available = 0;

if ($code) { + $preview_is_available = grep {$_ eq $code } qw( CHECKIN CHECKOUT HOLD_SLIP );

could optimize with an any. ::: tools/modborrowers.pl @@ +278,4 @@

for my $field ( qw/surname firstname branchcode categorycode city state zipcode country sort1 sort2 dateenrolled dateexpiry borrowernotes opacnote/ ) { my $value = $input->param($field); $infos->{$field} = $value if $value; + $infos->{$field} = "" if grep { $_ eq $field } @disabled;

if (any...) { $infos->{$field} = q{}; } else { $infos->{$field} = $value if $value; } -- double assign shorter vertically, but two assigns are slower than one. Plus, any is better than grep. @@ +313,4 @@

# If this borrower is not in the category of this attribute, we don't want to modify this attribute ++$i and next if $attr_type->{category_code} and $attr_type->{category_code} ne $borrower_categorycode; my $valuename = "attr" . $i . "_value"; + if ( grep { $_ eq $valuename } @disabled ) {

any ::: virtualshelves/shelves.pl @@ +102,4 @@

if ( $shelf ) { $op = $referer; my $sortfield = $query->param('sortfield'); + $sortfield = 'title' unless grep { $_ eq $sortfield } qw( title author copyrightdate itemcallnumber dateadded );

if none. also might want a "|| 'title'" added above. @@ +234,3 @@

if ( $shelf ) { if ( $shelf->can_be_viewed( $loggedinuser ) ) { my $sortfield = $query->param('sortfield') || $shelf->sortfield || 'title'; # Passed in sorting overrides default sorting

Actually, the following line is switch to title as default if the passed sortfield isn't one of the valid ones. Comment is wrong. @@ +234,4 @@

if ( $shelf ) { if ( $shelf->can_be_viewed( $loggedinuser ) ) { my $sortfield = $query->param('sortfield') || $shelf->sortfield || 'title'; # Passed in sorting overrides default sorting + $sortfield = 'title' unless grep { $_ eq $sortfield } qw( title author copyrightdate itemcallnumber dateadded );

if none. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.