a script to wrote to DB. Is it ok?

[bazz]

Hello again,

I have made a sub wqhich is to write to the session and also to the Db. It does what I want but is it the best way?

Please let me know.

bazz


#This sub adds the proposed GroupName to the Session and to the Db.
sub addGroupToSession {
$session->param('groupToAdd', $groupToAdd);
my $newGroupToBeAdded = $session->param('groupToAdd');

#PUT IN HERE THE SCRIPT TO WRITE TO THE TBL_GROUPS IN THE dB.
my $db = '**********';
my $srv = '*********';
my $user = '*********';
my $pass = '**********';
my $port = '****';
my $table = 'tbl_businessGroups';

if (!$groupToAdd) {
print qq(
<div id="adminInput">
<p>What are you doin'?? There is no name to add</p>
<p>Please go back and try again.</p>
</div>
);
} else {
my $dbh = DBI->connect("DBI:mysql:$db:$srv:$port", $user, $pass,
{RaiseError => 1, PrintError => 0})
or die "Can't connect: DBI->errstr()";

$dbh->do("INSERT into $table (Group_ID, Business_Group_Name) values ('', '$groupToAdd')");
$dbh->disconnect;

print qq($newGroupToBeAdded has been added to the Session and stored in the DataBase.
);
}
}

[FishMonger]

Is this the only place where you're doing any DB calls? If not, then I'd move the creation of the $dbh object so that it has file scope.

It's rarely a good idea to work directly with global vars inside the sub. It's better to pass in the var and work with a private copy.

The naming convention of your vars is inconsistent and doesn't follow the rules of "Best Practices". $newGroupToBeAdded would be better as $new_group_to_be_added

The assignment of $newGroupToBeAdded doesn't make sense since you just assigned the same value to the session var so that makes 3 copies of the same var in the sub. Also, since you're not using it anywhere else in the sub, it would be more appropriate to move the session var assignment either to outside of the sub or in the else block.

Printing the html as you did is fine, but I prefer to use the CGI methods.

Assuming that the $dbh creation is moved outside of the sub, I'd probably do it more like this (which would need to be tested and tweaked).sub add_group_to_session {
my $group_to_add = shift;
my ($session, $dbh) = @_; # passing the objects isn't really nessesary,
# but it's best to stay consistant
my $table = 'tbl_businessGroups';

if( !$group_to_add ) {
print div({id=>"adminInput"},
p("What are you doin'?? There is no name to add"),
p("Please go back and try again."),
);
}
else {
$session->param('groupToAdd', $group_to_add);
$dbh->do("INSERT into $table (Group_ID, Business_Group_Name) values ('', '$group_to_add')");
print div("$group_to_add has been added to the Session and stored in the DataBase.");
}
}

[bazz]

Is this the only place where you're doing any DB calls? If not, then I'd move the creation of the $dbh object so that it has file scope.


Yep it is the only place. On entering a value to a form, and having submitted it to this script, only this sub is run.


It's rarely a good idea to work directly with global vars inside the sub. It's better to pass in the var and work with a private copy.

The naming convention of your vars is inconsistent and doesn't follow the rules of "Best Practices". $newGroupToBeAdded would be better as $new_group_to_be_added

Great help there, thanks

The assignment of $newGroupToBeAdded doesn't make sense since you just assigned the same value to the session var so that makes 3 copies of the same var in the sub. Also, since you're not using it anywhere else in the sub, it would be more appropriate to move the session var assignment either to outside of the sub or in the else block.

Yeh, I was creating that variable just to check that the session var was created/entered.


Printing the html as you did is fine, but I prefer to use the CGI methods.

yep, I have still to take time to learn that part coz I still find this way easier to read.

Assuming that the $dbh creation is moved outside of the sub, I'd probably do it more like this (which would need to be tested and tweaked).sub add_group_to_session {
my $group_to_add = shift;
my ($session, $dbh) = @_; # passing the objects isn't really nessesary,
# but it's best to stay consistant
my $table = 'tbl_businessGroups';

if( !$group_to_add ) {
print div({id=>"adminInput"},
p("What are you doin'?? There is no name to add"),
p("Please go back and try again."),
);
}
else {
$session->param('groupToAdd', $group_to_add);
$dbh->do("INSERT into $table (Group_ID, Business_Group_Name) values ('', '$group_to_add')");
print div("$group_to_add has been added to the Session and stored in the DataBase.");
}
}

brilliant FishMonger; thanks. I'll look into moving the Db calls but this script has several subs which run as required, based on passed parameters. So each time the script runs, a different Db call is made, hence my thought about putting the calls into each respective sub. I'll think it through some more.

bazz