Skip to content

Conversation

@Shelob9
Copy link
Contributor

@Shelob9 Shelob9 commented May 28, 2018

Proposed additions to #6

Josh Pollock added 2 commits May 28, 2018 12:45
* Separates the concern of creating the schema arg out of the method `filterSchema` to new method `getAdditionalSchemaArguments`, which should mean our re-usable abstract can probably handle filterSchema most of the time.
@Shelob9 Shelob9 requested a review from hellofromtonya May 28, 2018 21:51
@Shelob9 Shelob9 mentioned this pull request May 29, 2018
if ($this->argsModify->shouldFilter($request)) {
$args = array_merge($args, $this->argsModify->getAdditionalQueryArguments());
}
return $args;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the conditional into a guard clause like this:

if (!$this->argsModify->shouldFilter($request)) {
	return $args;
}

return array_merge($args, $this->argsModify->getAdditionalQueryArguments());

Why is this better?

  1. It'll run slightly faster for merging the changes with the incoming. How?:
    • PHP does not have to do a variable assignment and memory update (i.e. updating the value of that variable in memory) on line 97.
    • PHP does not have to look up the value of the variable on line 99 before returning it.
  2. Architecturally you are:
    • communicating intent that this conditional is a stopping point (i.e. "no-go"), meaning no filtering work is going to happen .... so bail out.
    • you stop a no-go condition from flowing through your code.

*/
public function postTypePrepare($post_type)
{
add_filter("rest_{$post_type}_collection_params", [ $this, 'filterSchema'], 10, 2);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting inconsistency: Remove the extra space before the $this variable.


/**
* Initialize the search filter by binding a specific content getter implementation.
* Get or set search filter by binding a specific content getter implementation.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is a setter and not a getter. Update the DocBlock to describe what it does.

$args['post_type'] = $this->restBasesToPostTypeSlugs($request[ModifySchema::ARGNAME]);
$args = array_merge($this->getAdditionalQueryArguments(), $args);
}
return $args;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we flip this around to make the shouldFilter a guard clause, then we get:

  1. more readable code as the intent is clearly communicated, i.e. "stop her if we should not filter."
  2. faster code as we don't have to do the variable assignment or look up when doing the processing work.
  3. a better design as a "no go" is not allowed to continue flowing.
if (!$this->shouldFilter($args['post_type'])) {
    return $args;
}

$this->setRequest($request);
add_filter('posts_pre_query', [FilterWPQuery::class, 'posts_pre_query'], 10, 2);

$args['post_type'] = $this->restBasesToPostTypeSlugs($request[ModifySchema::ARGNAME]);
return array_merge($this->getAdditionalQueryArguments(), $args);

$query_params = array_merge($query_params, $this->schemaModify->getAdditionalSchemaArguments());
}

return $query_params;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the conditional into a guard clause like this:

if ($this->schemaModify->shouldFilter($post_type)) {
	return $query_params;
}

return array_merge($query_params, $this->schemaModify->getAdditionalSchemaArguments());

Why is this better?

  1. It'll run slightly faster for merging the changes with the incoming. How?:
    • PHP does not have to do a variable assignment and memory update (i.e. updating the value of that variable in memory) on line 81.
    • PHP does not have to look up the value of the variable on line 84 before returning it.
  2. Architecturally you are:
    • communicating intent that this conditional is a stopping point (i.e. "no-go"), meaning no filtering work is going to happen .... so bail out.
    • you stop a no-go condition from flowing through your code.

{
return in_array($postTypeSlug, $this->postTypesToSupport);
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I see a lot of redundant code here that code go into a Trait. Then use the trait in each of these anonymous classes.
  2. I'd move the object creation to a separate method, i.e. one method for createSchemaModify and createQueryModify. In doing so, it makes the code more readable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants