Reusable VS clean code - where's the balance?

Question

Let's say I have a data model for a blog posts and have two use-cases of that model - getting all blogposts and getting only blogposts which were written by specific author.

There are basically two ways how I can realize that.

1st model

class Articles {

    public function getPosts() {
        return $this->connection->find()
            ->sort(array('creation_time' => -1));
    }

    public function getPostsByAuthor( $authorUid ) {
        return $this->connection->find(array('author_uid' => $authorUid))
            ->sort(array('creation_time' => -1));
    }
}

1st usage (presenter/controller)

if ( $GET['author_uid'] ) {
    $posts = $articles->getPostsByAuthor($GET['author_uid']);
} else {
    $posts = $articles->getPosts();
}

2nd one

class Articles {

    public function getPosts( $authorUid = NULL ) {
        $query = array();

        if( $authorUid !== NULL ) {
            $query = array('author_uid' => $authorUid);
        }

        return $this->connection->find($query)
            ->sort(array('creation_time' => -1));
    }

}

2nd usage (presenter/controller)

$posts = $articles->getPosts( $_GET['author_uid'] );

To sum up (dis)advantages:

1) cleaner code

2) more reusable code

Which one do you think is better and why? Is there any kind of compromise between those two?

Answer 1

The trick is that you do not have to choose. One nice thing that many ORMs provide (for instance that of Django, Squeryl, SQLAlchemy and more) is composable queries. That is, you can not only query a table but query the results of another query. This is not as costly as it looks, as either the ORM or the DB query optimizer can reduce it to a combined SELECT under the hood.

So you would end up with something like (Django example)

class Article:
  @staticmethod
  def posts():
    return Article.objects.all()

  @staticmethod
  def author_posts(author):
    return Article.posts().filter(author=author)

There you have - two separate methods and no repetition. :-)

When you have to choose between clean and reusable, there is often a third option: a little more sophisticated but both clean and reusable.

(By the way, the static methods are there just to mimic your structure. Actually one would have instance methods on the Article manager class. I should also add that this is a trivial example, as the default manager - named Article.objects - already provides the all method to retrieve all results. But this is not the point - the point is that you have obtained a more specific result set by querying a larger result set.)

Answer 2

I like option #1 myself because it gives a clearer narrative to your code, and other programmers don't have to dig into your Articles class to see the what happens when $authorUid is null.

But if your going to be using that if() in many places then it makes sense to do something to wrap that logic up into a single function. Maybe make it a convention that any generically named method like getPosts() will have logic to resolve optional arguments. Maybe do something like this:

class Articles {

    public function getPosts($authorUid = NULL, $yada = NULL, $yada_yada = NULL) {
        // Keep all descision making logic in this method.
        if( $authorUid !== NULL ) {
            // call getPostsByAuthor
        } else if(...) {
            // call getAllPosts
        } // else if (...) and so on...
    }

    public function getAllPosts() {
        return $this->connection->find()
            ->sort(array('creation_time' => -1));
    }

    public function getPostsByAuthor( $authorUid ) {
        return $this->connection->find(array('author_uid' => $authorUid))
            ->sort(array('creation_time' => -1));
    }
}

Answer 3

Why not let an empty $query array indicate that there are no requirements on the posts to be retrieved? That way, the same function calls can be used for both pretty much throughout (only the outermost wrapper call need be different, if you don't want to hardcode dependencies on the array keys).

That is certainly the approach I would favor from both a reusability and cleanliness perspective, at least.

Answer 4

If it seems like an either/or case, it just means you haven't found the right solution yet.

I don't speak PHP, so my code example would likely be wrong, but to keep it abstract, the most obvious way to make your example both reusable and clean is to have your getPosts() function call your getPostsByAuthor() function with a null argument. Better yet, you simply refactor the code that does the search and use it directly from both other functions (one gets the search and executes it as it is, the other tacks on an extra condition first and then executes it). That way, you only define the code that does the work once (i.e. if it needs to be changed later on, it only needs to be changed in one place) and it's clean in terms of being able to call either method (which seemed to be your "clean" criterion, unless I'm misunderstanding you).