A description of the "userVote" type of problem, with proposed solutions including their drawbacks and advantages

gistfile1.md

The userVote Problem

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 Vote objects with its respective Project, Discussion or Comment, 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 $userVote a 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 these Projects (Discussions, ParentClasses, etc.) and the user that we are interested in. The solution must not depend on a global state object, such as currentUser(), as that creates a very brittle coupling between the code loading the objects and the existence of a CWebUser global object derived from an HTTP request.
• How to associate these loaded Votes and their respective Projects 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 using currentUser() in a relation).

Solutions

Note: all example solutions will use the userVote on Project example, but would be applicable to the class of the problem in general.

Proposed solutions for loading

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.

Static method

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]);
    }
}

Scoped CActiveRecord query

class Vote {
    public function byUserOnObjects($uid, $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' => 'user_id = :userId AND voteable_type = :voteableType AND voteable_id IN (:voteableIds)',
            'params' => [
                ':userId' => $uid,
                ':voteableType' => $voteableType,
                ':voteableIds' => $voteableIds
            ]);
        return $this;
    }
}

Proposed solutions for object association

Current implementation

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
}

// 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';

Advantages

• Simple to implement, simply set userVote property on each object. The vote is now accessible everywhere the object is.

Disadvantages

• Maintainability - Locktight coupling between the $userVote property on each of these models, the Vote::loadVote function, and the votes/_form view partial means a change in any one of these could cause breakages to the vote functionality throughout the site.
• Code complexity - Complex logic about the existence of the vote, its value, and how to interact with it are hidden away inside the votes/_form partial. Anyone coming back to this code and trying to discover how the voting functionality works will have to search in several unlikely places to fully understand it.
• Only allows for one user's vote - Although this serves for our current use cases, this solution only allows for one user's Vote to 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 $voteable type. 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 $userVote is a public member 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 of Vote::loadVote, where $voteable->userVote is directly set without the protection that a setUserVote abstraction might provide.

Delegator/Decorator solution

Outlined in more detail here, but a summarized and expanded example given below.

// Delegator from https://gist.github.com/jridgewell/5b90660ebf601a5243b5
// ProjectWithUserVote.php
class ProjectWithUserVote extends Delegator {
    public $vote;

    public __construct($project, $vote) {
        parent::__construct($project)
    }
    
    public static function createMultiple($votes, $projects) {
        $indexedVotes = self::indexVotesByProjectId($votes);
        $created = [];
        foreach ($projects as $project) {
            $vote = isset($indexedVotes[$project->id]) ? $indexedVotes[$project->id] : null;
            $created[] = new ProjectWithUser($project, $vote);
        }
        return $created;
    }
}

class ProjectsController {
    public function actionIndex() {
        ...
        $projects = Project::model()->findAll($criteria);
        
        // Option 1, using CActiveRecord scopes
        $votes = Vote::model()->byUserOnObjects(currentUser()->id, $projects)->findAll();
        $projectsWithVotes = ProjectWithUserVote::createMultiple($votes, $projects);
        
        // Option 2, have some static loadVotes function go ahead and return delegators
        $projectsWithVotes = Vote::loadVotes($projects, currentUser()->id);
        
        $data['projects'] = $projectsWithVotes;
        ...
        $this->render('index', $data);
    }
}

Advantages

• Encapsulation - The Project, Discussion and Comment classes now are completely separated from their userVotes. Code that defines the public interface between a Project's userVote and any views that use it will now be defined in one place, rather than in both Project and the /votes/_form partial.
• Controlled access - This has the advantage of preventing code in each of the Project, Discussion, and Comment classes from having access to the userVote, reducing the chance of inadvertent changes to userVote functionality within each model class.
• Single object - Maintains the simplicity of passing down a single object that has a reference to both the $project and its associated vote. Anywhere we pass the delegator, we also have the vote. The delegator will also behave like a Project.
• Could be expanded to store votes of multiple users - For each decorated Project, we can store multiple votes associated with different users. This addresses one concern about the current implementation.

