Beefy Boxes and Bandwidth Generously Provided by pair Networks
No such thing as a small change
 
PerlMonks  

Re: Critique requested for module code - QuickMemo+ reader

by stevieb (Canon)
on Jan 24, 2021 at 18:22 UTC ( #11127384=note: print w/replies, xml ) Need Help??


in reply to Critique requested for module code - QuickMemo+ reader

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?

Replies are listed 'Best First'.
Re^2: Critique requested for module code - QuickMemo+ reader
by Lotus1 (Vicar) on Jan 25, 2021 at 00:58 UTC
    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!

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: note [id://11127384]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others imbibing at the Monastery: (2)
As of 2021-12-05 23:32 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?
    R or B?



    Results (31 votes). Check out past polls.

    Notices?