Re: Make everything an object?

[Ovid]

I find repeatedly that simple code written in a procedural style becomes too limiting for me and, as a result, I often switch to objects. However, a very important concern of objects is that they should be responsible for the things they're responsible for and the calling code should not. As a result, I noticed something rather curious about your code (seems like a great start, but you're still thinking procedurally :):

my $paper = Paper->new($cnf_file);

for my $issue ($paper->issues){
  while (my $article = $issue->article){ # an iterator
    $paper->process($article);
  }
}

$paper->post_process;

Your paper is processing articles, which are fetched from issues, which in turn are fetched from the paper, which processes the articles, which are fetched from ...

See something circular there? :) Why not have the paper manage this responsibility since it knows everything in needs to know?

my $paper = Paper->new($cnf_file);

# and in your Paper package:

sub new {
    my ( $class, $cnf_file ) = @_;
    my $self = # construct your object
    $self->_initiaze;
}

sub _initialize {
    for my $issue ($self->_issues){
        while (my $article = $issue->_article){
            $self->_process($article);
        }
    }
    $self->_post_process;
}

That pushes the responsibility of processing the issues into the Paper class. Also note that I've made most of your methods private (starting with leading underscores). You should not make them public unless you must have access to them outside of this class, though I suspect you'll want to do this with your issues and articles.

The benefit of this approach is that the logic of processing the paper is encapsulated in your paper class and if you need to rewrite it, you can try to keep the same API rather than hunt through your code and find every place which is mistakenly controlling this logic. Just tell the paper what to do and let it do it :) You also might want to read The AuthenticationFairy for more background on this.

At this point, the question is whether or not you would need to intervene in any of those steps (i.e., do you need to filter out issues or something?). If you don't, the above code is fine. Otherwise, thinking about how to do that without exposing the process is important, particularly since exposing that logic can leave the object in an inconsistent state and this should never happen.

[wfsp]

Thanks for your response.

Yes, it is a tad circular. Your comment about filtering the issues is partly responsible.

This may eventually be a cgi app and in the back of my mind is the possibility of a backlog of issues to be updated. Processing them all at once could be bit of a strain on the web server. It may be best to show the user a list of outstanding issues and prompt to select one, rinse and repeat etc.. What's important is that all of an issue's articles are processed together not that all the issues are processed together.

I'm incorporating your points about "not exposing the process" and already the logic is looking clearer and the API is shrinking - always a Good Thing ™. :-)

Again, many thanks.

[BrowserUk]

If stuffing everything inside your objects works for you, then go for it.

But there is a point of view, most clearly expounded by a certain author of several very well respected books on C++ that strongly advocates not sticking everything inside your class.

A simple quote: "If you're writing a function that can be implemented as either a member or as a non-friend non-member, you should prefer to implement it as a non-member function. That decision increases class encapsulation. When you think encapsulation, you should think non-member functions."

A "non-friend non-member function" is also called a "free function". In Perl terms: a good old-fashioned sub rather than a method. The gist of the argument is that the only measure of encapsulation is the amount of code (number of functions/methods) that needs to change if the data format changes. Any function implemented as a method will likely need to change if the data format changes.

If, on the other hand, that function can be implemented as a non-memeber, non-friend, "free" function (sub) it won't have to change if the data format changes, so doing so increases encapsulation.

Don't be blinded by OO-dogma. To paraphrase and widen a quote from the same article. OO is a means not an end. OO is only useful because it yields other things that we care about. The greatest benefit OO can yield, indeed the greatest benefit any programming technique or methodology can yield, is simplicity. Whilst it does so, it is beneficial. As soon as it starts creating complexity that can be avoided--lines of code that can be avoided--it ceases to be beneficial.

[Ovid]

The problem I generally have with OO is that most people don't know it and then they get frustrated (and rightly so). This is because it's not being taught very well and I certainly had painful experiences with it when I started. That being said, I can't say I disagree with your post, but I can't agree with it, either, because I don't understand what you're trying to say. I guess I work better with examples :)

My argument goes like this: expose nothing.

Pretty simple, eh? If the class should be responsible for something, others should not know or care how the class does it. This is even more important if exposing those details allows code outside the class to put an instance (or worse, the class itself) into an invalid state. By pulling logic outside of the class, this risk increases. For example:

my $auth = Authentication::Manager->new( { store => $store } );
if ( $auth->authenticate($username, $password) ) {
    delete_the_data_store($store);
}
else {
    throw_some_exception "invalid credentials";
}

That if/else is a bug waiting to bite. It only takes one absent-minded programmer to forget an if/else block there and then life becomes pain for everyone.

The authenticate method should probably be throwing its own exception and the calling code, if it cares, should catch it. By not doing this, it's much easier for the calling code to forget to check the return status of that method. Forcing the consumer of a module to write extra code is a recipe for bugs. Here's a somewhat better way:

my $auth = Authentication::Manager->new( { store => $store } );
$auth->authenticate($username, $password);
delete_the_data_store($store);

Because authenticate throws its own exception, you can't reach the offending code unless you authenticate (i.e., forgetting an if/else block doesn't matter) and the code is easier to read. You can't forget to check the return code (unless you explicitly have a block eval and don't check $@).

So the point I am making is not "shove everything inside of the class". It's "expose nothing until you know you need to expose it". I've found that this has worked very, very well in managing larger code bases.