Assigning several variables from request URL using regexes

Question

I refactored some of my code:

Summary: This implementation works with PSGI/Plack. It gets the URL from $env->{PATH_INFO} and assigns a different variable according to the value in the URL with regex.

This approach can redirect users to an existing URL in the router system. It also remembers the last page the user visited and detects if the URL has a string with 10 digits (token). All this is done via session file.

The previous code:

my $app = sub {

  my $env = shift;

  my $form = $env->{' Auth::Form.LoginForm'};
  my $token = $env->{'psgix.session'}{token};
  my $fromreferer = $env->{PATH_INFO};

  my $onfirstnameapp;
  if ($fromreferer eq '/'){
  $onfirstnameapp = '';    
  $env->{'psgix.session'}{equal_root} = 'equal_root';  
  }
  elsif(grep(/(nameapp\d{11})/, $fromreferer)){
  $env->{'psgix.session'}{greather_ten_digits} = 'greather_ten_digits';
  $onfirstnameapp = '/greather_ten_digits';              
  }
  elsif(grep(/(nameapp\d{10})/, $fromreferer)){
  $env->{'psgix.session'}{equal_ten} = 'equal_ten';
  $onfirstnameapp = $1 if $fromreferer =~ /.:?(nameapp\d{10})\/?/;              
  }
  else{
  $env->{'psgix.session'}{not_root_notroute} = 'not_root_notroute';      
  $onfirstnameapp = $fromreferer . '/not_root_notroute';  
  }

  my $onlineafterAppurl = $1 if $fromreferer =~ /.:?nameapp\d+(\/.*)/;
  $env->{'psgix.session'}{current_request} = $onfirstnameapp; 

  if($env->{'psgix.session'}{user_id}){

  my $get_old_token = $env->{'psgix.session'}{old_token};
  my $offtoken = $env->{'psgix.session'}{offtoken};  

  my ($code) = $router->match($env->{PATH_INFO});

  if ( $onfirstnameapp ne $token  ){
    $env->{'psgix.session'}{no_token_equal} = 'no_token_equal';  
    check_url_exist_inrouter($onlineafterAppurl);     
    # }   
    if (grep(/$onlineafterAppurl/, @resulturlcheck) or
      (grep(/(nameapp\d{10})/, $offtoken) && $onlineafterAppurl eq '') ) {

    delete $env->{'psgix.session'}{offtoken} if $onlineafterAppurl eq '';  
    $onlineafterAppurl = "/$handler" if $onlineafterAppurl eq '';  

    if( $onfirstnameapp eq $offtoken || $onfirstnameapp eq $get_old_token ) {

    $env->{'psgix.session'}{menus_router_boom_off} = 'off work';  

    return [ 
         302, 
         [ Location => URI->new('/' . $token . $onlineafterAppurl) ],
         [ '' ]
      ];

      } 

    }   

  };

  return ["404", ["Content-Type" => "text/html"], ["Not Found, go to : <a href='/'>Home</a> "  ] ] unless $code;
  return $code->($env);
  } 

 if ($fromreferer ne '/'){

    return ["401", ["Content-Type" => "text/html"], 
    [ $form ]
    ];
  } 

  return ["404", ["Content-Type" => "text/html"], [ $form ]];

}; 

The current code refactory:

