[Koha-bugs] [Bug 23084] Replace grep {^$var$} with grep {$var eq $_}
bugzilla-daemon at bugs.koha-community.org
bugzilla-daemon at bugs.koha-community.org
Thu Jun 20 17:33:18 CEST 2019
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23084
--- Comment #5 from M. Tompsett <mtompset at 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.
More information about the Koha-bugs
mailing list