Conditions can be quite complex and there can be "chained" conditions (linked with logical 'or' or 'and' operations). When coding for OTRS, you have to be aware of several situations.

Perl Best Practices says, that high precedence operators ('&&' and '||') shouldn't mixed up with low precedence operators ('and' and 'or'). To avoid confusion, we always use the high precedence operators.

if ( $Condition1 && $Condition2 ) { ... }

# instead of

if ( $Condition and $Condition2 ) { ... }
                    
                    

This means that you have to be aware of traps. Sometimes you need to use parenthesis to make clear what you want.

If you have long conditions (line is longer than 120 characters over all), you have to break it in several lines. And the start of the conditions is in a new line ( not in the line of the 'if').

if (
    $Condition1
    && $Condition2
    )
{ ... }

# instead of

if ( $Condition1
    && $Condition2
    )
{ ... }
                    
                    

Also note, that the right parenthesis is in a line on its own and the left curly bracket is also in a new line an with the same indentation as the 'if'. The operators are at the beginning of a new line! The subsequent examples show how to do it...

if (
    $XMLHash[0]->{otrs_stats}[1]{StatType}[1]{Content}
    && $XMLHash[0]->{otrs_stats}[1]{StatType}[1]{Content} eq 'static'
    )
{ ... }

if ( $TemplateName eq 'AgentTicketCustomer' ) {
    ...
}

if (
    ( $Param{Section} eq 'Xaxis' || $Param{Section} eq 'All' )
    && $StatData{StatType} eq 'dynamic'
    )
{ ... }

if (
    $Self->{TimeObject}->TimeStamp2SystemTime( String => $Cell->{TimeStop} )
    > $Self->{TimeObject}->TimeStamp2SystemTime(
        String => $ValueSeries{$Row}{$TimeStop}
    )
    || $Self->{TimeObject}->TimeStamp2SystemTime( String => $Cell->{TimeStart} )
    < $Self->{TimeObject}->TimeStamp2SystemTime(
        String => $ValueSeries{$Row}{$TimeStart}
    )
    )
{ ... }
                    
                    

Generally we use "postfix if" statements to reduce the number of levels. But we don't use it for multiline statments and is only allowed when involves return statements in functions or to end a loop or to go next iteration.

Ok:

next ITEM if !$ItemId;
                    
                    

Not ok:

return $Self->{LogObject}->Log(
    Priority => 'error',
    Message  => 'ItemID needed!',
) if !$ItemId;
                    
                    

This is less maintainable than

if( !$ItemId ) {
    $Self->{LogObject}->Log( ... );
    return;
}
                    
                    

Ok:

for my $Needed (1..10) {
    next if $Needed == 5;
    last  if $Needed == 9;
}
                    
                    

Not ok:

my $Var = 1 if $Something == 'Yes';
                    
                    

If you have several variables, you can declare them in on line if the "belong together":

my ($Minute, $Hour, $Year);

                    

and break it into seperate lines otherwise:

my $Minute;
my $ID;
                    
                    

Do not set to "undef" or '' in the declaration as this might hide mistakes in code.

my $Variable = undef;

#is the same as

my $Variable;
                    
                    

You can set a variable to '' if you want to concatenate strings:

my $SqlStatement = '';
for my $Part ( @Parts ) {
    $SqlStatement .= $Part;
}
                    
                    

Otherwise you would get an "uninitialized" warning.

To fetch the parameters passed to subroutines, OTRS normally uses the hash %Param (not %Params). This leads to more readable code as every time we use %Param in the subroutine code we know it is the parameter hash passed to the subroutine.

Just in some exceptions a regular list of parameters should be used. So we want to avoid something like this:

sub TestSub {
    my ( $Self, $Param1, $Param2 ) = @_;
}
                    
                    

We want to use this instead:

sub TestSub {
    my ( $Self, %Param ) = @_;
}
                    
                    

This has several advantages: We do not have to change the code in the subroutine when a new parameter should be passed, and calling a function with named parameters is much more readable.

If a function call requires more than one named parameter, split them into multiple lines:

    $Self->{LogObject}->Log(
        Priority => 'error',
        Message  => "Need $Needed!",
    );
                    

instead of:

    $Self->{LogObject}->Log( Priority => 'error', Message  => "Need $Needed!", );
                    

Subroutines have to have a return statement. The explicit return statement is preferred over the implicit way (result of last statement in subroutine) as this clarifies what the subroutine returns.

sub TestSub {
    ...
    return; # return undef, but not the result of the last statement
}
                    
                    

Explicit return values means that you should not have a return statement followed by a subroutine call.

return $Self->{DBObject}->Do( ... );
                    
                    

The following example is better as this says explicitly what is returned. With the example above the reader doesn't know what the return value is as he might not know what Do() returns.

return if !$Self->{DBObject}->Do( ... );
return 1;
                    
                    

If you assign the result of a subroutine to a variable, a "good" variable name indicates what was returned:

my $SuccessfulInsert = $Self->{DBObject}->Do( ... );
return $SuccessfulInsert;
                    
                    

use strict and use warnings have to be the first two "use"s in a module. This is correct:

package Kernel::System::ITSMConfigItem::History;

use strict;
use warnings;

use Kernel::System::User;
use Kernel::System::Time;
                    
                    

This is wrong:

package Kernel::System::ITSMConfigItem::History;

use Kernel::System::User;
use Kernel::System::Time;

use strict;
use warnings;
                    
                    

In OTRS many objects are available. But you should not use every object in every file to keep the frontend/backend separation.

Subroutines should always be documented. The documentation contains a general description about what the subroutine does, a sample subroutine call and what the subroutine returns. It should be in this order. A sample documentation looks like this:

=item LastTimeObjectChanged()

calculates the last time the object was changed. It returns a hash reference with
    information about the object and the time.

    my $Info = $Object->LastTimeObjectChanged(
        Param => 'Value',
    );

This returns something like:

    my $Info = {
        ConfigItemID    => 1234,
        HistoryType     => 'foo',
        LastTimeChanged => '08.10.2009',
    };

=cut
                    

You can copy and paste a Data::Dumper output for the return values.

There are two special kinds of comments.

Example 1 - comment for a longer block of code

# -----------------------------------#
# here is the start of a special area
# -----------------------------------#
                    

Example 2 - comment for customizing standard OTRS files

# --- customizing for some project