closed review MD5 Hash (MD5 Package)

[Ton Plomp]

I think that an output name of 'MD5 Hash?' is better, if that's the case we should change the name of the function to 'Is an MD5 hash?'

Ton

[jgcode]

I think that an output name of 'MD5 Hash?' is better, if that's the case we should change the name of the function to 'Is an MD5 hash?'

I have taken this on board however, I checked with other similarly named LabVIEW primitives in e.g. Comparison palette - there is no is/an prefixing (hence I renamed the VIs).

Also added a noted about Hexadecimal String as I will be updating the MD5 Digest with a Hexadecimal String output as previously requested to compliment this VI.

Thoughts?

MD5 Hash.vi

TEST - MD5 Hash.vi

Code is in LabVIEW 2009.

[jgcode]

I think we have some cool code here, so this review is pending.

If there are no further comments in a few days I will close the review.

Thanks for everyone's input!

[Val Brown]

This is very cool - both the code as last posted but also the overall process of review demonstrated in this thread.. I'm looking forward to (now) having more time (and energy) to participate in these kinds of discussions. Kudos to all involved!

[Ton Plomp]

I've got on (little) thing:

This shows that the primary input (the string) is not in the center of the Left hand Side of the Icon.

For programming style where this is in a wire, this gives odd layouts.

All of the basic LabVIEW =xxxx (ref, 0, >0, >=0, <0, <=0, NaN etc) the primary input is inline with the primary output.

The optional input (strict Character case) can be connected at te bottom or top (I prefer top).

Another (more major) point is the documentation:

Input String should be Hexadecimal String which is 32 characters.

I find this misleading. This functions tests if this is TRUE, it looks to me like I have to make sure the input string is a Valid MD5 string before using this function (creating a chicken-and-egg situation).

I would rewrite this to:

For a valid MD5 hash, the Input String should be 32 characters long and consists of the following characters....

.

Ton

Edited September 18, 2011 by Ton Plomp

[Phillip Brooks]

I've got on (little) thing:

This shows that the primary input (the string) is not in the center of the Left hand Side of the Icon.

For programming style where this is in a wire, this gives odd layouts.

All of the basic LabVIEW =xxxx (ref, 0, >0, >=0, <0, <=0, NaN etc) the primary input is inline with the primary output.

The optional input (strict Character case) can be connected at te bottom or top (I prefer top).

I was going to suggest the same thing, but after years and years of working for managers who all have their own style (beauty is in the eye of the beholder) I was going to let it go. As a natural extension of functions, I think it should be consistent with the other comparison palette functions (in line).

I would suggest the Boolean 'strict' option be shown similar to other NI functions such as Array of Strings to Path and labeled 'strict case'.

[jgcode]

I think it should be consistent with the other comparison palette functions (in line).

I would suggest the Boolean 'strict' option be shown similar to other NI functions such as Array of Strings to Path and labeled 'strict case'.

(Sorry, I am unsure from your post) Are you suggesting you want it like Ton has requested (same as comparison palette) or like the Array if String to Path image?

I was going to suggest the same thing, but after years and years of working for managers who all have their own style (beauty is in the eye of the beholder) I was going to let it go.

Please don't (I did play around with inline too and other CP arrangements) - but this is the whole point of the review

[jgcode]

:This shows that the primary input (the string) is not in the center of the Left hand Side of the Icon.

For programming style where this is in a wire, this gives odd layouts.

I don't think it's odd esp given the input and output are different datatypes.

I do agree it is not the standard comparison CP, but we have an additional argument input here.

Even if it was string in / string out if you check the first three String functions (top row) they don't match (offset vertically).

Here is an inline example.

Personally, I don't think this looks 100% right where the argument is - what do you guys think?

Also, it would helpful if you guys could post code of this VI with the CP's you want.

Cheers!

[Phillip Brooks]

Code is in LabVIEW 2009.

Personally, I don't think this looks 100% right where the argument is - what do you guys think?

Also, it would helpful if you guys could post code of this VI with the CP's you want.

It does look a bit odd.

Sorry, I'm on 8.6 with no time or resources to have 2009 around.

[Wouter]

Maybe the code is faster when the check on length is done first. If 32 do check if chars are correct, otherwise return...

[Darin]

The Strict Case? input looks fine to my eye, although it appears to be required now.

Clearly you are not digging too deeply into what makes a valid MD5 hash, but the current version will allow mixed cases with Strict Case = False. I do not see myself using a function like this, so I will merely point this out without further comment.

jgcode, on 19 September 2011 - 06:30 AM, said:

Even if it was string in / string out if you check the first three String functions (top row) they don't match (offset vertically).

Just because NI f'ed up bad with those functions does not mean we have to.

[jgcode]

The Strict Case? input looks fine to my eye, although it appears to be required now.

