Re: [PATCH v2 15/22] git-remote-mediawiki: Put long code into a subroutine

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jun 7, 2013 at 5:42 PM, Célestin Matte
<celestin.matte@xxxxxxxxxx> wrote:
> Signed-off-by: Célestin Matte <celestin.matte@xxxxxxxxxx>
> Signed-off-by: Matthieu Moy <matthieu.moy@xxxxxxxxxxxxxxx>
> ---
>  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;
  }

>                 }
>         } 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;
>
> --
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]