Process bulk update actions

Question

It's using four sections of code block, I just want it to be reduced to one contiguous set that does either a or b with the two sections not a copy/pasted approach as done here:

  public function process_bulk_action() {

    //Detect when a bulk action is being triggered...
     if ( 'delete' === $this->current_action()){

      // In our file that handles the request, verify the nonce.
      $nonce = esc_attr( $_REQUEST['_wpnonce'] );

      if ( ! wp_verify_nonce( $nonce, 'sp_delete_customer' ) ) {
        die( 'Go get a life script kiddies' );
      }
      else {
        self::delete_customer( absint( $_GET['customer'] ) );
        wp_redirect( esc_url( add_query_arg() ) );
        exit;
      }

}
    //Detect when a Reset is being triggered...
     if ( 'reset' === $this->current_action()){

      // In our file that handles the request, verify the nonce.
      $nonce = esc_attr( $_REQUEST['_wpnonce'] );

      if ( ! wp_verify_nonce( $nonce, 'sp_reset_payouts' ) ) {
        die( 'Go get a life script kiddies' );
      }
      else {
        self::reset_record( absint ( $_GET['customer']));
        wp_redirect( esc_url( add_query_arg() ) );
        exit;
      }
}





    // // If the delete bulk action is triggered
    if ( ( isset( $_POST['action'] ) && $_POST['action'] == 'bulk-delete' )
         || ( isset( $_POST['action2'] ) && $_POST['action2'] == 'bulk-delete' )
    ) {

      $delete_ids = esc_sql( $_POST['my_CheckBoxes'] );

      // loop over the array of record IDs and delete them
      foreach ( $delete_ids as $id ) {
        self::delete_customer( $id );
      }
      wp_redirect( esc_url( add_query_arg() ) );
      exit;
    }


    //If the delete bulk action is triggered
    if ( ( isset( $_POST['action'] ) && $_POST['action'] == 'bulk-reset' )
         || ( isset( $_POST['action2'] ) && $_POST['action2'] == 'bulk-reset' )
    ) {

      $reset_ids = esc_sql( $_POST['my_CheckBoxes'] );

      // loop over the array of record IDs and delete them
      foreach ( $reset_ids as $id ) {
        self::reset_record( $id );

      }

      wp_redirect( esc_url( add_query_arg() ) );
      exit;
    }


  }

}

Answer 1

I have found myself doing code duplication from time to time, and often will find them in the code. It screams at you "fix me!".

I would recommend a book, Refactoring by Martin Fowler on Amazon. It is an excellent resource and has step to step guides to refactoring code as the example you provided here.

Using Martin Fowlers techniques, I refactored the second half of your code step by step. The first thing was to simplify the conditionals:

if ( ( isset( $_POST['action'] ) && $_POST['action'] == 'bulk-reset' )
     || ( isset( $_POST['action2'] ) && $_POST['action2'] == 'bulk-reset' )
) {

Became a query instead:

if($this->isBulkReset()) {
...

protected function isBulkReset() {
    return $this->isBulkAction('bulk-reset');
}

protected function isBulkDelete() {
    return $this->isBulkAction('bulk-delete');
}

protected function isBulkAction($action) {
    return (isset($_POST['action']) && $_POST['action'] == $action)
    || (isset($_POST['action2']) && $_POST['action2'] == $action);
}

Next I looked at some similarities in the functions, like these two:

$delete_ids = esc_sql( $_POST['my_CheckBoxes'] );
$reset_ids = esc_sql( $_POST['my_CheckBoxes'] );

I changed them in both functions:

$recordIds = $this->getRecordIds();

foreach ($recordIds as $id) {
...
protected function getRecordIds() {
    return esc_sql($_POST['my_CheckBoxes']);
}

Next I joined the two functions:

 if($this->isBulkDelete() || $this->isBulkReset()) {
        $recordIds = $this->getRecordIds();

        foreach ($recordIds as $id) {
            if($this->isBulkDelete()) {
                self::delete_customer($id);
            } else {
                self::reset_record($id);
            }
        }

        wp_redirect(esc_url(add_query_arg()));
        exit;
    }

Then I noticed that $recordIds is a totally unnecessary temporary variable, so I removed it and just did the query directly in the foreach:

if($this->isBulkDelete() || $this->isBulkReset()) {
    foreach ($this->getRecordIds() as $id) {
        if($this->isBulkDelete()) {
            self::delete_customer($id);
        } else {
            self::reset_record($id);
        }
    }
    wp_redirect(esc_url(add_query_arg()));
    exit;
}

This is how it typically progresses, small steps. But in the end, you will have more separation and cleaner code. The big secret here is that as you progress, you will see new ways in organising your code that you could not see before.

Heads up! This code is untested, simply example refactoring techniques.