Why don't you pack it all up into a proper Perl-distribution and put it on CPAN?

I am lazy. Why don't you pack it all up into a proper Perl-distribution and put it on CPAN?

Why should he? After all it wouldn't be very lazy of him. And AFAIK, CPAN encourages code-review before upload, and this is what the OP is asking for.

Why did the op turn here? xml rpc - Design improvements for Perl code library remote server - Stack Overflow

Please, rather than critizing my position, go ahead and review that code: download it, try to install it, debug it and let us all know. Good karma to you for your efforts.

CountZero

A program should be light and agile, its subroutines connected like a string of pearls. The spirit and intent of the program should be retained throughout. There should be neither too little or too much, neither needless loops nor useless variables, neither lack of structure nor overwhelming rigidity." - The Tao of Programming, 4.1 - Geoffrey James

My blog: Imperial Deltronics

Yes, I'm looking for code review feedback. It's hard to find for a project when you're the sole developer and it lacks a community of users using it.

It's a remote server for Robot Framework test framework, and from what I can tell, there are potential Perl users for that, but it's relatively small compared to Java, .NET, and Python. :(

And the main Robot Framework community developers are not Perl developers and don't have the time to help code review. :(

Packaging it as CPAN module is one goal I have in mind for the project. Haven't gotten to it. By the way, I'm a novice Perl developer, so have never actually built CPAN modules, nor have I developer Perl module test (*.t) files before. So that would be all new to me.

I know posting code or link to direct files is easier, but browsing an open source repo ain't that time consuming. But fine, here's the direct links:

code files
http://code.google.com/p/plrobotremoteserver/source/browse/trunk/robotremoteserver.pm

http://code.google.com/p/plrobotremoteserver/source/browse/trunk/examplelibrary.pm

http://code.google.com/p/plrobotremoteserver/source/browse/trunk/exampleremoteserver.pl

wiki page about the project & how to use
http://code.google.com/p/plrobotremoteserver/wiki/UsageInfo

list of open issues & to do items
http://code.google.com/p/plrobotremoteserver/issues/list

I am not a server specialist, so I am not the right Monk to comment on that aspect of your code.

One thing I am curious about is why you commented out use strict; use warnings;?

CountZero

A program should be light and agile, its subroutines connected like a string of pearls. The spirit and intent of the program should be retained throughout. There should be neither too little or too much, neither needless loops nor useless variables, neither lack of structure nor overwhelming rigidity." - The Tao of Programming, 4.1 - Geoffrey James

My blog: Imperial Deltronics

Your package doesn't match the filename, see Simple Module Tutorial , the filename should match the package name.

Instead of comments write documentation :) Tutorials: POD in 5 minutes and maybe

<Reveal this spoiler or all in this thread>

See Modern Perl and Class::Inspector AND Howto create a Moose baseclass/superclass/contract? Moose equivalent of Module::Pluggable? Favorite way to create base/super class?

You might start with

module-starter --author="David Luu" --email=man...mail.com --license=LGPL --module=RobotRemoteServer,RobotRemoteServer::Frontier,RobotRemoteServer,RobotRemoteServer::RPCXMLServer

Added to MANIFEST: Changes
Added to MANIFEST: ignore.txt
Added to MANIFEST: lib/RobotRemoteServer.pm
Added to MANIFEST: lib/RobotRemoteServer/Frontier.pm
Added to MANIFEST: lib/RobotRemoteServer/RPCXMLServer.pm
Added to MANIFEST: Makefile.PL
Added to MANIFEST: MANIFEST
Added to MANIFEST: README
Added to MANIFEST: t/00-load.t
Added to MANIFEST: t/boilerplate.t
Added to MANIFEST: t/manifest.t
Added to MANIFEST: t/pod-coverage.t
Added to MANIFEST: t/pod.t
Created starter directories and files

Then maybe write something like


package RobotRemoteServer;
use Class::Inspector;

sub get_keyword_names {
    my ($self) = @_;
    my $class = $self->lib;
    my @methods = Class::Inspector->methods( $class, 'public' );
    return \@methods;
}

sub run_keyword {
   my $self = shift;
   my ($stdout, $stderr) = capture { ... };
}
sub start_server {
    my $self = shift;
    $self->backend->start_server;
}
[download]

