4
\$\begingroup\$

I've got some code that works as intended, but I have a feeling that it's messy. This code is modifying a wordpress loop and the goal is to check the roles of the current user and show posts of certain categories, based on the user roles.

My working code:

/** Check if the current user is a Student, and if they are, run the custom loop. */
$current_user = wp_get_current_user();
$roles = $current_user->roles;
if ($roles[0] == 'student'){

   /** Replace the standard loop with our custom loop */
   remove_action( 'genesis_loop', 'genesis_do_loop' );
   add_action( 'genesis_loop', 'custom_student_loop' );
 
   function custom_student_loop() {

      $grades = array(); //Set up an empty array for the grades. We're gonna add grade categories as necessary based on user roles.

      /** Check the roles of the current user, and add grade categories to the Grades array as needed */
      $user = wp_get_current_user();
      if ( in_array( 'grade_1_student', (array) $user->roles ) ) {
         $grades[] = ('grade-1');
      }
      if ( in_array( 'grade_2_student', (array) $user->roles ) ) {
         $grades[] = ('grade-2');
      }
      if ( in_array( 'grade_3_student', (array) $user->roles ) ) {
         $grades[] = ('grade-3');
      }
      if ( in_array( 'grade_4_student', (array) $user->roles ) ) {
         $grades[] = ('grade-4');
      }
      if ( in_array( 'grade_5_student', (array) $user->roles ) ) {
         $grades[] = ('grade-5');
      }
      if ( in_array( 'grade_6_student', (array) $user->roles ) ) {
         $grades[] = ('grade-6');
      }
      if ( in_array( 'grade_7_student', (array) $user->roles ) ) {
         $grades[] = ('grade-7');
      }
      if ( in_array( 'grade_8_student', (array) $user->roles ) ) {
         $grades[] = ('grade-8');
      }
      if ( in_array( 'grade_9_student', (array) $user->roles ) ) {
         $grades[] = ('grade-9');
      }

       global $paged; // current paginated page
       global $query_args; // grab the current wp_query() args
            $args = array(
                'paged'            => $paged, // respect pagination
                'tax_query' => array(
                 array(
                     'taxonomy' => 'homework-grades',
                     'field'    => 'slug',
                     'terms'    => $grades,
                 ),
              ),
            );
         
       genesis_custom_loop( wp_parse_args($query_args, $args) );  
   }

}

Like I said, I'm sure there's a cleaner/better way to do this but I have not been able to find any examples to follow, and I've searched everything I can think of that would apply to this, but I don't know where else to look. If anyone can give me suggestions or point me in the right direction, that would be very much appreciated.

\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

Optimizations

I recommend separating the logic from the functionality, as well as using a generalized filter instead of explicit conditionals.

You should avoid declaring functions, classes, and methods in conditional statements. If they are unused they are ignored. The exception would be when using if (!function_exists('foo')) { function foo() { } }, etc.

Use strict-type comparison operators to avoid false positives. Such as 'student' == 0 is true in PHP <= 7.4
Example: https://3v4l.org/qEKNV

I also believe checking $roles[0] is equal to student may lead to unexpected results, in the event that the values have been reordered. You may want to utilize a different comparison, such as in_array('student', $roles, true) to ensure the condition is met when desired.
- I excluded this optimization from the examples below in favor of using strict-type comparison operators.

The inversion of the comparison operators is called Yoda conditions which is intended to avoid accidental assignment of a variable and has become a programming standard in Wordpress and Symfony frameworks.

function custom_student_loop() {
    $user = wp_get_current_user();

    //separated logic from functionality - see below for two different methods
    $grades = getStudentGradesFromRoles((array) $user->roles);

    global $paged; // current paginated page
    global $query_args; // grab the current wp_query() args
    $args = array(
        'paged' => $paged,
        'tax_query' => array(
            array(
                'taxonomy' => 'homework-grades',
                'field'    => 'slug',
                'terms'    => $grades,
            ),
        ),
    );    
    genesis_custom_loop( wp_parse_args($query_args, $args) ); 
}

$current_user = wp_get_current_user();
$roles = $current_user->roles;

//Yoda condition and strict-type matching
if ('student' === $roles[0]) {
   /** Replace the standard loop with our custom loop */
   remove_action( 'genesis_loop', 'genesis_do_loop' );
   add_action( 'genesis_loop', 'custom_student_loop' );
}

Regular Expression Filter Method https://3v4l.org/JKpi5

