"Smart" is usually part of the name of badly-named modules where the author is claiming that the module is "smart". So the current name came nowhere near pointing me toward what the module does. Test::AskAnExpert seems a much more informative name.1 Even Test::Ask would be better.
The documentation would be easier to understand if the examples were more illustrative. get_yes("question") illustrates very little. Since it doesn't give me any clues about what type of question would be asked, it also does very little to help me understand what the point of get_yes() even is.
# Display complex results for expert to review. Then:
get_yes( "Do the above results meet all requirements?" );
Test::Smart::Interface::File was very uninformative, of course.
Exporting a subroutine with a name as generic as initialize() seems a poor interface design. If you must stick with the exporting, then give the exported routine a name that at least hints at where it came from (and thus is less likely to conflict with the names of routines exported by other modules). But it seems more natural to avoid exporting (a good thing to avoid in general) and replace:
initialize( "Test::Smart::Interface::Subclass", @args );
with
Test::Smart::Interface::Subclass->initialize( @args );
even if you hadn't intended for someone to subclass in a way that overrides your initialization code and even if initialize() wasn't originally part of the subclass interface (just have the base class delegate it to the factory class). Yes, the user would then "require Test::Smart::Interface::Subclass;" as well as "use Test::Smart".
Commenting on the code is painful since trying to even find it when hidden between chunks of POD much less read it while having to skip over big chunks of POD over and over just because you want to glance at a different subroutine... Ugh.
I did manage to notice (because the strange interface made me wonder) that you have a single global (worse, actually, a file-scope lexical) so your module design is quite inflexible, not allowing for more than one interface to be used in any given test script.
The documentation seems to imply that initialize(), get_yes(), and get_no() are exported even if you list things to be exported and don't list those. I hope that isn't the case and this is just something to be improved in the documentation. I should be allowed to avoid exporting things that I don't want exported (perhaps I don't use them, perhaps I choose to use them with a fully-qualified name). Even more likely, I should be allowed to list everything that is exported so someone reading my code and searching for what the heck initialize() does, will find that name listed on the "use Test::Smart" line and thus know where to go read up on it.
The design feels very much like it came out of the "Now, which classes should I have inherit from which other classes?" school of design. I get the impression that this school of design comes out of (bad, IMO) newbie OO tutorials and experienced OO designers tend to be more of the "Inheritance of more than interfaces is usually best to avoid" school. And, IMHO, inheritance of interfaces in Perl is mostly just silly (since Perl doesn't really have interfaces). But that just qualifies as "design smell" rather than something specific I can point to and say "change this".
1 But I'm not sure why I'm suggesting you change the name, since experience shows that you'll just keep the same name no matter how many people reasonably point out that the name has problems.
| 