Skip to content

Coding Style

Jump to bottom Edit New page
moellep edited this page Nov 14, 2014 · 8 revisions

CodingStyle - documents BOP coding style

Format

  • Indent by four.

  • Keyword statements are formatted with the keyword (if, elsif, while, continue) beginning the line:

    if ($a eq $b) {
       some op;
    }
    elsif ($a eq $c) {
       some op;
    }
    else {
       some op;
    }
  • All lines should be less than 80 characters, including pod. Any perl line that exceeds 80 char should break on logic, '(', or '->', e.g.,
    long_line_call(
        arg1, arg2);

    really_long_args(
        arg1,
        arg2,
        ...
    );

    $some_object->long_method(
        arg1,
        arg2,
    );

    $some_long_object
        ->method;
  • Operators should have spaces around them:
    $a = $b + $c;
    $a = $b . $c;

  • For clarity, the list of imports should be sorted (ctrl-x ctrl-l on the region).

  • Don't line things up vertically. It gets messed up as the program is maintained and variables are renamed.

    my($name) = 'Paul';
    my($favorite_color) = 'green';
    ### not
    #
    # my($name)           = 'Paul';
    # my($favorite_color) = 'green';

Emacs

General Tips

  • Let emacs do the work for you. We rely on cperl-mode to do the formatting for us. Sometimes it screws up, but most of the time it does a great job. Hit tab on lines that don't make sense.

  • If a file does not end in .pl or .pm, include the line: #mode:cperl at the bottom before #End:. to make it fontify correctly

  • Use the bivio's builtin functions in emacs mode. bivio functions begin with b-perl when cperl-mode is loaded.

Special method names:

_trace

_trace does an Trace::IO::Trace->register and creates the variable $_TRACE.

handle_config

handle_config does an Bivio::IO::Config->register

Naming

Name things absolutely uniquely with non-redundant, names if possible esp. if you think the name might change. This is a difficult task. The importance of good names for large system components cannot be overemphasized. For example, we use the term honorific for the title of the user. This originally was title. We changed the name with some difficulty when its fundemental semantics changed.

Files:

  • Modules should end in .pm

  • Programs should not end in .pl (see the Emacs section for how to get them to fontify)

  • Programs should start with 'b-' eg, b-realm-admin and b-petshop.

  • Use dashes '-' instead of underscores '_' in URI components or names that might become URI components, e.g. foo-bar.bview. It's easier to read when underlined. For example, we have file-delete and not file_delete.

Variables:

  • Multiword variables names should use the form:
    my($long_name) = ...;

Methods:

  • Constant subs should be uppercase and use '_' to separate words. Declare constants before methods. e.g.
     sub SOME_CONSTANT {
         return 10;
     }

Use C-c c in emacs to create constants.

  • Protected methods begin with an internal_ prefix, as in:

      internal_this_class_family()

Called to do something from subclasses to superclasses or vice-versa.

     sub internal_this_class_family {
         my(self) = @_;
         die('protected method') unless
             caller(0)->isa(__PACKAGE__);
     }

Normally, we don't "protect" a protected method with an assertion like this.

Documentation

  • Write the code so that it doesn't need to be commented.

  • Don't use method prototypes. Perl doesn't use this when dynamically dispatching an object method anyway.

Comments:

  • Comments should always be on their own line, with a space after the '#'. Eg:
    sub _foo() {
        my($num) = @_;
        # This is a fine comment
        while ($num < 10) {
            #this one has bad spacing
            ...
        } # and don't put one here
    }
  • Code which is unfinished or a "hack" which should be fixed is marked as below. This comment is left justified, so it is easily recognizable.
     #TODO: Description of problem here.
  • Code which depends on a future release of BOP should get:
    #BEBOP-11.99 remove this method
  • b_warn should be used instead of perl's warn. We catch perl's warn and output a stack trace, because it indicates a program error of some sort.

