Hello jmsnet, and welcome to the Monastery!

2) ... if the ticket request or data request fails then the return value is undef. Each time i call the subroutine and get a return value i then have to do an if loop to check whether the value is defined or not. Is there a more efficient way of doing this because I am basically just repeating if statements after each call eg. if i didn't get a ticket I could just call exit in the subroutine but it seems more logical to me and easier to follow if I handle it outside the subroutine.

You’re right, a subroutine should never exit. But it can die (Perl’s way of throwing an exception):

sub get_ticket
{ 
    my ($rest_client, $sdn_controller) = @_; 
    my $ticket;

    ...

    die "\$ticket undefined in get_ticket()" unless defined $ticket;

    return $ticket;
}
[download]

and then in the calling code you’re free to handle the exception or not. If you don’t, it propagates up through the call stack until it reaches the top level and the script dies. If you want to change the way the script exits, catch the exception at the top level:

MAIN:
{
    my $result = 0;       # normal termination

    eval
    {
        ...               # main code goes here
    }

    $result = 1 if $@;    # abnormal termination

    exit $result;
}
[download]

See die, eval, also Try::Tiny and TryCatch.

Hope that helps,

should a subroutine do one thing and one thing well rather than trying to combine both subroutines into one because of the common code between them

Why not both? :) You don't show us an example of the calling code, but it sounds like get_ticket could call "do_request", which returns the data structure, but get_ticket could extract just the ticket ID (scalar) and return that:

sub get_ticket {
  my $data = do_request(@_);
  if ($data) {
    return $data->{ticket_ID};
  }
  return; # error?
}
[download]

As for variable names ... I like to keep my functions in other namespaces - put all your REST calls into a single module, and then the fact the names look the same wouldn't really matter, and having them have the same names can sometimes be an advantage.

1) should a subroutine do one thing and one thing well rather than trying to combine both subroutines into one because of the common code between them. In other words I could use one subroutine to use the common code but then would have to make it more complex and test whether I was asking for a ticket or a just requesting data from the controller.

2) the return value from the ticket and data subroutines are values, scalar for the ticket, and a reference to a data structure for the data subroutine, the exact type of structure depending on what JSON sent back. So if the ticket request or data request fails then the return value is undef. Each time i call the subroutine and get a return value i then have to do an if loop to check whether the value is defined or not. Is there a more efficient way of doing this because I am basically just repeating if statements after each call eg. if i didn't get a ticket I could just call exit in the subroutine but it seems more logical to me and easier to follow if I handle it outside the subroutine.

Explicitly checking for errors after each call by testing the definedness (or simply the truthiness) of $ticket is one way to do that. The other common way is to use an exception-based model, in which the sub calls die if something goes wrong and then the calling code can either catch the exception (I recommend Try::Tiny for this) if it's something recoverable, or ignore it and let the exception terminate the program if not.

Either way, though, you should never, ever, ever exit in response to an error in a subroutine, because exit terminates the program immediately without giving the calling code (or any code further up the call stack) an opportunity to examine the problem and attempt to recover. Even if it's an unrecoverable error today, that might change six months from now, or the code could get reused in a different context where recovery may be possible.

Also, die allows you to provide an error message indicating what went wrong; exit does not. So always die on errors and reserve exit for clean exits when nothing has gone wrong.

3) finally as you can see from above i use the same variable names in both main and the subroutine but use my to keep them separate. I can see how that might confuse someone else looking at the code later and also if I forget to use my how it might confuse me :) I just keep running out of names. Is there a good standard to follow that others have found works ?

But I have nothing against reusing variable names for the same thing in different scopes, which is what you seem to be doing here. (The sub's $ticket gets returned and assigned to the other $ticket, so the name $ticket always refers to the same data, even if it's actually two different variables.)

Thanks to all who have responded, this forum is a very helpful place :)

Having a subroutine calling another subroutine would work really well for my case. I have no idea where I got the idea from but for some reason I was under the impression this wasn't a good thing to do but after reading the replies here and doing a bit of other searching it seems like quite a common way to do things as long as the flow is still logical.

Much appreciated

Have you considered using objects ?