my $app = sub {

  my $env = shift;

  my @all_urls_names = @{$env->{all_urls_names}};

  my $router = Routerapp->new; 

  my %set = (
      handler               => $all_urls_names[0],
      tk                    => '/' . $env->{'psgix.session'}{token},
      fromreferer           => $env->{PATH_INFO},
      token                 => $env->{'psgix.session'}{token},
      form                  => $env->{theform},
      user                  => $env->{'psgix.session'}{user_id},      
      offtoken_form         => $env->{'psgix.session'}{offtoken},
      get_old_token_form    => $env->{'psgix.session'}{old_token},      
  ); 

  my $onfirsnameapp;
  my $onafterappurl //= ''; #here I set a empty value to avoid unitialized value

  $onfirsnameapp = do {
    if    ($set{fromreferer} eq '/') {''}  
    elsif (grep(/(tapp\d{11})/, $set{fromreferer})) {'/greather_ten_digits'}    
    elsif (grep(/(tapp\d{10})/, $set{fromreferer})){ $1 if $set{fromreferer} =~ /.:?(tapp\d{10})\/?/}
    else  {$set{fromreferer} . '/not_root_notroute'}
  };  

  my %add_sesion = (
     equal_ten          => sub { $env->{'psgix.session'}{equal_ten} = 'equal_ten' if grep( /(tapp\d{10})/, $onfirsnameapp ); },
     current_request    => sub { $env->{'psgix.session'}{current_request} = $onfirsnameapp;  },
     no_tkn_equal       => sub { $env->{'psgix.session'}{no_tkn_equal} = 'no_tkn_equal'; },
     menus_router_off   => sub { $env->{'psgix.session'}{menus_router_off} = 'off work'; },      
  ); 

  $add_sesion{equal_ten}->(); 
  $add_sesion{current_request}->();

  if($set{user}){   

    my ($code) = $router->match($env->{PATH_INFO}); 
    if ( $set{onfirsnameapp} ne $set{token} ){  
        $onafterappurl = $1 if $set{fromreferer} =~ /.:?tapp\d+\/(.*)/; 

        $add_sesion{no_tkn_equal}->();

        if (grep(/$onafterappurl/, @all_urls_names) 
        or (grep(/(tapp\d{10})/, $set{offtoken_form}) 
        && $onafterappurl eq '') 
        ) { 
        delete $env->{'psgix.session'}{offtoken_form} if $onafterappurl eq '';  

        if( $onfirsnameapp eq $set{offtoken_form} || $onfirsnameapp eq $set{get_old_token_form} ) {

        $add_sesion{menus_router_off}->(); 
        return [ 
             302, 
             [ Location => URI->new('/' . $set{token} . '/' . $onafterappurl) ],
             [ '' ]
          ]; 
          }  
        }   

     }; #here finish is not equal to token

     return ["404", ["Content-Type" => "text/html"], ["Not Found, go to : <a href='/'>Home </a> "  ] ] unless $code;
     return $code->($env);
  } 

  if ($set{fromreferer} ne '/'){
     return ["401", ["Content-Type" => "text/html"], 
     [ $set{form} ]
     ];
  } 

  return ["404", ["Content-Type" => "text/html"], [ $set{form} ]];       

}

Is my refactoring good enough?

Answer 1

I'm not really comparing before and after. Without seeing the data that comes in that's a bit hard. Instead I'll focus on the new code.

In general, a lot of your variables could benefit from clearer naming. If you use more than one word for a name, which is good, use underscores _ to separate them, like $on_first_name_app. You've already done this in a couple of places. Being consistent is important to make code more readable.

There are also quite a few typos in your variable names and comments. When you fix those, make sure not to fix the typos in the URLs as those have been there before, and you have to retain existing behavior when you refactor code.

• regarding $onfirsnameapp:

  It's best practice in Perl to declare variables as late as possible. You can move the my $onfirsnameapp down to where you assign it.

  elsif (grep(/(tapp\d{11})/, $set{fromreferer})) 
  

  {'/greather_ten_digits'}

  You seem to be using grep for a single scalar regular expression match. $set{fromreferrer} is just a string, not an array, so grep doesn't really make sense. You also don't need the capture group there.

  You can shorten this to:

  elsif ( $set{fromreferer} =~ m/tapp\d{11}/ ) { '/greather_ten_digits' }
  

  Similarly in the next one you can also remove the grep and the capture group. In addition, you can use a different delimiter than // with m to get rid of the backslash \ escape.

  elsif ( $set{fromreferer} =~ m/tapp\d{10}/ ) {
       $1 if $set{fromreferer} =~ m{.:?(tapp\d{10})/?};
  }
  

  This grep thing keeps appearing throughout the code.

• my $onafterappurl //= '';

  The //= assignment is useless. A new lexical variable will always be undef unless you assign it something. If you want it to be an empty string, just say so.

  my $onafterappurl = q{};
  

  I prefer q{} over '' because it's clearer that it means an empty string.

• %add_sesion (it's session with two s).

  I don't understand why this is a dispatch table. None of this is complicated, and each of them are only used once. This is useless and the code could just be written as code at the time where it's called.

Besides these points, the code is fine. If you pick better names and add some comments as to why things are done, I would like it a lot more though.

When you write comments, try to explain the business logic decisions. The code already clearly shows that a value is assigned an empty string because it's used as a string later. But some of deeply nested decisions where the redirect is issued would really benefit from explaining why this happens.

Finally, when you refactor code, always work with unit tests to be on the safe side. Look at Plack::Test if you haven't to build some tests for this. You can also use a combination of Test::WWW::Mechanize and LWP::Protocol::PSGI if you feel more comfortable with having a $mech object in your tests.

Answer 2

First, install perltidy and run it against your code until you get used to looking at well formatted code and type it that way.

Second, names are really, really important. Names for functions and variables should be readable (use camel case or snake case to break up long names, but be consistent) and descriptive. Make sure the names are spelled correctly. Don't use names like greater_than_ten. The rule may change and you start caring about URLs with more than 12 or 8 digits. Instead name it for the meaning. Maybe something $url_with_token

You have logic and effects all mixed up and spread out. Instead of setting a variable at line 10 and then maybe resetting at line 30 so that you can use it at line 90, (line numbers are hypothetical here), either do all your input analysis in one place and set up a packet of info you need later, OR analyze your data and take action on within a few lines.

Look for repetition and factor it into subroutines.

Putting a bunch of related (or not) variables into a hash doesn't really do much except add syntax to access or mutate the values.

Let's analyze your code. It does one of five things with all inputs:

1. Runs some PSGI application code
2. Redirects to another URL
3. Returns HTTP 401 status: Unauthorized.
4. Returns HTTP 404 status: Not found with two different response bodies.

Well, 404 with the form data (whatever that is) is your fallback case. If you have a user_id then you redirect, run code if you have it, or 404. And you 401 if you your referrer data looks fishy.

So your app boils down to:

sub app {
    my $env = shift;

    return
          get_user_id($env)           ? run_app_or_redirect($env)
        : is_request_authorized($env) ? FILE_NOT_FOUND( get_form($env) )
        :                               UNAUTHORIZED( get_form($env) )
    ;
}

Huh, wow, checking auth only for not found? That's probably a bug. For now, let's forge ahead.

The FILE_NOT_FOUND and other error subs just look like this:

sub FILE_NOT_FOUND {
    my @body = @_;
    my $default_message = "Not Found, go to : <a href='/'>Home </a> ";

    return ["404", ["Content-Type" => "text/html"], [ @body ? @body : $default_message ]];       
}

The getters that fetch values from $env are like this:

sub get_user {
    my $env = shift;
    return $env->{'psgix.session'}{user_id} // '';
}

Checking request authorization is combines the lookup when you set fromreferer and the spot where you check it to maybe return an error message.

sub is_request_authorized {
    my $env = shift;
    return '/' eq $env->{PATH_INFO};
}

The tricky one is, of course, run_app_or_redirect(). I'm not going to work that all out for you. You should be able to do that yourself and probably better than I can since you know the problem domain. Just break the code into sections. First decide which thing you are going to do, and then collect the information needed to do it. You may even wish to make the main sub look like:

sub app {
    my $env = shift;

    return
          should_redirect_user($env)  ? redirect($env)
        : should_run_user_app($env)   ? run_user_app($env) || FILE_NOT_FOUND()
        : is_request_authorized($env) ? FILE_NOT_FOUND( get_form($env) )
        :                               UNAUTHORIZED( get_form($env) )
    ;
}

I hope this helps.