You are not currently doing object-oriented programming. You are doing something that might superficially resemble OOP, but it's not OOP.

If you want to do OOP in Perl, I'd strongly suggest Moose or one of its friends (Mouse, Moo or Mo). These toolkits hide away much of the boilerplate code associated with Perl OOP, and allow you to concentrate on the business of writing and using your classes.

OOP aside, I'd say that your biggest problem though is the fact that you're not using DBI's prepared statements properly. Don't do this:

my $sth = $dbh->prepare("SELECT * FROM foo WHERE bar='$baz'");
$sth->execute;
[download]

Do this (thanks Anonymous Monk below for the correction):

my $sth = $dbh->prepare("SELECT * FROM foo WHERE bar=?");
$sth->execute($baz);
[download]

This allows DBI to handle escaping issues for you, and in many cases optimize multiple calls of execute.

Anyhow, here's my rewrite of your package using Moose and prepared statements...

... WHERE bar = ?, not ... WHERE bar = '?'

Before anyone bothers to look at this, you might want to remove your plaintext password from the code. Not that anyone will hack your database, but it is very common for people to reuse one or several passwords for all their various logins. It's obviously not relevant to making your code work, so you really should replace it with "dbpassword" or "xxxxxxxxx". And again, I'm not saying that anyone in this community will try to take advantage of this, but it's just a good idea to replace any sensitive information with junk before posting it to a publicly accessible web site that is indexed by major search engines.

thanks but these are not real. though i should have used 'dbuser' rather than 'xxxaccountxxx'

When I made my first reply, it was still 'root', and the other was also something else that I won't repeat but I do remember ;)


$,=qq.\n.;print q.\/\/____\/.,q./\ \ / / \\.,q.    /_/__.,q..
Happy, sober, smart: pick two.

What are you doing???

You have a constructor that doesn't do anything. Normally it would perform actions necessary to set up the object, for instance if your object was the database, the constructor might attempt to open a connection and store a handle to the database, as well as provide a mechanism for catching and reporting errors that occur.

You have a bunch of unfinished and unstarted methods. And then the methods that you do have are procedural and don't really fit with the OO paradigm.

Before you go any further, you should look up information about Object Oriented Programming, learn about abstraction, encapsulation, the difference between classes and objects, the relationship between objects and methods, etc. Then, only when you understand how OOP works, if you are working on a project that relies heavily on a database, I highly recommend DBIx::Class, a module that provides an object-oriented interface to a relational database (it is compatible with many backends, including MySQL). This will eliminate the need for SQL and it can generate classes with accessors and relationship mapping for all of your tables in a way that is intuitive and easy to understand. Right now your code, aside from being messy, wasteful, and mind-bogglingly obtuse, is vulnerable to SQL injection attacks (what will happen if I call $database->check_comment_record('1; drop table comment_record')?). Using DBIx::Class will solve these problems for you without you having to know anything about database security. Creating new records is as easy as

$schema->resultset('NewsPost')->create({
  heading => "Llama thief still at large!",
  text => "Police are investigating leads but have refused to comment 
+on possible suspects...",
  photos => [{
      img => 'llamas_01.png',
      caption => 'Five llamas have been confirmed missing since Tuesda
+y (File photo)',
     },{
      img => 'llamas_02.png',
      caption => 'Police Commissioner Turgidson, shown here making a p
+ress statement Wednesday morning',
     }],
   url => '/news/llamas',
})
[download]

You have a lot of reading to do before you can even try to do something like this. Wikipedia is an OK place to start. There are certain websites where you can obtain pdf copies of books about database programming. The CPAN documentation for DBI modules will be helpful if you sit down and read it very carefully. When you have enough of an idea about what you're doing to ask an actual question you can come back. The community here would be glad to help you, but only if you actually show us that you are someone who knows how to RTFM, use Google, and that you can figure basic things out on your own before asking for help with a specific issue or problem.

As a side note, building SQL strings is dynamic SQL. It is inefficient, pone to error, and allows for SQL injection.

As you're using prepare and execute on the actual statements, you can use placeholders and pass the variable's in a hash (the optional second parameter to execute), which is the first line of defense against SQL injection and more efficient.

That is, instead of:

my $sql = "select MNref from comment_record where MNid=" . "\"$self->{
+_MNid}\"";
my $ccr = $dbh->prepare($sql);
$ccr->execute() or die "$!";
[download]

use

my $sql = "select MNref from comment_record where MNid= ?";
my $ccr = $dbh->prepare($sql);
$ccr->execute($self->{_MNid}) or die $ccr->errstr;
[download]

It's also a lot easier to read. Note, also, return DBI's error, instead of just $!.