As a general thing lines that are very long, especially if there is important stuff at the end, and even worse of the structure is convoluted, compared with short lines are hard to understand. Keep your lines short, or at least wrap them sensibly. For example:

    return $self->{'dbh'}->selectrow_array("SELECT COUNT(*) FROM $Bod:
+:Variables::M_Table WHERE $Bod::Variables::C_Table_id$Bod::Variables:
+:C_Table = ?, $Bod::Variables::B_Table_id$Bod::Variables::B_Table = ?
+ AND Permission_idPermission = ? AND date > NOW() - INTERVAL 2 SECOND
+", undef, $contact, $business, $permission);
[download]

is easier to maintain written:

    return $self->{'dbh'}->selectrow_array(
        "SELECT COUNT(*) 
            FROM $Bod::Variables::M_Table
            WHERE $Bod::Variables::C_Table_id$Bod::Variables::C_Table 
+= ?,
                    $Bod::Variables::B_Table_id$Bod::Variables::B_Tabl
+e = ?
                AND Permission_idPermission = ?
                AND date > NOW() - INTERVAL 2 SECOND",
        undef, $contact, $business, $permission
        );
[download]

Consider using spaces instead of tabs for indentation.

Consider having new return an instance of Bod::CRM with its error field set, or throw an exception, for a DB connection failure.

Consider removing warnings in lower level code. Low level code may not have enough context to be really helpful and low level warnings can end up generating a noisy log. Calling code can emit warnings if appropriate and can generally provide a better semantic context.

Consider using exceptions for handling all errors so that calling code can chunk error handling into blocks of related code rather than having to deal with handling errors on all function calls. That extends quite well to having DBI code throw exceptions rather than having fine grain error checking at a low level (see RaiseWarn and RaiseError in DBI).

Consider passing named parameters to find and add. That way your existing hash ref gets passed in as a hash with no other change, but a simple find doesn't need to have a hash ref spun up. That makes it easier to write the code and easier to read and check:

    my %fields = (
        firstname => 'John',
        lastname  => 'Smith',
        pri_email => 'john.smith@example.com',
    );
    $crm->add(%fields);

    $crm->add(firstname => 'John', lastname => 'Smith', pri_email => '
+john@xxxx.com');
    my @contacts = $crm->find(firstname => 'John');
[download]

Consider using spaces instead of tabs for indentation.

8 simple words to start a holy war.

---

🦛

From that node subtitled "My arguement in favor of tabs": "Reputation: -34 (+44 -78)" - says it all really.

Optimising for fewest key strokes only makes sense transmitting to Pluto or beyond

Consider using spaces instead of tabs for indentation

I never cease to be amazed at how The Monastery reveals that things I thought were standard practice...just aren't!

The only formal education I have ever had relating to writing code was a short C++ course as part of my undergrad degree course and an even shorter Java course at another university as a small part of wider study. In both courses in the 1990's we were told to use tabs as indents and I didn't question why. Since then I have never questioned that anyone might do anything else...

Is this a case of academia being out of touch with working practices or has the favour of spaces changes in the last quarter of a century?

Consider passing named parameters to find and add. That way your existing hash ref gets passed in as a hash with no other change, but a simple find doesn't need to have a hash ref spun up.

Thank you - I don't know why I didn't do it exactly like that because most of the time it will be a hash, not a hashref that gets passed in. Except there are a lot of parameters which could change...hence why I read the table columns out of the information_schema in the new method.

Two points from Perl Best Practices (Documentation section) you might want to contemplate:

• Distinguish user documentation from technical documentation.
• Place POD as close as possible to the end of the file.

Personally, I dislike intermingling POD with code. Documentation of code is for programmers, so I use simple Perl comments (not POD) for that, right by the code it is documenting. Makes it easier to update the comments in step with the code.

Technical documentation of the system architecture (not the code) I place in a single large POD documentation section at the end of the file, or in a separate file.

User documentation should be kept separate from technical documentation. For that, I suggest either a separate POD file (or a big POD block at the end of the source file) ... or simply a Usage subroutine displayed when the user enters --help at the command line, making the user documentation more discoverable and more likely to be read.

See also:

• Ten Essential Development Practices by Damian Conway
• Re: looking for feedback on my first script

G'day Bod,

As far as I can see, there's nothing horribly wrong with what you've presented; however, there are a number of things I would have done differently. The following is very much biased towards my own preferences: pick and choose any bits you like; skip the bits you don't want; feel free to ignore all of it. I've also added some useful POD-related documentation links.

I write a lot of modules. The majority would be for $work; although, I do write quite a few for personal use. Regardless, these are not for CPAN: the $work ones are Commercial in Confidence; the personal ones are too specific to my needs, or in some cases just testing ideas, and are not considered to be of general usefulness.

Having said that, I still write these modules as if they were going to be uploaded to CPAN, and include: POD with the code; standard files such as Makefile.PL, MANIFEST, README and Changes; tests in a t/ directory; and so on. This allows me to, amongst other things: test changes (make test); easily install the modules (make install); create distributions (make dist) which I can transfer to other machines (e.g. scp to $work). I also find the POD to be exceptionally useful: six months or so after completing a module, I can rarely remember the exact details of every function and method, particularly when it comes to optional arguments and complex return values (e.g. array and hash references) — perldoc ModuleName makes my life so much easier.

I never sit down and write all the files needed for the type of module described above; instead I use Module::Starter with the Module::Starter::PBP plugin. The latter is mainly for the excellent templates and does not represent a slavish adherence to "Perl Best Practices" guidelines.

I have multiple config* files for my various uses. As an example, here's the currently active one for personal modules using Perl v5.32:

ken@titan ~/.module-starter
$ cat config
author:  Ken Cotterill
email:   kcott@cpan.org
builder: ExtUtils::MakeMaker
plugins: Module::Starter::PBP
template_dir: /home/ken/.module-starter/P532
[download]

I have modified the templates for my own preferences. For personal modules, that can be pretty much whatever I want. For $work modules, there are certain constraints, policies, and so on, that I need to take into account (e.g. specific Perl version, copyright text, etc.).

There are three test files that I always add to the templates. These use Test::Pod, Test::Pod::Coverage and ExtUtils::Manifest.

The idea behind all of this is to create templates once, then let module-starter create as many module frameworks as I want, all with the same, consistent look and feel. Obviously, I still need to fill in details, such as the description of functions, but all of the boilerplate has been done for me and I can pretty much forget about that part of the process. Partly DRY; mostly "the first great virtue of a programmer": laziness.

Here's the POD documentation to which I alluded earlier:

perlpod

This is the basic documentation and a handy reference. If you're not familiar with it, check out the use of multiple angle brackets allowing nesting of "Formatting Codes" (it's near the end of that section).

perlpodspec

This is the gory details. It's probably more geared to those writing POD parsers and the like, but still has useful information. I'd generally only use this if I couldn't find sufficiently specific information in perlpod.

perlpodstyle

This has information on the standard sections of a manual page. You may find this useful.

Things I would have done differently (in no particular order) and bear in mind, as I said above, these are my personal preferences:

• I put all the code first; and all the POD last.
• I always start POD with =encoding utf8.
• I generally put whitespace before and after code blocks (if, while, etc.).
• I try to keep lines under 80 characters; I'll go out of my way not to exceed 120 characters. This simply makes reading the code easier. I acknowledge that this is not always possible, but mostly it is. As an example, I'd prefer:

  my $dbh = DBI->connect(
      "dbi:mysql:$Bod::Variables::db_prefix$env:localhost:3306",
      "$Bod::Variables::db_username",
      "$Bod::Variables::db_password"
  );
  [download]

• Your use of $Bod::Variables::varname, locks you into a single class. Consider this which allows subclassing:

  # Uncomment one of these
  # my $bvar = Bod::Variables::->new();
  # my $bvar = Bod::Variables::Special::->new();
  my $dbh = DBI->connect(
      'dbi:mysql:' . $bvar->db_prefix() . "$env:localhost:3306",
      $bvar->db_username(),
      $bvar->db_password()
  );
  [download]

  Obviously, as I have no knowledge of Bod::Variables, that's highly contrived with a lot of guesswork; however, I hope you get the idea of the increased flexibility it offers.
• I'd generally use more meaningful names than find() and add(); perhaps find_people() and add_person() would aid in reading and understanding the code.
• The get() function could also do with a more meaningful name. If it's part of the API, document it in the SYNOPSIS; if it's not, prefix it with an underscore, to nominally indicate that it's private, and remove it from the POD (if renamed to _get_person_info(), the function is so simple that it wouldn't even need a comment).

