We are struggling to find a way to efficiently and cleanly implement the following common use case in our code base:
- There is an object that can be voted on
- Project
- Discussion
- Comment
- We have an array of these objects
- We would like to know if a user has voted on each of these objects
- We need to associate each of these
Voteobjects with its respectiveProject,DiscussionorComment, if it exists - Throughout our view hierarchy, we need easy access to these related
Votes in the same locations that we need access to the associated Vote object, such that the pattern for accessing them is clear, simple to repeatedly implement, and preferable to any sort of "hacky" workaround solution. (Such as making$userVotea public property of the voted object, or defining a "shortcut" relation-that-is-not-a-relation to access the vote.)
This problem also describes the userWatchlist relation on ParentClasses, and the _isFollowing property of Discussion. The problem also exists in the Circle model which keeps track of user followers.
The above points can be divided into two separate yet related problems:
- How to load these
Votes (Watchlists,FollowDiscussions) efficiently when given an array of theseProjects (Discussions,ParentClasses, etc.) and the user that we are interested in. The solution must not depend on a global state object, such ascurrentUser(), as that creates a very brittle coupling between the code loading the objects and the existence of aCWebUserglobal object derived from an HTTP request. - How to associate these loaded
Votes and their respectiveProjects such that they are easily accessible from every level of the view hierarchy. Ideally, the solution will be very DRY and maintainable, will be simple to implement repeatedly, will present a straightforward method of use within the view, and will provide an easily discernable composition of code to aid debugging and discoverability. Together, these goals will promote adoption of this pattern and discourage deviations into anti-patterns (like usingcurrentUser()in a relation).
Note: all example solutions will use the userVote on Project example, but would be applicable to the class of the problem in general.
Two very similar solutions. One implemented as a static method, the other implemented as a scoped query following Yii's CActiveRecord style. Neither of these solutions deals with the secondary problem of how to associate the loaded Vote s with their respective models. These are simply two ways of answering the first part of the problem, which is how to load the Votes efficiently and without depending on session state.
class Vote {
public static function load($uid, $voteables) {
if (!is_array($voteables)) {
$voteables = [$voteables];
}
$voteableIdsByClass = [];
foreach ($voteables as $voteable) {
$class = get_class($voteable);
$voteableIdsByClass[$class][] = $voteable->primaryKey;
}
$voteableClauses = array_walk($voteableIdsByClass, function($voteableIds, $voteableClass) {
$idValues = implode(', ', $voteableIds);
$voteableClass = app()->db->quoteValue($voteableClass);
return "(voteable_class = $voteableClass AND voteable_id IN ($idValues))";
});
$voteableClauses = implode(' OR ', $voteableClauses);
$condition = "user_id = :userId AND ($voteableClauses)";
// Here, either return the resulting Votes for use somewhere else, or
// somehow go ahead and associate each Vote with its proper object
$votes = Votes::model()->findAll($condition, [':userId' => $uid]);
}
}class Vote {
public function byUser($uid) {
$this->dbCriteria->mergeWith([
'condition' => 'user_id = :userId',
'params' => [ ':userId' => $uid ]
]);
return $this;
}
public function onObjects($objects) {
// For now, assume all objects are the same type (like Project), but this could
// be extended to handle a mixture of types, as with the static solution above
if (!is_array($objects)) {
$objects = [$objects];
}
$voteableType = get_class(current($objects));
$this->dbCriteria->mergeWith([
'condition' => 'voteable_type = :voteableType AND voteable_id IN (:voteableIds)',
'params' => [
':voteableType' => $voteableType,
':voteableIds' => $voteableIds
]
]);
return $this;
}
}Right now, this is the existing solution on Project, Discussion and Comment:
// protected/models/Project.php
class Project {
public $userVote; // Sentinel value to indicate that a Vote (or null Vote)
// has been associated with the object. Sometimes set to -1.
// Currently, Vote::loadVotes sets this to the associated user's
// Vote, if it exists
}// controllers/ProjectsController.php
$projects = Project::model()->findAllByPk([123, 456, 789]);
$votes = Vote::model()->byUser($user->primaryKey)->onObjects($projects)->findAll([
'index' => 'voteable_id'
]);
__::each($projects, function($project) use ($votes) {
$project->userVote = isset($votes[$project->primaryKey] ? $votes[$project->primaryKey] : null;
});
$this->render('_form', compact('projects', 'votes'));// protected/views/votes/_form.php
$hasUserVote = (isset($voteable->userVote));
$userVoteValue = ($hasUserVote) ? $votable->userVote->value : null;
$hasUserVoteUp = ($hasUserVote && $userVoteValue == 1);
$endpoint = ($hasUserVoteUp) ? deleteVoteUrl() : createVoteUrl();
$voteDirection = ($hasUserVoteUp) ? 0 : 1;
if ($hasUserVoteUp) $btnClasses .= ' voted';- Simple to implement, simply set userVote property on each object. The vote is now accessible everywhere the object is.
- Maintainability - Locktight coupling between the
$userVoteproperty on each of these models, theVote::loadVotefunction, and the votes/_form view partial means a change in any one of these could cause breakages to the vote functionality throughout the site. - Only allows for one user's vote - Although this serves for our current use cases, this solution only allows for one user's
Voteto be associated with each model. If we want to expand to answer the question "Which of these users have voted on these projects?", this solution will present a significant refactoring challenge. - Encourages inappropriate patterns - By accessing the vote through
$voteables->userVote, a programmer who is used to the Yii environment may mistakenly recognize userVote as a relation on each$voteabletype. Treating this as a relation has a host of performance and dependency problems. This is what has led us to define such "pseudo-relations" on some of these models, as it appears to be a logical fix for a missing$voteables->userVote. - Limited control - Because
$userVoteis apublicmember of each of the voteables, we have no control over what it might be at any given time or who accesses it, or how. This again leads to inappropriate patterns scattered throughout the codebase, as seen in the current implementation ofVote::loadVote, where$voteable->userVoteis directly set without the protection that asetUserVoteabstraction might provide.
We could modify the current $userVote to address some of the concerns with the current solution.
interfaces/Voteable.php
interface Voteable {
...
public function setUserVotes($votes);
public function getUserVote($userId);
}traits/UserVoteTrait.php
trait UserVoteTrait {
private $userVotes = [];
public function setUserVotes($votes) {
foreach ($votes as $vote) {
if (get_class($this) == $vote->voteable_type && $this->primaryKey == $vote->voteable_id) {
$this->userVotes[$vote->user_id] = $vote;
}
}
}
public function getUserVote($userId) {
return isset($this->userVotes[$userId]) ? $this->userVotes[$userId] : null;
}
}// models/Project.php
class Project extends SSModel implements Voteable {
use UserVoteTrait;
}// controllers/ProjectsController.php
$projects = Project::model()->findAllByPk([123, 456, 789]);
$votes = Vote::model()->byUser($user->primaryKey)->onObjects($projects)->findAll();
__::each($projects, function($project) use ($votes) {
$project->setUserVotes($votes);
});
$this->render('_form', compact('projects', 'votes'));// protected/views/votes/_form.php
$hasUserVote = $voteable->userVote($user->primaryKey);
$userVoteValue = ($hasUserVote) ? $votable->userVote->value : null;
$hasUserVoteUp = ($hasUserVote && $userVoteValue == 1);
$endpoint = ($hasUserVoteUp) ? deleteVoteUrl() : createVoteUrl();
$voteDirection = ($hasUserVoteUp) ? 0 : 1;
if ($hasUserVoteUp) $btnClasses .= ' voted';- Little added complexity - Simple getter/setter methods to views.
- Modular code - Adding the userVote functionality is then as simple as adding the interface to the
implementsline of the class, and fulfilling the requirements of the interface by using the trait. - Multiple users' votes - Array access allows setting multiple users' votes for a voteable.
- Large mixins are a code smell - Pulling out this functionality into a trait (essentially a mixin) could lead us down the path of adding more and more functionality to each of these traits. As it grows larger, it tends to become more tightly coupled with the using classes. We'll need to guard against this and keep these traits very small.
Alternatively, we can remove any association between the model and the vote. Arguments against this have been that $userVote is a "common" pattern. However, that pattern is only used in 5 places, each made almost 2 years ago. New code is not being written that way.
// controllers/ProjectsController.php
$projects = Project::model()->findAllByPk([123, 456, 789]);
$votes = Vote::model()->byUser($user->primaryKey)->onObjects($projects)->findAll([
'index' => 'voteable_id'
]);
$this->render('_form', compact('projects', 'votes'));// protected/views/votes/_form.php
$userVote = isset($votes[$voteable->primaryKey]) ? $votes[$voteable->primaryKey] : null;
$userVoteValue = ($hasUserVote) ? $userVote->value : null;
$hasUserVoteUp = ($userVoteValue == 1);
$endpoint = ($hasUserVoteUp) ? deleteVoteUrl() : createVoteUrl();
$voteDirection = ($hasUserVoteUp) ? 0 : 1;
if ($hasUserVoteUp) $btnClasses .= ' voted';- Little added complexity - Simple array access
- Does not encourage inappropriate patterns - Models are blissfully unaware of any state, they are perfectly represented in the DB.
- Explicit dependencies - Any views that want to show related
$voteswill error if the dependency isn't met, as opposed to silently failing because$userVotewas never preloaded onto the model.
- Small amount of code changes in view - Anywhere that uses
->$userVotewill need to be updated to use the queried array. Likewise, any templates (including deeply nested partials) that require$voteswill need to have it passed in.
- Comments
- $userVote
Vote#loadVotes
- $userVote
- Discussion
- $userVote
Vote#loadVotes
- $_isFollowing
Discussion#withFollowedByUser
- $userVote
- ParentClasses
- userWatchlist()
- Project
- $userVote
Vote#loadVotes
- $userVote
- Session
- sessionCompletion()
- Unit
- unitCompletion()
- Classes
$attendingOrTeaching- htdocs/protected/components/dashboard/CalendarWidget.php
- htdocs/protected/components/dashboard/views/_calendar.php
- ParentClasses
$next_start_tsUsers#parentClassesTeaching()
$hoursParentClasses#getClassesTaughtBySkillsetIdParentClasses#getRelatedUpcomingClasses
$num_studentsParentClasses#getClassesTaughtBySkillsetId
$attended_idParentClasses#getClassesAttendedBySkillsetId
"Arguments against this have been that $userVote is a "common" pattern. However, that pattern is only used in 5 places, each made almost 2 years ago. New code is not being written that way."
The biggest feedback remains the same as it has the last few times we've spoken about this. On pages with lists of objects where a user can take action on them, we often want to know whether an object already exists between that user and object or not. That is why these similar patterns keep cropping up. If you have a solution that directly addresses this use case and demonstrate specific code examples of how this can still be easily achieved, then we should by all means do it. Continuing to talk about removing state as the biggest reason still does not address the root cause of why these pseudo relations crop up.