This approach uses a preg_filter regular expression to determine if the string pattern matches and removes those that do not. Extracting the values within the parentheses () known as capture groups, to return the prefix text of grade and the number \d, separated by a -.

Simplified from array_filter+preg_match+str_replace - credit to mickmackusa for pointing this out.

function getStudentGradesFromRoles(array $roles) {
    return array_values(preg_filter('/^(grade)_(\d)_student$/', '$1-$2', $roles));
}

Indexed Method https://3v4l.org/IODVN

This approach uses an indexed array as a definition list, to intersect on the keys and extract the matching values.

The use of the static keyword allows PHP to retain the initial declaration within the scope of the function.

function getStudentGradesFromRoles(array $roles) {
    static $grades = [
       'grade_1_student' => 'grade-1', 
       'grade_2_student' => 'grade-2', 
       'grade_3_student' => 'grade-3', 
       'grade_4_student' => 'grade-4', 
       'grade_5_student' => 'grade-5', 
       'grade_6_student' => 'grade-6', 
       'grade_7_student' => 'grade-7', 
       'grade_8_student' => 'grade-8', 
       'grade_9_student' => 'grade-9', 
   ];
   
   return array_values(array_intersect_key($grades, array_flip($roles)));
}
\$\endgroup\$
8
  • \$\begingroup\$ Isn't $g = array_filter($roles, function($r){ return 1 === preg_match('/^grade\_\d\_student$/', $r); }); just the long way of saying preg_grep()? I don't know if regex is attractive in this case. Maybe you'd even entertain rewriting getStudentGradesFromRoles() with preg_filter()? \$\endgroup\$ Commented Feb 21, 2022 at 23:23
  • \$\begingroup\$ Yea, I don't see any reason why it can't be used. I wanted to avoid the false return value on error, since array_filter will return an empty array. \$\endgroup\$
    – Will B.
    Commented Feb 21, 2022 at 23:30
  • \$\begingroup\$ 3v4l.org/aYAHn (okay, I see the value in regex for this task now) \$\endgroup\$ Commented Feb 21, 2022 at 23:32
  • 1
    \$\begingroup\$ @mickmackusa Good idea, updated to simplify with preg_filter instead of array_filter + preg_match + str_replace. \$\endgroup\$
    – Will B.
    Commented Feb 21, 2022 at 23:35
  • 1
    \$\begingroup\$ Personally, I wouldn't use the capture group on the leading literal word because capture groups cost steps for the engine -- but no one will ever notice an impact on such a small data set. Your way is more brief though. Also, it is not necessary to fallback to an empty array. preg_filter() will return an empty array when there are no qualifying strings. ...and thanks for finally contributing to CR after 4 years of membership. \$\endgroup\$ Commented Feb 21, 2022 at 23:37
2
\$\begingroup\$

I can't speak to the WordPress specific logic of removing/replacing the function in that manner, but your code uses indentation properly (for the most part), properly named variables, and is easy to read. The only issue I see is the repetitive if statement. As soon as I see repetitive logic like this I try to find a way to replace it with another structure. This is normally a loop of some kind.

In this case, I'd use a simple array that maps the grade to the category. Then, use a loop to build up your final grade array.

$roles = (array)$user->roles should be done above the loop to avoid casting to an array on each iteration. But double-check what data type roles is because it might be defined as an array. If that's the case, you don't need to cast it to an array at all and can use $user->roles directly in the loop.

If the number of grades increases you should look into building the mapping using another, more automated method instead of hardcoding it. But, since there are only nine, this looks fine for now.

<?php

...

function custom_student_loop() 
{
    $mapping = [
        'grade_1_student' => 'grade-1',
        'grade_2_student' => 'grade-2',
        'grade_3_student' => 'grade-3',
        'grade_4_student' => 'grade-4',
        'grade_5_student' => 'grade-5',
        'grade_6_student' => 'grade-6',
        'grade_7_student' => 'grade-7',
        'grade_8_student' => 'grade-8',
        'grade_9_student' => 'grade-9',
    ];
    
    $grades = [];
    $roles = (array)$user->roles;
    foreach ($mapping as $grade => $category) {
        if (in_array($grade, $roles)) {
            $grades[] = $category;
        }
    }

    global $paged;
    ...
\$\endgroup\$
1
  • 1
    \$\begingroup\$ When implementing a lookup array, the goal should be to use isset() instead of in_array() for performance reasons. Potentially array_intersect_key(). \$\endgroup\$ Commented Feb 21, 2022 at 22:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.