https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23084
--- Comment #5 from M. Tompsett <[email protected]> --- 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. _______________________________________________ Koha-bugs mailing list [email protected] http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