1. Use a configuration file e.g. to store your environment setting as well as database credentials (decouple settings from code)
2. Use DBIx::Class - you'll get search/insert and more functionality (almost) for free.
3. Consider named parameters for methods with more than two args (makes code more self documenting).

Use a configuration file

Sorry if I'm being a bit thick but isn't that what I'm doing with use Bod::Variables; because all that's in there is a package statement and a list of scalars defined with our.

Thanks for the suggestions.

You're still keeping sensitive information in your code base, so that's not a configuration file.

In the real world they are typically managed separately from the code base and often by people who may not be programmers (I realise your use case is simpler than the typical one).

See Config::Tiny for an example.

Thank you

Yes, I did wonder if I was going to start a major debate about POD being inline or clumped at the bottom. I took the view, without thinking about it too much, that it is probably easier to write it next to the subroutines it relates to whilst developing the module. Then, once that subroutine is proven working and shouldn't need touching again, shift its POD to the bottom out of the way. Whether this moving would ever happen of course is up for debate!

For this one I don't need to worry about CPAN rendering but I still want to try and get it right for the future.

Is it worth also trying to write tests for this - again for practice and learning - if so, where would I start?

Is it worth also trying to write tests for this - again for practice and learning - if so, where would I start?

Before you write the module. :)

The main point of TDD is that writing the tests first (or at least early) affects and improves the design of the module! Writing a test first forces you to focus on interface - from the point of view of the user. Hard to test code is often hard to use. Simpler interfaces are easier to test. Functions that are encapsulated and easy to test are easy to reuse. Components that are easy to mock are usually more flexible/extensible. Testing components in isolation ensures they can be understood in isolation and promotes low coupling/high cohesion. Implementing only what is required to pass your tests helps prevent over-engineering.

