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 and 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 statements and is only allowed when involves return statements in functions or to end a loop or to go next iteration.

This is correct:

next ITEM if !$ItemId;
                    
                    

This is wrong:

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

This is less maintainable than this:

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

This is correct:

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

This is wrong:

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

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

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

                    

Otherwise break it into separate lines:

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.