patternMinor
Align text (left, center, right) without additional packages
Viewed 0 times
leftwithoutaligntextcenterpackagesadditionalright
Problem
I wrote this short subroutine to left, center, right-align a multiline string.
How can I make it better (shorter, prettier, optimized)? Can you offer some suggestions?
How can I make it better (shorter, prettier, optimized)? Can you offer some suggestions?
#!/usr/bin/env perl
use strict;
# A bit of text
$_ = $max and $max = length($1) while /$re/g;
if ($align eq 'left') { s|$re|$1 . " " x ($max-length($1))|ge }
elsif ($align eq 'center') { s|$re|" " x (($max-length($1))/2) . $1|ge }
elsif ($align eq 'right') { s|$re|" " x ($max-length($1)) . $1|ge }
return $_;
}Solution
Welcome to Perl, and to Code Review :)
There are a couple of things that could be improved:
-
It's good that you
-
If you are using any recent Perl (say, anything since v5.10.1), then you can gain access to many convenient new features. You can do this by either specifying a minimum version (such as
-
Please do not use here-docs like
my ($text_to_align, $alignment) = @_;
my @lines = split /\n/, $text_to_align;
# Trim whitespace, and find the longest line.
# This is an OK use-case for the $_ variable.
my $max_length = 0;
for (@lines) {
# I prefer \A and \z over ^ and $ because they always match at
# string beginning and end, and are never influenced by /m.
s{\A\s+}{};
s{\s+\z}{};
$max_length = length if length > $max_length;
}
# Perform the alignment, depending on the alignment type.
# The alignment is performed in-place.
if ($alignment eq 'left') {
for my $line (@lines) {
my $padding = $max_length - length $line;
$line .= " " x $padding;
}
}
elsif ($alignment eq 'right') {
for my $line (@lines) {
my $padding = $max_length - length $line;
$line = (" " x $padding) . $line;
}
}
elsif ($alignment eq 'center') {
for my $line (@lines) {
my $padding = $max_length - length $line;
# we divide the $padding into two halves: $left and $right.
# But what if $padding is an odd number?
# Let's put the smaller padding $left.
my $left = int($padding / 2);
my $right = $padding - $left;
$line = (" " x $left) . $line . (" " x $right);
}
}
else {
# this happens if an invalid $aligment was passed as argument.
# Let's throw an error. We could use
# an user of our function where he made an error. Instead, we use
# the Carp module:
require Carp;
Carp::croak("Alignment must be one of left, right, center.");
}
# join the lines back together again, and return
return join "\n", @lines;
}
`
There are a couple of things that could be improved:
-
It's good that you
use strict. I recommend that you also use warnings.-
If you are using any recent Perl (say, anything since v5.10.1), then you can gain access to many convenient new features. You can do this by either specifying a minimum version (such as
use v5.10) or by explicitly asking for a feature (use feature 'say'). The say feature is exactly like print, but appends a newline automatically.-
Please do not use here-docs like
-
Avoid the $_ variable whenever possible. Instead, use a variable with a good name such as $unaligned_text. As an aside, do not declare a $_ variable with my. This is currently still allowed, but will probably be forbidden in upcoming versions of perl:
$_ is by default a global variable. However, as of perl v5.10.0, you can use a lexical version of $_ by declaring it in a file or in a block with my. […] Though this seemed like a good idea at the time it was introduced, lexical $_ actually causes more problems than it solves. If you call a function that expects to be passed information via $_, it may or may not work, depending on how the function is written, there not being any easy way to solve this. Just avoid lexical $_, unless you are feeling particularly masochistic. For this reason lexical $_ is still experimental and will produce a warning unless warnings have been disabled. As with other experimental features, the behavior of lexical $_ is subject to change without notice, including change into a fatal error.
– from perldoc -v '$_'
-
If you want to match any whitespace character in a regex, consider using the \s charclass rather than [ \t]. In this case, [ \t] may be preferable, but it will not match Unicode whitespace.
-
Unless needed, do not put regexes into separate variables. Rather, specify them in-place. If you do put a regex object into a variable (which here makes sense, as you are reusing it), use a proper name, such as $match_line.
-
Some regex modifiers change how a regex is compiled, others how a regex is matched. When creating a regex object with qr//, it is compiled. The /m flag changes the behaviour of the ^ and $ regex operators. By specifying this flag at the place of matching, you may be triggering multiple recompilations, slightly degrading performance. Instead: my $match_line = qr/.../m. The /g and /e flags change how matching and substitutions work, so they can't be used with qr//. For more information, see Regexp Quote-Like Operators in perldoc perlop.
-
Instead of matching and changing each line, consider splitting the input into an array of lines. Then loop over each line and apply the required transformation. Afterwards, join the lines together again. This happens to avoid all that hassle with regexes.
-
If an invalid alignment is passed, then you should throw an error rather than returning the input unaligned. I recommend you prefer the Carp module over the builtin die and warn.
Here is your align function, as I would write it.
sub align {my ($text_to_align, $alignment) = @_;
my @lines = split /\n/, $text_to_align;
# Trim whitespace, and find the longest line.
# This is an OK use-case for the $_ variable.
my $max_length = 0;
for (@lines) {
# I prefer \A and \z over ^ and $ because they always match at
# string beginning and end, and are never influenced by /m.
s{\A\s+}{};
s{\s+\z}{};
$max_length = length if length > $max_length;
}
# Perform the alignment, depending on the alignment type.
# The alignment is performed in-place.
if ($alignment eq 'left') {
for my $line (@lines) {
my $padding = $max_length - length $line;
$line .= " " x $padding;
}
}
elsif ($alignment eq 'right') {
for my $line (@lines) {
my $padding = $max_length - length $line;
$line = (" " x $padding) . $line;
}
}
elsif ($alignment eq 'center') {
for my $line (@lines) {
my $padding = $max_length - length $line;
# we divide the $padding into two halves: $left and $right.
# But what if $padding is an odd number?
# Let's put the smaller padding $left.
my $left = int($padding / 2);
my $right = $padding - $left;
$line = (" " x $left) . $line . (" " x $right);
}
}
else {
# this happens if an invalid $aligment was passed as argument.
# Let's throw an error. We could use
die, but that doesn't tell# an user of our function where he made an error. Instead, we use
# the Carp module:
require Carp;
Carp::croak("Alignment must be one of left, right, center.");
}
# join the lines back together again, and return
return join "\n", @lines;
}
`
Context
StackExchange Code Review Q#60866, answer score: 2
Revisions (0)
No revisions yet.