Disadvantages

• Not typesafe - Although the delegator objects will behave like the underlying Project, they will not be actual Projects for the purposes of type hinting and instanceof operations. We do not use type hinting extensively, but there are several example cases in projects_helpers.php, and instanceof is used in notification_helpers.php. Although we could refactor these out, it seems that type-hinting in general is useful (as a way of enforcing interface contracts and catching type errors earlier), and any replacement for instanceof to check for the Project or one of its delegators would be much more complex.
• Code bloat - In order to add this "WithUserVote" functionality to N models, we will need to create N delegator classes. If M models want to have their own delegator patterns, we may need to define up to M x N delegator classes. Implementing new behavior on these delegators involves updating all "XWithUserVote" classes, reducing code maintainability. Perhaps this could be mitigated by defining common "ModelWithUserVote"-like delegators, but this would exacerbate the type identity problem outlined above (Is this a ModelWithUserVote around a Project or a Discussion?).
• Additional implementation complexity - Adding voting functionality to a model now involves adding a new delegator class, replacing instances and arrays of the models with its delegators, and ensuring that no type hints or instanceof functionality is broken. How will this be made apparent? This is a lot to expect of an unfamiliar programmer trying to add vote functionality to another model. It will be more tempting to create shortcuts such as the existing $userVote anti-pattern.
• Multiple delegators is very complex - What if we want a User to be both Upvoteable and Followable? Wrapping decorators around decorators would work, but would greatly exacerbate the identity and complexity problems above. Because of PHP's single-inheritance model (and more generally, the diamond problem of multiple-inheritance languages), the recommended approach is a full Decorator pattern, which would involve defining a system of interfaces that all delegators would need to implement (ProjectWithUserVote implements PojectInterface). This would add still more complexity. Do we want to maintain all of these additional classes just to add a bit of functionality to certain models?

getUserVote method

We could modify the current $userVote to address some of the concerns with the current solution.

class SSModel extends CActiveRecord {
    private $relatedObjects = [];
    ...
    public function setUserVotes($votes) {
        $thisClass = get_class($this); // What child class of SSModel are we?
        foreach ($votes as $vote) {
            if ($vote->voteable_id == $this->id && $vote->voteable_type == $thisClass) {
                $this->relatedObjects['Vote'][$vote->user_id] = $vote;
            }
        }
    }
    
    public function setUserWatchlists($watchlists) {
        foreach ($watchlists as $watchlist) {
            if (get_class($this) == 'ParentClasses' && $watchlist->parent_class_id == $this->id) {
                $this->relatedObjects['Watchlist'][$watchlist->user_uid] = $watchlist;
            }
        }
    }
    
    public function getUserVote($userId) {
        return (isset($this->relatedObjects['Vote'][$userId])) ? $this->relatedObjects['Vote'][$userId] : null;
    }
    ...
}

Advantages

• Maintains simplicity - This solution would involve a minimal refactor. We simply mark the $userVotes array as private, define the getter and setter, and update our references to $voteable->userVote. Vote::loadVotes would need to call the setUserVotes method on each Project. Views such as views/_form would then call $project->getUserVote($userId).
• Allows for more than one user - As with the other solutions, this is generalizable for multiple users and multiple types of objects, such as Votes, Watchlists and FollowDiscussions
• Protects the userVotes - By defining a setter for the userVote and making it a private member, we now have better control over how these votes are used.

Disadvantages

