Illegal character in prototype for main

[hillgoogle]

I have an issue in my perl and its due to my mis-underatanding of perl
sub.

In my main pgm i have this statement that is calling my mail program.
It works fine UNLESS i have a user like:
tim.o'(e-mail address removed). When my user is just like
(e-mail address removed) then its fine. The error is "Illegal
character in prototype for main::SendMail : $,$,$ "

&SendMail($eml,$mail_text,$mail_sub);

sub SendMail($,$,$)
{
#who the email is being sent to
my $To = shift;

#the message body
my $mymsg = shift;

#the mail subject
my $mysubject = shift;

#From email address
my $from = "me\@somecompany.com";

my $msg = new MIME::Lite
To =>$To,
From =>$from,
Subject =>$mysubject,
Type =>'text/html',
Data =>$mymsg;

$msg->send;
}

Should I be putting these 3 vars in an array and then using that
array?

Thanks for your help. Mike

 

[Uri Guttman]

h> In my main pgm i have this statement that is calling my mail program.
h> It works fine UNLESS i have a user like:
h> tim.o'(e-mail address removed). When my user is just like
h> (e-mail address removed) then its fine. The error is "Illegal
h> character in prototype for main::SendMail : $,$,$ "

h> &SendMail($eml,$mail_text,$mail_sub);

don't use & for calling subs. actually it will also bypass the prototype
you are using

h> sub SendMail($,$,$)

