Critique requested for module code - QuickMemo+ reader

[Lotus1]

While I'm still thinking over the excellent suggestions I received for the name for the module here is the code. Please help with suggestions or critiques.

The module extracts the memo text from the lqm files that QuickMemo+ exports. The lqm file is actually a zip file that contains a JSON file that contains the memo text so it was an easy module to write. Each memo is in a separate archive so it is very tedious to extract each memo by hand.

package Data::QuickMemoPlus::Reader;
use 5.008001;
use strict;
use warnings;
use JSON;
use Archive::Zip qw( :ERROR_CODES :CONSTANTS );

our $VERSION = "0.01";

use Exporter qw(import);
 
our @EXPORT_OK = qw( lqm_to_str );
our $suppress_header = 0;

sub lqm_to_str {
    ## pass an lqm file exported from QuickMemo+
    my ( $lqm_file ) = @_;
    if (not -f $lqm_file){
        warn "$lqm_file is not a file";
        return '';
    }
    my $note_created_time = "";
    if ( $lqm_file =~ /(QuickMemo\+_(\d{6}_\d{6})(\(\d+\))?)/i) {
        $note_created_time = $2;
    }
    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;
}

#####################################
#         Unzip jlqm file and 
#           return json file contents.
#
sub extract_json_from_lqm {
    #   unzip
    #   extract the memoinfo.jlqm file.
    my $lqm_file = shift;
    # Read a Zip file
    my $lqm_zip = Archive::Zip->new();
    unless ( $lqm_zip->read( $lqm_file ) == AZ_OK ) {
        warn "Error reading $lqm_file";
        ####### to do: add the zip error to the warning?
        return "";
    }
    my $jlqm_filename = "memoinfo.jlqm";
    my $member = $lqm_zip->memberNamed( $jlqm_filename );
    ############### to do: add warning here if memoinfo.jlqm is missin
+g.
    if( not $member ){
        warn "File not found: $jlqm_filename in archive $lqm_file";
        return "";
    }
    my ( $string, $status ) = $member->contents();
    if(not $status == AZ_OK){
        warn "Error extracting $jlqm_filename from $lqm_file : Status 
+= $status";
        return "";
    }
    return \$string;
}

###############################################
#       Decode json file contents and
#         return the text in 'DescRaw'
sub extract_text_from_json {
    my $ref_json_string = shift;
    
    ############# To do: eval this and trap errors.
    my $href_memo  = decode_json $$ref_json_string;
    if (not $href_memo){
        warn "Error decoding text";
        return '','';
    }
    my $text = "";
    foreach( @{$href_memo->{MemoObjectList}} ) {
        $text .= $_->{DescRaw};
        $text .= "\n";
    }
    my $category = $href_memo->{Category}->{CategoryName};
    $category //= '';
    $category =~ s/[^\w-]/_/;
    return $text, $category;
}
1;
__END__

Update: I neglected to add my tests as Discipulus wisely pointed out. Since I have prove -l working now with my tests I have been enjoying refactoring and letting the tests help me find the errors.

00_compile.t

use strict;
use Test::More 0.98;

use_ok $_ for qw(
    LG::QuickMemo_Plus::Extract::Memo
);

done_testing;

01_warnings.t

use strict;
 
use Test::More tests => 5;
use Test::Warn;
 
use LG::QuickMemo_Plus::Extract::Memo qw( lqm_to_str );
 
my $lqm_file = 'not_a_file';
warning_is    {lqm_to_str($lqm_file)} "$lqm_file is not a file", "warn
+ing for missing file.";
$lqm_file = 't/data/good_file.lqm';
warning_is    {lqm_to_str($lqm_file)} [], "no warnings for good file."
+;

$lqm_file = 't/data/junk.lqm';
warnings_like {lqm_to_str($lqm_file)}
              [ {carped => qr/format error:/i},
                qr/Error reading $lqm_file/,
              ],
              "warnings for non-zip, junk file.";
$lqm_file = 't/data/missing_jlqm.lqm';
warnings_like {lqm_to_str($lqm_file)}
              [ qr/File not found: memoinfo.jlqm in archive $lqm_file/
+,
              ],
              "warnings for archive missing file - memoinfo.jlqm.";
$lqm_file = 't/data/garbled.lqm';
warnings_like {lqm_to_str($lqm_file)}
              [ {carped => qr/error: inflate error data error/i},
                qr/Error extracting memoinfo.jlqm from $lqm_file/,
              ],
              "warnings for garbled file - memoinfo.jlqm.";

02_main.t

use warnings;
use strict;
use Test::More tests => 10;
 
use LG::QuickMemo_Plus::Extract::Memo qw( lqm_to_str );

BEGIN {
    unshift @INC, 't/lib';
}

use_ok 'ExampleMemo';
can_ok 'LG::QuickMemo_Plus::Extract::Memo', 'lqm_to_str';
can_ok 'ExampleMemo', 'memo_with_header';
can_ok 'ExampleMemo', 'memo_with_header_no_timestamp';
can_ok 'ExampleMemo', 'memo_no_header';
can_ok 'ExampleMemo', 'jlqm';


my $lqm_file = 't/data/QuickMemo+_191208_220400(5).lqm';
is  ( lqm_to_str($lqm_file), ExampleMemo::memo_with_header(), 'memo wi
+th full header' );

$lqm_file = 't/data/good_file.lqm';
is  ( lqm_to_str($lqm_file), ExampleMemo::memo_with_header_no_timestam
+p(), 'memo with header - missing timestamp' );

$LG::QuickMemo_Plus::Extract::Memo::suppress_header = 1;
is  ( lqm_to_str($lqm_file), ExampleMemo::memo_no_header(), 'memo with
+ no header' );

is  ( ${LG::QuickMemo_Plus::Extract::Memo::extract_json_from_lqm($lqm_
+file)}, ExampleMemo::jlqm(), 'jlqm json contents' );

[Discipulus]

Hello Lotus1,

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*

[Lotus1]

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!

[Discipulus]

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*

[1nickt]

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!

[Lotus1]

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.

[stevieb]

Remove things like this:

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

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;

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;

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?

[Lotus1]

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!

[stevieb]

"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.

[Lotus1]

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.

[GrandFather]

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.

[Lotus1]

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.