1
\$\begingroup\$

I have a function that has a few and/or operators in it. I'm sure there is a better way to write this but can't seem to figure it out. I thought I could use arrays but ran into an issue with having more than one search value I need to check for.

My function: If the user (level 3) has clicked a button, user_meta is updated with what the current quarter is. This user meta is going to be used to track if and in which quarter the button has been clicked for said user:

// When book now button is clicked, update the user meta with current month. 
        $month = date('n');

        $curtQuarter = 'cc_events_Q' . ceil($month / 3);

        if (!empty($_REQUEST['add_qrtly_user_meta_cc']) && $level_id == 3) {

            update_user_meta($member_id, $curtQuarter, true);
        }

Level 3 users that have clicked the button will no long have access to the content for the rest of the quarter unless they pay or until the next quarter. In the function below, if the current quarter and user_meta quarter does not match they are able to access the content. If the current quarter and user_meta quarter does match they are not able to access the content. I then added code to remove old user_meta for quarters that aren't the current one to ensure that the following year I would get the same results:

function all_access_levels($levels)
{

    global $current_user;


    $member_id = $current_user->ID;

    $month = date('n');

    $curtQuarter = 'cc_events_Q' . ceil($month / 3);

    $key = get_user_meta($member_id, 'cc_events_Q1', true);
    $key2 = get_user_meta($member_id, 'cc_events_Q2', true);
    $key3 = get_user_meta($member_id, 'cc_events_Q3', true);
    $key4 = get_user_meta($member_id, 'cc_events_Q4', true);


    if (
        $curtQuarter == 'cc_events_Q1' && empty($key) ||
        $curtQuarter == 'cc_events_Q2' && empty($key2) ||
        $curtQuarter == 'cc_events_Q3' && empty($key3) ||
        $curtQuarter == 'cc_events_Q4' && empty($key4)
    ) { // Change post ID and user ID value. Adjust this accordingly.

        $levels = '3';
        return $levels;

    } else {

        if (
            $curtQuarter == 'cc_events_Q1' && !empty($key2) ||
            $curtQuarter == 'cc_events_Q1' && !empty($key3) ||
            $curtQuarter == 'cc_events_Q1' && !empty($key4)
        ) {
            delete_user_meta($member_id, 'cc_events_Q2');
            delete_user_meta($member_id, 'cc_events_Q3');
            delete_user_meta($member_id, 'cc_events_Q4');
        }

        if (
            $curtQuarter == 'cc_events_Q2' && !empty($key) ||
            $curtQuarter == 'cc_events_Q2' && !empty($key3) ||
            $curtQuarter == 'cc_events_Q2' && !empty($key4)
        ) {
            delete_user_meta($member_id, 'cc_events_Q1');
            delete_user_meta($member_id, 'cc_events_Q3');
            delete_user_meta($member_id, 'cc_events_Q4');
        }

        if (
            $curtQuarter == 'cc_events_Q3' && !empty($key) ||
            $curtQuarter == 'cc_events_Q3' && !empty($key2) ||
            $curtQuarter == 'cc_events_Q3' && !empty($key4)
        ) {
            delete_user_meta($member_id, 'cc_events_Q1');
            delete_user_meta($member_id, 'cc_events_Q2');
            delete_user_meta($member_id, 'cc_events_Q4');
        }

        if (
            $curtQuarter == 'cc_events_Q4' && !empty($key) ||
            $curtQuarter == 'cc_events_Q4' && !empty($key2) ||
            $curtQuarter == 'cc_events_Q4' && !empty($key3)
        ) {
            delete_user_meta($member_id, 'cc_events_Q1');
            delete_user_meta($member_id, 'cc_events_Q2');
            delete_user_meta($member_id, 'cc_events_Q3');
        }
        return false;
    }
}
add_filter('cc_all_access_levels', 'all_access_levels', 10, 3);

I would really like to clean up my code to make it more efficient. Thank you in advance for your insight.

UPDATE:

The quarter is a calendar quarter e.g. Jan, Feb, Mar = Q1, Apr, May, Jun = Q2 etc. This is for a membership site. I have a member level (3) that will be able to access content for free once a quarter. If they want more access that quarter they will have to pay for additional access until the next quarter.

The code works as is but I know it's sub-par and I'm not sure of the proper way to clean it up.

\$\endgroup\$
7
  • \$\begingroup\$ Welcome to Code Review! In what sense is the word quarter used? Is it like a quarter of a game? A quarter of an education term? Something else? \$\endgroup\$ Oct 30, 2022 at 5:04
  • \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ Thank you! I didn't know this place existed till yesterday. I'm new to php so bare with me, I am eager to learn. The quarter is being used in the sense of a calendar quarter e.g. Jan, Feb, Mar = Q1, Apr, May, Jun = Q2 etc. This is for a membership site. I have a member level (3) that will be able to access content for free once a quarter. If they want more access that quarter they will have to pay for additional access until the next quarter. \$\endgroup\$ Oct 30, 2022 at 16:33
  • \$\begingroup\$ sigh Yes @KIKOSoftware this is why coders get the bad reputation of being socially awkward. I appreciate the apology though. I am new to php and like anything someone starts, it's not always perfect, which is why I'm seeking advice. The code works but I know it's sub-par and want to do it better. I did address the basic problems that you listed, (good to know that such issues can have the probability of eliciting that kind of response), though $levels is used in the second if statement. \$\endgroup\$ Oct 30, 2022 at 16:44
  • \$\begingroup\$ @KIKOSoftware I appreciate the apology. You know It's like a obese guy at the gym having bad form or using a machine wrong. He's putting in the effort that he knows how to put in, a "hey let me show you how you can do this better" would go much further than "are you even trying?" That said I just had an aha moment with function arguments and when I need to call the database so thank you for that! I have corrected those two issues. I always thought I had to use the database query regardless of what other global items I had set. \$\endgroup\$ Oct 30, 2022 at 17:24
  • \$\begingroup\$ Yes, it's is starting to look better now. Let me see if I can wrestle the rest into shape (to stick with the gym analogy). \$\endgroup\$ Oct 30, 2022 at 17:46

2 Answers 2

1
\$\begingroup\$

OK, this is a simplified version of your function:

function all_access_levels($current_user)
{
    $user_id = $current_user->ID;
    $current_quarter = ceil(date('n') / 3);
    // unset meta data for all other quarters
    for ($quarter = 1; $quarter <= 4; $quarter++) {
        if ($quarter != $current_quarter) {
            delete_user_meta($user_id, 'cc_events_Q' . $quarter);
        }    
    }
    // level 3 users can access when they haven't any meta data
    if (empty(get_user_meta($user_id, 'cc_events_Q' . $current_quarter, true)) {
        return ['3'];
    }
    return false;
}
  • I didn't use a global for the current user, but a function argument. This helps to know what the dependencies of the function are.
  • I get the current quarter as number, not as a key string. This is more flexible, might I want to reuse it for another purpose in this function. Note that I actually do that.
  • The check whether the current quarter exists as meta data has been simplified.
  • A loop was used to delete meta data for other quarters.
  • I swapped the two sections of this function around. Why? Well if there is no meta data for the current quarter the meta data for other quarters would not be deleted, I think it should.
\$\endgroup\$
8
  • \$\begingroup\$ Just to be clear: I have not been able to test this code. \$\endgroup\$ Oct 30, 2022 at 18:18
  • \$\begingroup\$ I do believe this function might get into trouble because it doesn't include a year number. Suppose I an user ran this function in June 2019 and came back in April 2020? It would seem like they already ran this function in that quarter, whereas in fact they hadn't. Whether that could be a common occurrence, I don't know. \$\endgroup\$ Oct 30, 2022 at 18:21
  • \$\begingroup\$ Thank you for this! I didn't realize how many unnecessary variables I created i.e. '$month' inside of '$curtQuarter'. The for loop is another thing that I neglected to consider. I did have to change something though. I had to add back in the global '$current_user' and remove the '$current_user' argument. get_user_meta was not being read without the change. So if you update that in the answer I will up vote this! -Thank you for your help. I learned a good bit here. \$\endgroup\$ Oct 30, 2022 at 22:04
  • \$\begingroup\$ With regards to the year question, it is very possible a user could do that. That is why I wanted to delete the user_meta if the quarter has past. That way, everything starts again come the next quarter. In theory, and please let me know if I'm missing something, come July 2019 (Q3) the (Q2) user_meta would be removed so the following year in April 2020 (Q2) the content would be available. At least that's how I worked it out in my head. \$\endgroup\$ Oct 30, 2022 at 22:04
  • \$\begingroup\$ Moving a function argument to a global variable goes against what is considered good practice in PHP. There can also not be an reason that get_user_meta() would work with a global, and not with a function argument, unless you forgot to call the function with that argument? \$\endgroup\$ Oct 30, 2022 at 22:15
0
\$\begingroup\$

This answer is completely based on the code and help from -KIKO Software. I did have to add in the global '$current_user' for it to work correctly. Also, per @kikosoftware foresight, I added a means to track by year, just in case a user did not visit the site until the same quarter the following year.

function all_access_levels($current_user)
{
    global $current_user;

    $user_id = $current_user->ID;
    $current_quarter = 'Q' . ceil(date('n') / 3) . '_' . date('Y');
    $past_meta_year = get_user_meta($user_id,'cc_event_date_clicked');
    $get_meta_date = $past_meta_year[0];
    $last_four_of_past_year = substr($get_meta_date,-4);

    // unset meta data for all other quarters
    for ($quarter = 1; $quarter <= 4; $quarter++) {
        if ('Q' . $quarter . '_' . $last_four_of_past_year != $current_quarter) {
            delete_user_meta($user_id, 'cc_event_date_clicked',  'Q' . $quarter . '_' . $last_four_of_past_year);
        }
    }

    // level 3 users can access when they haven't any meta data
    if (empty(get_user_meta($user_id, 'cc_event_date_clicked'))) {
        return ['3'];
    }
    return false;
}


add_filter('cc_all_access_levels', 'all_access_levels', 10, 3);
\$\endgroup\$
3
  • \$\begingroup\$ It looks good. The year is a nice addition. The only thing that is off is the global, you really should try to use a function argument. \$\endgroup\$ Nov 1, 2022 at 23:43
  • 1
    \$\begingroup\$ @KIKOSoftware The OP can't change the arguments passed to the function, as that is determined by WordPress (at least I assume so, as that's the only place I find an add_filter function). You would have to choose an entirely different approach if you want to make it something other than a global. E.g. you could make $current_user a property of an object and all_access_levels a method on that object or use a closure similarly. But you can't change the function parameters. \$\endgroup\$
    – mdfst13
    Nov 2, 2022 at 2:04
  • \$\begingroup\$ @mdfst13 Ah, yes, that could explain it. I was wondering what that add_filter() line was for. \$\endgroup\$ Nov 2, 2022 at 7:07

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.