* [pve-devel] [PATCH manager] vzdump mail: Refactor text part @ 2020-11-16 10:14 Dominic Jäger 2020-11-16 17:12 ` Thomas Lamprecht 0 siblings, 1 reply; 4+ messages in thread From: Dominic Jäger @ 2020-11-16 10:14 UTC (permalink / raw) To: pve-devel Less lines, less nesting, less duplicate code, less magic numbers. Signed-off-by: Dominic Jäger <d.jaeger@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. 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" }; + $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; -- 2.20.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH manager] vzdump mail: Refactor text part 2020-11-16 10:14 [pve-devel] [PATCH manager] vzdump mail: Refactor text part Dominic Jäger @ 2020-11-16 17:12 ` Thomas Lamprecht 2020-11-17 8:35 ` Dominic Jäger 0 siblings, 1 reply; 4+ messages in thread From: Thomas Lamprecht @ 2020-11-16 17:12 UTC (permalink / raw) To: Proxmox VE development discussion, Dominic Jäger 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@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; > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH manager] vzdump mail: Refactor text part 2020-11-16 17:12 ` Thomas Lamprecht @ 2020-11-17 8:35 ` Dominic Jäger 2020-11-17 8:48 ` Thomas Lamprecht 0 siblings, 1 reply; 4+ messages in thread From: Dominic Jäger @ 2020-11-17 8:35 UTC (permalink / raw) To: Thomas Lamprecht; +Cc: Proxmox VE development discussion On Mon, Nov 16, 2020 at 06:12:37PM +0100, Thomas Lamprecht wrote: > > 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. my $namelength = 20; $text .= sprintf ("%-10s %-${namelength}s %-6s %10s %10s %s\n", qw(VMID NAME STATUS TIME SIZE FILENAME)); my $render_task_plain = sub { my ($vmid, $task) = @_; my $successful = $task->{state} eq 'ok'; $text .= sprintf("%-10s %-${namelength}s %-6s %10s " . ($successful ? "%10s": "%8.2fMB")." %s\n", $task->{vmid}, $name, $task->{state}, format_time($task->{backuptime}), $size, $filename); }; foreach my $task (@$tasklist) { $text .= render_task_plain($vmid, $task); } Not sure about this, we cannot move the heading into render_task_plain => Still two format strings? So I don't really see how we would benefit from the additional sub. > we could avoid the sub and the if by using the $size_conversion directly in > the format string? I personally would prefer this idea. Then we can still decide if we prefer 1. Short, but two format strings written out my $namelength = 20; $text .= sprintf ("%-10s %-${namelength}s %-6s %10s %10s %s\n", qw(VMID NAME STATUS TIME SIZE FILENAME)); foreach my $task (@$tasklist) { my $successful = $task->{state} eq 'ok'; $text .= sprintf("%-10s %-${namelength}s %-6s %10s " . ($successful ? "%10s": "%8.2fMB")." %s\n", $task->{vmid}, $name, $task->{state}, format_time($task->{backuptime}), $size, $filename); } 2. or a longer version. We could put all the decisions into the $fmt sub (idea thanks to Hannes). Then it's a little longer, but relatively easy to read I think, and has no two written out format strings. my $namelength = 20; my $fmt = sub { my ($successful) = @_; my $fmt = "%-10s %-${namelength}s %-6s %10s "; $fmt .= $successful ? "%10s": "%8.2fMB"; $fmt .= " %s\n"; return $fmt; }; $text .= sprintf ($fmt->(1), qw(VMID NAME STATUS TIME SIZE FILENAME)); foreach my $task (@$tasklist) { 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} : '-'; $text .= sprintf($fmt->($successful), $task->{vmid}, $name, $task->{state}, format_time($task->{backuptime}), $size, $filename); } ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH manager] vzdump mail: Refactor text part 2020-11-17 8:35 ` Dominic Jäger @ 2020-11-17 8:48 ` Thomas Lamprecht 0 siblings, 0 replies; 4+ messages in thread From: Thomas Lamprecht @ 2020-11-17 8:48 UTC (permalink / raw) To: Dominic Jäger; +Cc: Proxmox VE development discussion On 17.11.20 09:35, Dominic Jäger wrote: > On Mon, Nov 16, 2020 at 06:12:37PM +0100, Thomas Lamprecht wrote: >> >> 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. > > my $namelength = 20; > $text .= sprintf ("%-10s %-${namelength}s %-6s %10s %10s %s\n", > qw(VMID NAME STATUS TIME SIZE FILENAME)); > my $render_task_plain = sub { > my ($vmid, $task) = @_; > my $successful = $task->{state} eq 'ok'; > $text .= sprintf("%-10s %-${namelength}s %-6s %10s " . > ($successful ? "%10s": "%8.2fMB")." %s\n", $task->{vmid}, > $name, $task->{state}, format_time($task->{backuptime}), $size, > $filename); > }; > foreach my $task (@$tasklist) { > $text .= render_task_plain($vmid, $task); > } > > Not sure about this, we cannot move the heading into render_task_plain => Still > two format strings? So I don't really see how we would benefit from the > additional sub. the line rendering is cleanly separated, headings are headings, those are often separated - one would need a column definition structure to solve that, e.g.: my $columns = { 'heading 1' => "%fmt1", 'heading 2' => "%fmt2", }; maybe reusing the exisitng CLI formatter module (with borders disabled) from pve-common could be used? So that we do not have two slightly over engineered ways of doing this ^^ But, I'd go for a middle ground for now (see below), as that could be another wormhole to get pulled into. :) > >> we could avoid the sub and the if by using the $size_conversion directly in >> the format string? > > I personally would prefer this idea. Then we can still decide if we prefer > 1. Short, but two format strings written out > > my $namelength = 20; > $text .= sprintf ("%-10s %-${namelength}s %-6s %10s %10s %s\n", > qw(VMID NAME STATUS TIME SIZE FILENAME)); > foreach my $task (@$tasklist) { > my $successful = $task->{state} eq 'ok'; > $text .= sprintf("%-10s %-${namelength}s %-6s %10s " . > ($successful ? "%10s": "%8.2fMB")." %s\n", $task->{vmid}, $name, > $task->{state}, format_time($task->{backuptime}), $size, $filename); If, I'd prefer some other formatting, each param on it's own line: my $size_fmt = $successful ? "%10s": "%8.2fMB"; $text .= sprintf( "%-10s %-${namelength}s %-6s %10s $size_fmt %s\n", $task->{vmid}, $name, $task->{state}, format_time($task->{backuptime}), $size, $filename, ); above format, oriented on what rustfmt normally does for such things, is IMO more readable than the other proposed variants. > } > > 2. or a longer version. We could put all the decisions into the $fmt sub (idea > thanks to Hannes). Then it's a little longer, but relatively easy to read I > think, and has no two written out format strings. > > my $namelength = 20; > my $fmt = sub { > my ($successful) = @_; > my $fmt = "%-10s %-${namelength}s %-6s %10s "; > $fmt .= $successful ? "%10s": "%8.2fMB"; > $fmt .= " %s\n"; > return $fmt; > }; > $text .= sprintf ($fmt->(1), qw(VMID NAME STATUS TIME SIZE FILENAME)); > foreach my $task (@$tasklist) { > 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} : '-'; > $text .= sprintf($fmt->($successful), $task->{vmid}, $name, > $task->{state}, format_time($task->{backuptime}), $size, $filename); > } > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-11-17 8:48 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-16 10:14 [pve-devel] [PATCH manager] vzdump mail: Refactor text part Dominic Jäger 2020-11-16 17:12 ` Thomas Lamprecht 2020-11-17 8:35 ` Dominic Jäger 2020-11-17 8:48 ` Thomas Lamprecht
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox