[pve-devel] [PATCH manager] vzdump mail: Refactor text part

[Thomas Lamprecht]

On 16.11.20 11:14, Dominic Jäger wrote:
> Less lines, less nesting, less duplicate code, less magic numbers.
> 
> Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
> ---
> Depends on the previous patch for #3136.
> 
> Admittedly, using a sub and ternary operator might be seen as more complex than
> just the  if-statements. So I'm not mad if we leave it as it is.

we could avoid the sub and the if by using the $size_conversion directly in
the format string?

Or move out even more than just the format string generation out, so that it
becomes a simple loop calling

$text .= render_task_plain($vmid, $task);

or something similar to that.

> But comparing the (first) parameters of 3 sprintf is not that much fun either.
> 
>  PVE/VZDump.pm | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index 1096d594..aaf8a84d 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -258,24 +258,17 @@ sub sendmail {
>  
>      # text part
>      my $text = $err ? "$err\n\n" : '';
> -    $text .= sprintf ("%-10s %-20s %-6s %10s %10s  %s\n", qw(VMID NAME STATUS TIME SIZE FILENAME));
> +    my $namelength = 20;
> +    my $conversions = sub { "%-10s %-${namelength}s %-6s %10s $_[0]  %s\n" };

if, I'd call this $fmt but actually I'd rather avoid the sub completely (see above)

> +    $text .= sprintf ($conversions->("%10s"), qw(VMID NAME STATUS TIME SIZE FILENAME));
>      foreach my $task (@$tasklist) {
> -	my $vmid = $task->{vmid};
> -	if  ($task->{state} eq 'ok') {
> -
> -	    $text .= sprintf ("%-10s %-20s %-6s %10s %10s  %s\n", $vmid,
> -				substr($task->{hostname}, 0, 20),
> -				$task->{state},
> -				format_time($task->{backuptime}),
> -				format_size ($task->{size}),
> -				$task->{target});
> -	} else {
> -	    $text .= sprintf ("%-10s %-20s %-6s %10s %8.2fMB  %s\n", $vmid,
> -				substr($task->{hostname}, 0, 20),
> -				$task->{state},
> -				format_time($task->{backuptime}),
> -				0, '-');
> -	}
> +	my $name = substr($task->{hostname}, 0, $namelength);
> +	my $successful = $task->{state} eq 'ok';
> +	my $size = $successful ? format_size ($task->{size}) : 0;
> +	my $filename = $successful ? $task->{target} : '-';
> +	my $size_conversion = $successful ? "%10s": "%8.2fMB";
> +	$text .= sprintf($conversions->($size_conversion), $task->{vmid}, $name,
> +	    $task->{state}, format_time($task->{backuptime}), $size, $filename);
>      }
>  
>      my $text_log_part;
>