and speaking of prototypes, they are not a good thing in general and
rarely needed. the , is not a prototype char and that is the bug you are
seeing. it should just be $$$. but since you bypass it by calling with &
you can see it is of little use here. just drop using them unless you
find a reason to use them.
h> {
h> #who the email is being sent to

useless comment.

h> my $To = shift;

h> #the message body
h> my $mymsg = shift;

h> #the mail subject
h> my $mysubject = shift;

much better to just assign to a list of vars:

my( $to, $msg, $subject ) = @_ ;

h> #From email address
h> my $from = "me\@somecompany.com";

use single quotes for literals without interpolation. and then you don't
need the \@

my $from = '(e-mail address removed)';


h> my $msg = new MIME::Lite

don't use indirect object calls. perldoc perlobj explains why. use a
direct call like this:

MIME::Lite->new(

uri

 

[hillgoogle]

  h> In my main pgm i have this statement that is calling my mail program.
  h> It works fine UNLESS i have a user like:
  h> tim.o'(e-mail address removed). When my user is just like
  h> (e-mail address removed) then its fine. The error is "Illegal
  h> character in prototype for main::SendMail : $,$,$ "

  h> &SendMail($eml,$mail_text,$mail_sub);

don't use & for calling subs. actually it will also bypass the prototype
you are using

  h> sub SendMail($,$,$)

and speaking of prototypes, they are not a good thing in general and
rarely needed. the , is not a prototype char and that is the bug you are
seeing. it should just be $$$. but since you bypass it by calling with &
you can see it is of little use here. just drop using them unless you
find a reason to use them.
  h> {
  h> #who the email is being sent to

useless comment.

  h> my $To = shift;

  h> #the message body
  h> my $mymsg = shift;

  h> #the mail subject
  h> my $mysubject = shift;

much better to just assign to a list of vars:

        my( $to, $msg, $subject ) = @_ ;

  h> #From email address
  h> my $from = "me\@somecompany.com";

use single quotes for literals without interpolation. and then you don't
need the \@

        my $from = '(e-mail address removed)';

  h> my $msg = new MIME::Lite

don't use indirect object calls. perldoc perlobj explains why. use a
direct call like this:

        MIME::Lite->new(

uri

Uri, so you are saying my code shold be as follows:
******************************************************************************

SendMail($eml,$mail_text,$mail_sub);

sub SendMail($$$)
{
my ($To, $mymsg, $mysubject) = @_;

my $from = "me\@somecompany.com";

my $msg = MIME::Lite->new(
To =>$To,
From =>$from,
Subject =>$mysubject,
Type =>'text/html',
Data =>$mymsg);

$msg->send;
}

******************************************************************************
Right?
Mike

 

[Rainer Weikusat]

[...]

h> &SendMail($eml,$mail_text,$mail_sub);

don't use & for calling subs.

At least not when calling them with arguments. &something is useful in
subroutines performing some 'auxiliary task' and then 'forwarding' the
call to another routine: It means 'invoke something and use the
current @_ as argument list'. Semantically, this is (mostly)
equivalent to something(@_) except that it is shorter and the overhead
of creating a new @_ whose content is identical to the present @_ is
avoided.

 

[hillgoogle]

"h" == hillgoogle  <[email protected]> writes:

[...]

  h> &SendMail($eml,$mail_text,$mail_sub);

don't use & for calling subs.

At least not when calling them with arguments. &something is useful in
subroutines performing some 'auxiliary task' and then 'forwarding' the
call to another routine: It means 'invoke something and use the
current @_ as argument list'. Semantically, this is (mostly)
equivalent to something(@_) except that it is shorter and the overhead
of creating a new @_ whose content is identical to the present @_ is
avoided.

OK ... then it should be:

******************************************************************
&SendMail($eml,$mail_text,$mail_sub);

sub SendMail($$$)
{
my ($To, $mymsg, $mysubject) = @_;

my $from = '(e-mail address removed)';

my $msg = MIME::Lite->new(
To =>$To,
From =>$from,
Subject =>$mysubject,
Type =>'text/html',
Data =>$mymsg);

$msg->send;
}
******************************************************************

Better? TY Mike

 

[Uri Guttman]

h> Uri, so you are saying my code shold be as follows:

h> SendMail($eml,$mail_text,$mail_sub);

h> sub SendMail($$$)

drop the prototype.

sub SendMail {

and most put the opening { on the sub line.

h> {
h> my ($To, $mymsg, $mysubject) = @_;

don't mix upper/lower case names. also why the my prefix? they are
scoped with my so $msg and $subject are better names.

also indent code in the sub body.

h> my $from = "me\@somecompany.com";

you didn't change the quotes as i showed you.

h> my $msg = MIME::Lite->new(

h> To =>$To,
h> From =>$from,
h> Subject =>$mysubject,
h> Type =>'text/html',
h> Data =>$mymsg);

some white space helps there. also don't put the close ); on the data
line. it is hard to see and means you can't cut/paste that line or move
the close ) as easily.

h> $msg->send;
h> }

h> Right?

not completely but better. this is how i would do it. note ALL the
little changes like whitespace and name fixes.

send_mail( $eml, $mail_text, $mail_sub );

sub send_mail {

my( $to, $msg, $subject ) = @_;

my $from = '(e-mail address removed)';

my $msg = MIME::Lite->new(
To => $to,
From => $from,
Subject => $subject,
Type => 'text/html',
Data => $msg
);

$msg->send() ;
}

uri

 

[Uri Guttman]

[...]

  h> &SendMail($eml,$mail_text,$mail_sub);

don't use & for calling subs.

At least not when calling them with arguments. &something is useful in
subroutines performing some 'auxiliary task' and then 'forwarding' the
call to another routine: It means 'invoke something and use the
current @_ as argument list'. Semantically, this is (mostly)
equivalent to something(@_) except that it is shorter and the overhead
of creating a new @_ whose content is identical to the present @_ is
avoided.

h> OK ... then it should be:

h> ******************************************************************
h> &SendMail($eml,$mail_text,$mail_sub);

no.

h> sub SendMail($$$)

and no again.

drop both the & and the prototype. one disables the other and the
prototype is useless to you here. it is not a good feature in general
and has only a few very narrow uses.

uri

 

[Wolf Behrenhoff]

Am 23.08.2011 23:46, schrieb (e-mail address removed):

[...]

h> &SendMail($eml,$mail_text,$mail_sub);

don't use & for calling subs.

At least not when calling them with arguments. &something is useful in
subroutines performing some 'auxiliary task' and then 'forwarding' the
call to another routine: It means 'invoke something and use the
current @_ as argument list'. Semantically, this is (mostly)
equivalent to something(@_) except that it is shorter and the overhead
of creating a new @_ whose content is identical to the present @_ is
avoided.

OK ... then it should be:

******************************************************************
&SendMail($eml,$mail_text,$mail_sub);

sub SendMail($$$)

No! Re-read what has been written. Using the & overrides possible
prototypes - and in addition (and that's what Rainer's comment was
about) it passes all current arguments to the sub if you don't use
parameters.

Maybe an example can explain this better:

sub foo {
print "Foo: @_";
}
sub bar { &foo }
bar("Hello", 42);

Here you're calling bar with two parameters. bar then calls foo. Since &
has a special meaning, the program will output Foo: Hello 42. Try
removing the & here!

To summarise: don't use & if you don't know what it is for.


Please use this:

SendMail($eml,$mail_text,$mail_sub);

sub SendMail {
...
}

 

[Charlton Wilbur]

u> drop the prototype.

u> sub SendMail {

u> and most put the opening { on the sub line.

Don't make me get my torch and pitchfork.

Charlton

 

[Ted Zlatanov]

UG> not completely but better. this is how i would do it. note ALL the
UG> little changes like whitespace and name fixes.

send_mail( $eml, $mail_text, $mail_sub );

sub send_mail {

my( $to, $msg, $subject ) = @_;

my $from = '(e-mail address removed)';

my $msg = MIME::Lite->new(
To => $to,
From => $from,
Subject => $subject,
Type => 'text/html',
Data => $msg
);

$msg->send() ;
}

(end Uri's example)

I wanted to suggest something perhaps more generally useful, where all
the parameters you pass to your function will simply become hash key
overrides. Also note the warning at the end.

Ted

#+begin_src perl

#!/usr/bin/perl

use warnings;
use strict;
use Data:umper;
use MIME::Lite;

send_mail( From => '(e-mail address removed)', To => "you", Data => "message data", Subject => "subject you want" );

sub send_mail {

my %settings = (
To => "Default Recipient",
From => "Default Sender",
Subject => "Default Subject",
Type => 'text/html',
Data => "Default Message Text"
);

my %overrides = @_;

$settings{$_} = $overrides{$_} foreach sort keys %overrides;

my $msg = MIME::Lite->new(%settings);
$msg->send() if defined $msg;
warn "Could not use settings to create message " . Dumper(\%settings)
unless defined $msg;
}

#+end_src

 

[Ted Zlatanov]

TM> I often start out my subroutines thusly:

TM> sub send_mail {
TM> my %args = (
TM> To => "Default Recipient",
TM> From => "Default Sender",
TM> Subject => "Default Subject",
TM> Type => 'text/html',
TM> Data => "Default Message Text",
TM> @_
TM> );

TM> ...

TM> There is no need to explicitly hashify and overwrite, I just let the
TM> usual "last one wins" behavior of a list assignment to a hash
TM> handle all of that for me.

I thought that might feel a little too much like black magic to the OP,
who is probably a Perl beginner. Also it's easier to hook further
changes or messages based on certain keys if you do the overrides
explicitly. FWIW I would probably do the list assignment in my own
code but I tend to overthink examples I post to c.l.p.misc

Ted