[MTOS-dev] Looking for code review - please be gentle!

[Mark Paschal]

Steve Ivy wrote:
> The code can be found at: http://redmonk.net/svn/plugins/Friends/trunk/

Looks good!

We've inadequately documented it, but the recommended way in MT 4 of 
declaring a plugin is with YAML. In your plugin's case, as you don't 
do anything else in Friends.pl but set up the plugin info and registry 
settings, it's probably not a big win.

Don't forget the "1;" at the end of your modules, such as Friends:: 
Friend.

We're far from perfect, but in MT code we try to follow the Perl Best 
Practices, using "perltidy -pbp" and perlcritic. (PBP suggests 
indenting with sets of 4 spaces exclusively.)

There are a few places you don't use modern Perl idiom, but you use it 
in other places, so I expect that's just clean-up. For instance, 
instead of:

     $link->name ? $link->name : $link->uri

one would normally write:

     $link->name || $link->uri

In Friends::App::CMS::edit_friend, the "pretty way" to pull out $obj's 
values into a hashref is $obj->column_values(), like you used in 
list_friends and save_friend. Here you'd use it like:

     $param = {
         id => $friend_id,
         %{ $obj->column_values() },
     };

On Friends::App::CMS lines 201 and 205, you loop over keys of a 
hashref, then select into it using the key. Usually one would use 
"each" for that:

     while (my ($source_uri, $source) = each %{ $sg->{nodes} }) {

On lines 190 and 242, you use "use" inside code, which is not 
recommended. If you use "require" instead, the module won't be loaded 
until that code runs (if it's not already loaded). Using "use" will 
load the "use"d module when the "use"ing module itself is loaded, 
whether or not we take that code path during the request.

In save_friend, you could factor out the copying of data from the 
query into the object using a loop. You should also check that a 
Friend was really loaded, since someone could submit an invalid one by 
editing the URL's query string. I would write that code as:

     my $obj;
     if ($friend_id) {
         $obj = $pkg->load($friend_id)
             or return $app->error('Invalid friend ID');
     }
     else {
         $obj = $pkg->new;
         $obj->init();
     }

     $obj->blog_id($blog_id);
     for my $field (qw( name uri description rel )) {
         $obj->$field($app->param($field));
     }

After saving the changes, you then simulate a list_friends response. 
Normally you'd redirect to a real list_friends request:

     $obj->save() or die "Saving failed: ", $obj->errstr;

     return $app->redirect($app->uri(
         mode => 'list_friends',
         args => {
             blog_id     => $blog_id,
             saved_added => 1,
         },
     ));

That way you don't have to duplicate that code. It also prevents the 
POST request being resubmitted if the user hits reload in the browser. 
(If you really didn't want to do a client redirect, there's also 
$app->forward(), which lets you do an internal redirect.)

You should probably have a saved-but-not-added message on the 
list_friends page, in addition to the "saved_added" message, for when 
you still return to it when an existing friend is saved after editing.

For your list_friends handler, there's an (also underdocumented) 
MT::App::listing() convenience method you could probably use.

Is the Friend object's "tags" field for folksonomic tags? For that and 
"rating", you can use the MT::Taggable and MT::Scorable mixins to 
inherit all the built-in tag and scoring behavior, if you like.

Again, this looks like good work. I hope this helps make it even better!


Mark Paschal
Software developer, Movable Type
mark at sixapart.com