To give a clearer picture of where I'm coming from, a quote from Ten Essential Development Practices:

The most important aspect of any module is not how it implements the facilities it provides, but the way in which it provides those facilities in the first place. If the module’s API is too awkward, or too complex, or too extensive, or too fragmented, or even just poorly named, developers will avoid using it. They’ll write their own code instead. In that way, a poorly designed module can actually reduce the overall maintainability of a system.

Designing module interfaces requires both experience and creativity. Perhaps the easiest way to work out how an interface should work is to “play test” it: to write examples of code that will use the module before implementing the module itself. These examples will not be wasted when the design is complete. You can usually recycle them into demos, documentation examples, or the core of a test suite.

The key, however, is to write that code as if the module were already available, and write it the way you’d most like the module to work.

Once you have some idea of the interface you want to create, convert your “play tests” into actual tests. Then it’s just a Simple Matter Of Programming to make the module work the way that the code examples and the tests want it to.

See also: Re: Winning people over to better development practises (TDD) and On Interfaces and APIs

100%, yes, write tests for your module. It's unclear if you meant for the POD. I am not sure I would go that far. Besides it wouldn't go to improving your test coverage :).

Automatic A++ for even using POD

What are Monks using to generate readable documents from POD?
I'm guessing something better than pod2html...

*I* *usually* have an init() method called automatically in new() to do any initialisation, e.g. connect and read DB. This has the advantage of being able to re-initialise any time I like after instantiation by just calling $obj->init().

I am still undecided (or haven't read enough) on how to propagate errors, error messages and error codes to caller all the way up. The method you are using by saving the *last* error message will need some tweaking when you are nesting sub calls (and if you do that it will soon result to a paper-pushing system). Perhaps simply append to $error with obligatory clearing it when starting a-fresh. Alternatively throw an exception which is the "modern" thing to do but that imposes a whole new style which I don't see often in Perl modules (arbitrary judgement), see e.g. Exception::Class. Sometimes I return a complex data structure with error code, error message, data etc. but I only do that for complex subs.

I appreciate Perl more when I have to work with Java and its super-strict typing.:

sub work {
  my $something_wrong = 1;
  if( $something_wrong ){
    #return Error->new(0, "because ...");
    return bless [0, "because ..."] => 'Error'
  }
  # edit: changed to a hashref to show returning different types
  # it need not be a ref at all, e.g. can send a hash too
  #return [1,2,3]
  return {1=>2, 3=>4}
}

my $ret = work();
if( ref($ret) eq 'Error' ){ die "error: @$ret" }
print "got some results:".Dumper($ret);
[download]

I am still undecided (or haven't read enough) on how to propagate errors, error messages and error codes to caller all the way up

As you can tell from my code - I am also undecided!
This was something that I hoped people would comment on and I wasn't disappointed.

I am fairly sure that I don't need to go as far as implementing full blown Exceptions, especially for modules that will only ever be used internally. This is the first time I have tried setting an 'error' field within the module to provide a verbose message and it seems to work quite well. Having said that, I would like to adopt more consistency both in this module and for other modules going forward.

I do like the idea of an init(); method although I'm not sure of a situation where I would want to reconnect to the database within the same session.