Re: [PATCH v2 15/22] git-remote-mediawiki: Put long code into a subroutine
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jun 8, 2013, 00:27 UTC
- Message-ID
- <CAPig+cRckkYnDxqzvM4TZ_bOwpDBkn3aXNPy-sTg551d38L7jA@mail.gmail.com>
- In-Reply-To
- <1370641344-4253-16-git-send-email-celestin.matte@ensimag.fr>
On Fri, Jun 7, 2013 at 5:42 PM, Célestin Matte <celestin.matte@ensimag.fr> wrote:
Show 32 quoted lines
> Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> ---
> contrib/mw-to-git/git-remote-mediawiki.perl | 42 +++++++++++++++++----------
> 1 file changed, 26 insertions(+), 16 deletions(-)
>
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
> index 1c34ada..f37488b 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -133,22 +133,7 @@ while (<STDIN>) {
> @cmd = split(/ /);
> if (defined($cmd[0])) {
> # Line not blank
> - if ($cmd[0] eq "capabilities") {
> - die("Too many arguments for capabilities\n") if (defined($cmd[1]));
> - mw_capabilities();
> - } elsif ($cmd[0] eq "list") {
> - die("Too many arguments for list\n") if (defined($cmd[2]));
> - mw_list($cmd[1]);
> - } elsif ($cmd[0] eq "import") {
> - die("Invalid arguments for import\n") if ($cmd[1] eq "" || defined($cmd[2]));
> - mw_import($cmd[1]);
> - } elsif ($cmd[0] eq "option") {
> - die("Too many arguments for option\n") if ($cmd[1] eq "" || $cmd[2] eq "" || defined($cmd[3]));
> - mw_option($cmd[1],$cmd[2]);
> - } elsif ($cmd[0] eq "push") {
> - mw_push($cmd[1]);
> - } else {
> - print STDERR "Unknown command. Aborting...\n";
> + if (parse_commands() == 1) {
> last;Better. A few minor nits:
The new subroutine is only parsing/invoking a single command at a time from the input line rather than multiple commands, so the name parse_commands() is slightly misleading. Perhaps parse_command() would be more appropriate.
Now that the functionality has been pushed into a subroutine, it does not necessarily need to be accessing global @cmd. It might be appropriate to pass @cmd as an argument to parse_command() (or not depending upon your preference).
The "Unknown command. Aborting" case indicates a failure. It's pretty typical for failures to return false rather than true. The resulting conditional would then read a bit more idiomatically:
if (!parse_command(\@cmd)) {
last;
}Show 35 quoted lines
> }
> } else {
> @@ -168,6 +153,31 @@ sub exit_error_usage {
> die "ERROR: git-remote-mediawiki module was not called with a correct number of parameters\n";
> }
>
> +sub parse_commands {
> + if ($cmd[0] eq "capabilities") {
> + die("Too many arguments for capabilities\n")
> + if (defined($cmd[1]));
> + mw_capabilities();
> + } elsif ($cmd[0] eq "list") {
> + die("Too many arguments for list\n") if (defined($cmd[2]));
> + mw_list($cmd[1]);
> + } elsif ($cmd[0] eq "import") {
> + die("Invalid arguments for import\n")
> + if ($cmd[1] eq "" || defined($cmd[2]));
> + mw_import($cmd[1]);
> + } elsif ($cmd[0] eq "option") {
> + die("Too many arguments for option\n")
> + if ($cmd[1] eq "" || $cmd[2] eq "" || defined($cmd[3]));
> + mw_option($cmd[1],$cmd[2]);
> + } elsif ($cmd[0] eq "push") {
> + mw_push($cmd[1]);
> + } else {
> + print STDERR "Unknown command. Aborting...\n";
> + return 1;
> + }
> + return 0;
> +}
> +
> # MediaWiki API instance, created lazily.
> my $mediawiki;
>
> --