something I had no time to mention to your previous post is http://prepan.org/prepan.org even if it is not heavily used nowadays is a good place to ask for comments. Put you module there, or the intended usage and ask also for the best name as you already have done here.

About the module: I like Carp and returning errors and warning from the user point of view: The end user like to know where the error occured in their code, rather than in your module. You can improve the error checking in your module.

For your convenience you can use a debug flag: I use something like: our $debug = $ENV{MY_MODULE_DEBUG} // 0 so that I can leverage it from outside the module. Then you can fill in your module of debugging statements: print "Ouch because damn!" if $debug; (PS: our is there so that this can be set by evntual submodules ).

Then you must add a pod section if you have not. perlpod is the place where to start.

And, wait.. no tests? :) You must test it against valid and invalid input to see it behaves as you expect. Then document well what is the normal behaviour. If you are totally new to test see my (I'm not the king of test anyway ;) step-by-step-tutorial-on-perl-module-creation-with-tests-and-git or at least its resource part.

good luck!

L*

Thanks for the suggestions. I'll add an update with my tests!

Update: I'll look into using carp. Archive::Zip gave me a lot of those kinds of warnings. You are right that I wanted to see where the errors were in my code not in Archive::Zip. Thanks again!

Hello Lotus1,

> I wanted to see where the errors were in my code not in Archive::Zip

No is not what I intended to mean. With Carp the user of your module sees the error from his perspective, like: Ouch file does not exists at consumer_script.pl line 42 and this is generally more useful.

L*

There are no rules, there are no thumbs..
Reinvent the wheel, then learn The Wheel; may be one day you reinvent one of THE WHEELS.

Hi,

Quick observation: I don't think I would return a reference to a string from extract_json_from_lqm -- it complicates the flow down the chain (having to dereference a ref to a scalar). Just return and pass around the string.

Hope this helps!

Thanks this helps. I will get rid of the ref for the string. My original module had a function to convert memos to Simplenote format but I decided to simplify down to the bare bones for my first attempt.

Remove things like this:

###############################################
#       Decode json file contents and
#         return the text in 'DescRaw'
[download]

If the name of the subroutine does not accurately describe what the sub does, come up with a better name. Beyond that, you can put a description in the documentation. Don't clutter code with comments unless absolutely necessary.

Speaking of documentation, where is it? How about unit tests?

Why are you passing around a reference to a scalar for a string? Why not just work with the string? Scalar refs aren't that normal and can lead to easy to miss subtleties.

Put new lines around return statements. You should easily be able to see where exit paths are in code:

From:

    my $ref_json_str = extract_json_from_lqm( $lqm_file );
    return '' if not $ref_json_str;
    my ($extracted_text, $note_category) = extract_text_from_json($ref
+_json_str);
    my $header = "Created date: $note_created_time\n";
    $header .= "Category:   $note_category\n";
    $header .= "-"x79 . "\n";
    $header = '' if $suppress_header;
    return $header . $extracted_text;
[download]

To:

    my $ref_json_str = extract_json_from_lqm( $lqm_file );

    return '' if not $ref_json_str;

    my ($extracted_text, $note_category) = extract_text_from_json($ref
+_json_str);
    my $header = "Created date: $note_created_time\n";
    $header .= "Category:   $note_category\n";
    $header .= "-"x79 . "\n";
    $header = '' if $suppress_header;

    return $header . $extracted_text;
[download]

You're not checking the value of $note_created_time before using it in a string. It very well could be empty.

I probably mentioned tests already, but it's worth repeating. Write tests. Learn about Devel::Cover. This is a very, very simple distribution so 100% coverage or at minimum extremely close to it should be trivial.

Is this in a version control system?

This is a very, very simple distribution so 100% coverage or at minimum extremely close to it should be trivial.

Part of the problem I had with writing tests was that it is difficult to trigger some of the Archive::Zip errors. To test for read errors I used a hex editor to garble the JSON file in the zip archive and was able to trigger that one. To me that seems like an unlikely edge case but I'll continue looking at other modules for more examples of what to test. It's such a small module there aren't a lot of things to test. Testing warnings and carp is something I just recently found so I was happy and proud to get that to work. I found myself bulletproofing the code as I wrote the tests and thought of things that could go wrong.

Thanks for all the suggestions so far!

"I have been enjoying refactoring and letting the tests help me find the errors."

Don't get complacent. You have barely a minimal amount of tests there. You're not covering any edge cases. You're doing the smallest amount of testing possible. It's definitely better than nothing, but there's room for improvement.

Every single time you make a change to your code, you should have a test case for it. Hell, write the test before you write the code. The test should fail before you make the change, then succeed after you've changed code. All other tests should still pass.

You want your test suite to test every single unimaginable scenario that your weird ass users can throw at it. Stuff you can't even imagine. Guess what... users do these kinds of things, trust me.

You ask the user for a banana as a function parameter, and they throw in human body parts. Be prepared for these things, because your code may be allergic to human flesh, and a crash could cause invalid data to be returned.

I read somewhere that the first step in software engineering is to research to make sure there isn't already a solution available. I did that step but not well enough. I just found a Youtube video that someone made two years ago with a Java solution that does the same thing as my module. It's better than my solution since all you have to do is download a jar file and double-click it. No programming required.

https://github.com/powerpoint45/QuickMemo-lqm-to-txt-File-Converter

At least my node can help people find that and it has been a good learning experience for me.

On the other hand, if someone wants to do the conversion as part of a larger task having a module there to do the heavy lifting can be a great boon. On top of that there is the immediate benefit you get from writing the code and getting it into shape for submission to CPAN, and the possibility that you can use it as part of your code portfolio to aid job hunting.

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

Thanks for the kind words. I have learned a lot. The next time I write a module it will be much easier to make it ready for CPAN. Using 'prove -l' has been a big help since I no longer have to run my (non-automated) tests manually and check everything visually. This is also my first full use of Git for a project and it has finally clicked in my mind how it works. Having this in my Github profile would be a great start for a code portfolio.