b_info should be used for normal logging.

  • IO.Trace for debugging output. Don't check in libraries that use print STDERR for debugging statements. You'll never find the print statements again.

If you define the subroutine _trace in cperl-mode, it will automatically insert the following statements:

    b_use('IO.Trace');
    our($_TRACE);

This creates private routine _trace and defines the variable $_TRACE.

Both of these are local to the module in which register is called. The value of $_TRACE and the implementation of _trace are dynamically modified with the value of the trace parameters. See the module documentation for more details.

Usage:

     _trace($bla) if $_TRACE;

In emacs this can be inserted with C-c t.

  • Configuration is handled by registering with IO.Config. If you define the method handle_config in cperl-mode, it automatically inserts the following code:
     b_use('IO.Config')->register(my $_CFG = {
        param1 => 'default1',
     });

and the method handle_config which takes specific parameters.

You can insert this with C-c s handle_config [return]

  • Exceptions are managed by Bivio.Die. Modules which would like to catch exceptions "along the way" should define a handle_die method. This method will be called if a method "up the stack" calls Bivio::Die->catch. See Agent.Dispatcher, it calls catch. See Agent.Task, it defines a handle_die.

  • Primary Ids are strings, not numbers. Use "eq" and "ne" for comparison. Do not depend on the value of the contents.

  • Use Bivio::ShellUtil as the base class for program function classes. The programs themselves should simply call:

   Bivio::Some::Program::Package->main(@ARGV);
  • Avoid the use of Bivio::Agent::Request->get_current or get_current_or_new. Instead in Widgets, use $source->get_request, which should always return the request being rendered.

You'll need to use:

   $source->get_request->get_widget_value

on widget values set from inherited attributes, e.g. form_model.

  • ListModel queries are sometimes tricky to get right, because Oracle tends towards the "constants" in the query. The first constant evaluated should be the auth_id, since this will narrow the query to just the realm's data. If there is another constant, e.g. entry_type, make sure the auth_id qualifies this table.

  • Magic variables are conveniences. Here is a list:

     $_A is 'Bivio::Type::Amount'.
     $_W is 'Bivio::UI::HTML::Widget'.
     $_IDI is the instance data index, see Bivio::UNIVERSALx

Syntax

Perl/General:

    #!/usr/bin/perl -w
    use strict;
  • To force the debugger to stop at a particular piece of code, use:
     $DB::single = 1;

It is harmless if the debugger isn't running. compile.PL has this otherwise you never get a chance to "break" the debugger.

  • 0 and '0' are false, '0.00' is not false:
     my($value) = '0.00';
     #--- case one ------
     $value ||= 'blah';
     #--- case two ------
     $value = 'blah' if $value == 0;

The two cases aren't equivalent, the first case isn't using numeric comparison so it won't be applied. The second is the correct form.

  • Avoid variable interpolation in trace and die statements. If the variables are undef, this may cause problems. Also avoid string concatenation (which defeats string checking).
           _trace('Name = ', $name) if $_TRACE;

Incorrect usage:

           _trace("Name = $name") if $_TRACE; # WRONG
           _trace('Name =' . $name) if $_TRACE; # WRONG