Yes, that is just an example (not uploaded to the repo) for discussion only (at the moment) - my LabVIEW options are set as CP Required (apologies for any confusion - I have edited the image).

So you like it in the middle of the Comparison icon?

Just because NI f'ed up bad with those functions does not mean we have to.

No I meant OpenG... Ha!

Clearly you are not digging too deeply into what makes a valid MD5 hash

Can you go into detail please?

Sorry, I'm on 8.6 with no time or resources to have 2009 around.

That's no dramas, I would want the code review to stay in LabVIEW 2009.

Anyways, you can easily contribute (if you want to) CP ideas from 8.6.

Here I got you started

Comparsion.vi

Code is in LabVIEW 8.6.1

[Darin]

So you like it in the middle of the Comparison icon?

Considering the alternatives, yes. I would be very tempted to make that connection optional.

No I meant OpenG... Ha!

Well then try not to let that happen again.

Can you go into detail please?

Upon closer inspection I see you use Match Pattern now, I saw the word Regex thrown around which to me means PCRE (Perl compatible..). The '$' character means different things between the two functions, with MP it should be fine.

[jgcode]

Considering the alternatives, yes. I would be very tempted to make that connection optional.

That was always an error (by me) - it should be optional.

Do you like the CP style (was my question tho)?

Well then try not to let that happen again.

lol

I saw the word Regex thrown around which to me means PCRE (Perl compatible..).

That was me (but it was too slow compared to MP).

[asbo]

Upon closer inspection I see you use Match Pattern now, I saw the word Regex thrown around which to me means PCRE (Perl compatible..). The '$' character means different things between the two functions, with MP it should be fine.

Match Pattern uses regular expressions and in PCRE $ is still the end-of-subject anchor. PCRE does have the option to mean EOL, but that's not the default behavior.

You did raise a good point about check the length first (I thought that it should be faster) but it seems the original VI wins out by a tiny margin. Weird.

MD5 Hash Benchmark.zip

[jgcode]

You did raise a good point about check the length first (I thought that it should be faster) but it seems the original VI wins out by a tiny margin. Weird.

I guess it would only win if its invalid?

[asbo]

I guess it would only win if its invalid?

Yeah (sub 100 measurements), which might validate using that method. I was hoping the compiler would make the difference completely moot, but I guess it's pretty dang close. Subroutine priority gets them even closer together, so it's probably a good tradeoff.

[Phillip Brooks]

On my OpenG Comparison Tools palette, there is one item (Data Changed).

I would suggest that since there are no 'typical' nodes that we could standardize on a the larger 'Select' triangle along with a 5-3-3-5 pattern.

Rationale:

OpenG comparison functions will be extensions of existing functions and require additional inputs/outputs.

5-3-3-5 allows us more choices for the inputs and outputs.

The larger node provides additional room for text inside and differentiates the node from the native node.

Comparsion.vi

We could also make it an Express VI

Edited September 22, 2011 by Phillip Brooks

[jgcode]

On my OpenG Comparison Tools palette, there is one item (Data Changed).

I would suggest that since there are no 'typical' nodes that we could standardize on a the larger 'Select' triangle along with a 5-3-3-5 pattern.

Rationale:

OpenG comparison functions will be extensions of existing functions and require additional inputs/outputs.

5-3-3-5 allows us more choices for the inputs and outputs.

The larger node provides additional room for text inside and differentiates the node from the native node.

Comparsion.vi

We could also make it an Express VI

There are some good reasons there - I looked at the Select Case node type too when reviewing what to use.

I would argue that a NI comparison is more familiar to users than the Select Case - and we are still doing a comparison but there are more inputs.

What are others take on this?

[jgcode]

The OpenG Board has decided to use on the following icon and connector pane:

Additionally (as suggested in this thread) a Hexademical String output has been added to MD5 Message Digest.

This could have been implemented in two ways:

1. Depreciated old VI, create new VI and add additional output
   
2. Convert to Polymorphic API
   

The Polymorphic API was chosen as it allows for future support of additional outputs (if ever needed).

Note: It will not break existing code.

The Hexadecimal String is just a thin wrapper around the original VI:

I will close this review in the next few days.

Thanks for everyone's input!

[Ton Plomp]

Two suggestions:

1. use the Icon of the polymorphic instance instead of the icon of the polymorphic VI.
   
2. Add a to-lower-case in the Hexidecimal VI, LabVIEW formats HEX with UPPERcase, while we've seen that the official standard is lowercase
   

Ton

[jgcode]

Two suggestions:

1. use the Icon of the polymorphic instance instead of the icon of the polymorphic VI.
   
2. Add a to-lower-case in the Hexidecimal VI, LabVIEW formats HEX with UPPERcase, while we've seen that the official standard is lowercase
   

Ton

Good suggestions Ton.

[jgcode]

You can track the new MD5 Hash VI here: ID: 3410441

You can track changes to the MD5 Message Digested here: ID: 3410442