• Cross-cutting - We want to implement the same setUserVotes and getUserVotes behavior on several of our models, but not all of them. If we move these implementations down into each of Project, Discussion and Comment, we are reducing maintainability, as we now need to update every implementation. We could theoretically use a common base class for all UserVoteModels, but with PHP's single-inheritance model we would then not be able to combine, say, userVote and userWatchlist into one model.
• Clutters all SSModels with unnecessary and undesirable behavior - Not all of our models will need to allow for userVotes, etc. Adding this functionality to all of our models will confuse programmers trying to familiarize themselves with the code. "ParentClasses inherits from SSModels which has a setUserVotes method. So, ParentClasses can be voted on?" How do we explicitly spell out which models should use which methods?
• Allows for invalid use - Because the setUserVotes is now defined on all SSModels, we cannot easily discern between an acceptable use of the method and an invalid one. We could validate at the loadVotes side and say "if any of the passed in models should not be using userVote, throw an exception", but this seems to be a brittle workaround for more established ways of enforcing contracts.
• Encourages pattern abuse - By defining this behavior on a parent class, we give the impression that these methods are meant to be overridden, when in fact we are simply using inheritance for code reuse. This was one of the issues that led to the abuse of $userVote in the API. When creating hydration's Project model, I wanted to include the userVote functionality. Because it was defined on the base Project class, my natural inclination was to override the behavior in the child class, even going so far as to define my own private version of $userVote and re-defining the __get magic method to access this local $userVote that would be hydrated instead. We could try to discourage this by specifying final public function getUserVote() to discourage overriding, but it seems that the tendency will still exist.

Hybrid pattern

The delegator pattern provides several benefits that would encourage good pattern use, such as encapsulation and well-defined interfaces. However, it would add much complexity. Some of this complexity is a result of the dynamic nature of delegators; a delegator is especially useful for adding functionality to an existing instance at "runtime". However, we can avoid this complexity by simply adding this functionality at "compile" time. Since we want every Project to be able to handle user votes, we should just compose the userVote functionality into the Project class.

Similarly, the simplicity of the extended $userVotes solution is diminished by the fact that it does not set a clear and easy-to-follow pattern. It cannot alert the programmer when the pattern is misused, is prone to abuse due to its accessiblity, and must either sacrifice maintainability for specificity, or confusingly compose its behavior into all of our models.

Ideally, we want a solution that minimizes the drawbacks and captures the benefits of each.

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;
}

References: http://stackoverflow.com/questions/7892749/traits-in-php-any-real-world-examples-best-practices/7893384#7893384

Advantages

• Maintainability - By using Traits for code re-use, the userVote functionality is implemented on multiple models, while being defined in a single location that is separate from the SSModel inheritance tree. Updating the implementation of setUserVotes, or how the votes are stored, is accomplished by editing a single file.
• Reduced added complexity - Follows the getUserVote solution in terms of exposing a simple getter method to views. Rather than needing to define a delegator class for each combination of Vote-like and Voteable-like objects, one interface and one trait are defined for each occurrence of this pattern. Adding the userVote functionality is then as simple as adding the interface to the implements line of the class, and fulfilling the requirements of the interface by using the trait.
• Preserves identity of models - Models will still respond correctly to type hinting and instanceof operations. This allows us to expand our use of type hinting, as we may want to define a function that operates on a Voteable and uses the user's vote, whether the voteable is actually a Discussion, Project or Comment.
• Encourages proper pattern use - By removing the getUserVote and setUserVote functions from the SSModel inheritance tree, we discourage overriding. Defining this behavior as part of an interface allows us to govern behavior and restrict usage. Calls to Vote::loadVotes on a list of sessions will now fail, because Session does not implement Voteable.

Disadvantages

• Adds coupling - The UserVoteTrait would expect some behavior from the using class. In the example, the UserVoteTrait expects a primaryKey property to be defined on the using model, in order to filter out which Votes actually belong.
• More complex than simply extending $userVote - This does necessitate several new files to our code base that would not be required in the getUserVote pattern alone.
• Makes class composition more indirect - A programmer attempting to become familiar with how the userVote behavior is implemented will now have to search through both the Project model and the UserVoteTrait. Although the function calls found within the views will be easily searchable back to their definitions in their respective traits, it will diffuse the model's definition.
• 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.