_trace statements never need a '\n' at the end.

  • Perl commands that are methods should be followed by a parenthesis (no space) like all methods.
           if (defined($foo)) {...
      not  if (defined $foo)  {...

Imports:

  • To avoid namespace pollution created by the @EXPORT=... statement in third party modules, packages should be imported as
     use Third::Party::Package ();

Unfortunately, this doesn't always work. perl's autoload feature is abused by many third party packages, e.g. GDBM, and you must pollute your namespace. In general, wrap third party modules when possible to avoid general name space pollution and implicit coupling. This allows us to maintain stable interfaces in the face of third party API changes.

Method Dispatch:

  • Always call public static methods using the dynamic dispatch (->) form.
     ex.
     Foo->bar(); # not Foo::bar(), or bar().

In other words, the first parameter to all public methods is either $proto or $self. There is no such restriction on private methods, since the _method() form is used--see below.

  • Constants should be called with dynamic dispatch (->), because they may be overriden,
     e.g. Bivio::Biz::FormModel->SUBMIT_OK.

Invoke private methods using the form <method-name>(...). This avoids dynamic lookup and will always use the method from the current package. Prepend a '_' to private methods and variables. ex.

     my($_GLOBAL_PRIVATE) = ...;
     sub _foo {
         my($self) = @_;
         ...
     }
     _foo($self);

cperl-mode automatically inserts methods which begin with an underscore (_) in the #=PRIVATE SUBROUTINE section of the module.

HTML:

  • If you have a series of td's, and you want one of them to expand into filling a browser of any size, with the other ones fixed, you can't use a colspan anwhere in the table. The solution is to use two tables, and not have any whitespace between table ... /table ><table > ... </table >.

BTW, <td width=xxx> is sometimes required for IE to behave properly. So <td><table width=xxx> is not the same as <td width=xxx><table width=xxx> for instance. This is an IE, not a NS bug.

Style

General:

  • Keep instance state in its own namespace within the field hash. This avoids having to know the field names of all super/sub classes when adding a new field to a class.

    ex.

     my($fields) = {};
     $self->{$_PACKAGE} = $fields;
     $fields->{'foo'} = ...;

When calling overridden method in superclass, try to use the following form:

     sub some_method {
         my($self) = shift;
         .... my stuff here...
         return $self->SUPER::my_method(@_);
     }

This allows the superclass to change its parameters and return types without changing all subclasses.

Any class which has subclasses should define the factory method new(). This allows the generated code for new() in the subclasses to work correctly.

The new() method should always create a $self->{$_PACKAGE} (fields) reference.

$self->{$_PACKAGE} (fields) should never be reassigned or created in any other method.

  • Avoid global variables in modules except at initialization. This avoids subtle bugs in supposedly stateless servers. It's ok to cache certain things from the database, e.g. see Bivio::Auth::Realm. State should be held in objects. There is global state here, but more easily managed. In the servers, Bivio::Agent::Request holds the state which is cleared after each request. Put it on the request if you need global state (context) during a operation.

  • Avoid putting logic in perl scripts. It is likely it will be reused, so create a module that contains the logic and call it from the script. For an example, see the perl/Bivio/Biz/Util directory.

  • Don't use temporary variables in a method if it can be avoided. Never use the word 'temp' in a variable name. Avoiding temporary variables makes it easier to analyze long methods. This rule can be relaxed in certain cases (like format statements). ex.

     sub add_error {
         my($self, $error) = @_;

         ### like this
         push(@{$self->[$_IDI]->{'errors'}}, $error);

         ### not this
         #my($temp_ref) = $self->[$_IDI]->{'errors'};
         # push(@$temp_ref, $error);
     }
  • In general, pass and return references to arrays and hashes. For example, Bivio::Collection::Attributes->new takes a hash_ref, which it assumes ownership of.

There are exceptions, e.g. multi-variable method results, e.g. Bivio::Type::from_literal and variable argument methods, e.g. Bivio::Biz::PropertyModel::load.

  • Import "minimally", i.e. don't use packages unless you have to. This goes against the standard of declaring before use, but this standard is only useful for unqualified imports. We always import qualified (full package name), so you know where the object is coming from.

  • Little languages pop up everywhere. perl is an interpreted language, so you don't need to invent your own languages. For examples, see Bivio::IO::Config, Bivio::Biz::Model::Preferences, and Bivio::IO::Trace. If the language is part of a critical run-time path or there are security concerns, make up your own. There is a performance penalty, of course. For example, Bivio::Agent::HTTP::Cookie doesn't use eval, but uses split and stuffs the attributes in a hash.

The advantages of using one language for everything outweighs almost any disadvantage.

  • Methods that return a value should use the 'return' keyword.
     sub foo {
         return 1;
     }

  • Encapsulate anything that can be. Don't expose unnecessary variables, methods, etc.

  • Avoid modifying global data structures, even if they aren't used again.

Die

  • We use a fail fast policy within the application. If the code cannot recover from a state problem, call die. However, exceptions should be thrown or returned when data is invalid, e.g. no rows match a user query.

  • Bivio::Die->die or die should be used to indicate a program error. Use Bivio::Die when in doubt. In general, die is just a short hand which doesn't check its arguments carefully. You can also use:

     Bivio::Die->throw('DIE', {entity => $bla, ....});

This is more cumbersome, but allows you to set an arbitrary list of attributes associated with the error. Typically, you just want to print a message with some arguments, so the above form is rarely used.

  • If you can continue, but would like to log the error use
   Bivio::IO::Alert->warn

Arguments

  • Try to always unwrap arguments as the first statement.
     sub foo {
         my($self, $count, $file_name) = @_;
         ...
     }

There are a few exceptions. In constructors, you may pass the arguments on as follows:

     sub new {
         my($self) = shift->SUPER::new(@_);
     }

If the function is overloaded or takes unlimited arguments, i.e. it checks the number and type of its parameters, it may make sense to check @_ explicitly.

If you have no need for $self or $proto, you may say

     sub my_static_func {
         my(undef, $other_arg) = @_;
     }

Another special case is the use of $fields (see discussion of instance state</a>). If you don't need $self, you may use:

     sub my_func {
         my($fields) = shift->[$_IDI]
     }

Explicit Coupling

  • Avoid implicit coupling in code. Here's a subtle example of implicit coupling that was in Model.AuthUserRealmList. It's caused by the implicit coupling natural to the use of inheritance:
    sub internal_post_load_row {
        my($self, $row) = @_;
        return 0
            unless shift->SUPER::internal_post_load_row(@_);
        my($fields) = $self->[$_IDI];
        return $fields->{is_defined_for_facade}
            && $_R->new($self->new_other('RealmOwner')->load_from_properties($row))
            ->can_user_execute_task($fields->{task}, $self->req);
    }

What's happening here is that it assumes $fields is defined. That's not necessarily the case. Perl autovivifies the $fields hash, which means that is_defined_for_facade is false even if you don't enter this routine:

    sub load_all_for_task {
        my($self, $task_id) = @_;
        $task_id = $_TI->from_any($task_id || $self->req('task_id'));
        $self->[$_IDI] = _init($self, $task_id);
        return $self->req->with_realm(
            $self->req('auth_user'),
            sub {
                return $self->load_all({
                    task_id => $task_id,
                });
            },
        );
    }

There are many other entry points that will call internal_post_load_row to be called. Therefore, you need to protect against $fields not being defined, like this in internal_post_load_row:

    my($fields) = $self->[$_IDI] || b_die('must call load_all_for_task');

This way if anybody calls internal_post_load_row via iterate or load_all (methods which are implicitly coupled via inheritance), we detect the incorrect entry point (only one is load_all_for_task) and fail fast.

You don't need to protect against every incorrect parameter to every entry point. Rather, just protect what is implicitly coupled. Someone can read load_all_for_task, and figure out what the required parameters are. That's normal programming. They can also write a test for their module so that it tests that the result of load_all_for_task is something they expect (via the chain of use). However, they can't know that load_all is the wrong thing to call by looking at the module. Even a comment "must call load_all_for_task" is insufficient, because you might not see it. The new assertion protects against the implicit coupling that is the benefit of inheritance.

  • Implicit coupling is ok in tests and test data. While it makes it hard to update tests sometimes, tests are supposed to break when you change things. It's often good to revisit a test if the test data changes.

This isn't to say that you should try to couple implicitly, but rather you can be lazy with tests, and it won't affect the reliability of the system.

Statefulness

  • Try to protect global state changes (e.g. set_realm) with b_catch calls. In the set_realm case, use with_realm except when you are sure you need set_realm.
Add a custom footer
Clone this wiki locally