package RobotRemoteServer::Frontier;
sub _shut_down_request {
RobotRemoteServer::Frontier
sub start_server {
  my ($self) = @_;
  my $svr = Frontier::Daemon->new(
    methods => {
      get_keyword_names => sub { $self->get_keyword_names(@_) },
      run_keyword => sub { $self->run_keyword($_[0], eval{@{$_[1]}}) }
+,
      stop_remote_server => sub {
          $srv->_shutdown('yes please');
          $self->schedule_shutdown;
          return;
      },
    ....
}

sub schedule_shutdown {
    my $self = shift;
    my $url = $self->url;
    if( eval { require threads; } ){
        threads::async {
           sleep 5;
           require Frontier::Client;
           Frontier::Client->new( 'url' => $url )->call('stop_remote_s
+erver');
        };
    } elsif( eval { require Proc::Background; } ){
        Proc::Background->new(
            $^X,
            '-e', q{
use Frontier::Client;
sleep 5;
my $url = shift @ARGV;
Frontier::Client->new( 'url' => $url )->call('stop_remote_server');
exit;       },
            $url,
        );
    }
}
sub Frontier::Daemon::_shutdown {
    my( $self, $true ) = @_;
    ${*$self}{'_Frontier::Daemon::_shutdown'} = !!$true if $true; 
    return ${*$self}{'_Frontier::Daemon::_shutdown'} ;
}

sub Frontier::Daemon::accept { ## monkeypatch
    package Frontier::Daemon;
    my $self = shift;
    if( $self->_shutdown ){
        close $self;
        return;
    } else {
        return $self->SUPER::accept;
    }
}
[download]

To handle shutdown in similar way as https://robotframework.googlecode.com/hg/tools/remoteserver/robotremoteserver.py you could monkeypatch shutdown, but writing your own frontier deaemon ( Frontier::Daemon::Forking ) is probably the correct way to do it

backend is the new argument, RobotRemoteServer::Frontier by default

use RobotRemoteServer;
#my $rob = RobotRemoteServer->new (qw' host localhost port 666  ');
my $rob = RobotRemoteServer->new (qw'
  host    localhost 
  port    666 
  backend Frontier
  lib     ExampleLibrary
');
$rob->start_server;
[download]

Maybe it makes more sense for backends to be named as RobotRemoteServer::Backend::Frontier ...

But that is a start, pod beats comments, seperate backends into seperate modules, use existing modules for introspection , has-a not isa-a ...

Future direction:
maybe Moo/Moose or Badger for accessors/mutators...
Maybe POE server ( ex Hopkins::Plugin::RPC ), ex?Catalyst::Plugin::Server::XMLRPC, Plack for ideas
maybe cooperation with Daemon::Generic, AnyEvent::XMLRPC, ...

If I were more knowledgeable, I might make better suggestions ;)

Welcome daluu,

First, I don't have a clue about what you are trying to do, but since you do, maybe you could explain why your approach is 'better' or 'easier' or 'friendlier' or 'whatever' for your potential users.

One way to find the value of your code, is to profile your server with 'Devel::NYTProf'. This great tool will help you find your server's bottlenecks and help you improve your server performance. Set a realistic benchmark like ___ (your input needed) serves per minute, and then use the detailed information from 'Devel::NYTProf' to improve your own code. Look for code that can be moved outside of a loop and at the same time it will help you improve your Perl coding skills.

Try to improve your code by 100%. If you only get 1-2% then your code is optimized already. But be honest with yourself.

When you find something that you think could be better, but don't know how to improve the code -- that's when to ask the PM members for specific help. You'll get some good answers and a lot more help.

Good Luck!

Hi flexvault,

Thanks for the performance tuning tip, I will look into that some time. Though my server is not likely to be used heavily with high traffic. But performance checking it might be good for finding any potential memory or resource leaks or high CPU consumption, deadlocks. Otherwise, I don't really need to optimize the code for performance at this time.

I actually made this post in terms of code design improvements. Specifically 2 things. (1) whether some of the code could be written according to modern (and proper?) Perl standards and using better Perl modules, optimizing for standards (rather than performance). (2) for any suggestions on how to fix or tackle the open issues and todo items I've listed in the project.

FYI, the server is used to serve QA/automation test libraries written in Perl remotely (on different machine or even just localhost) over XML-RPC for integration/use by RobotFramework.org test automation framework, using it's remote library API specification (which utilizes XML-RPC). The details on how the server works and what it's use is can be found on the wiki page link I mention earlier and in the Robot Framework documentation. Functionally, it works for basic support, but there's a few areas that can be improved for it to be up to standards with the reference spec implementation (the Python remote server for same framework).