Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rationalize computeHash and ComputeMD5 #333

Closed
jrwiebe opened this issue Jul 30, 2019 · 5 comments
Closed

Rationalize computeHash and ComputeMD5 #333

jrwiebe opened this issue Jul 30, 2019 · 5 comments

Comments

@jrwiebe
Copy link
Contributor

jrwiebe commented Jul 30, 2019

I suggested on Slack that we rationalize the naming of the matchbox.package method computeHash and the ComputeMD5 object. Or eliminate one. (I vote to keep the ComputeMD5 object.) I recently pushed a change that makes the output of these identical.

@ruebot @lintool

@ruebot
Copy link
Member

ruebot commented Jul 30, 2019

Is it just this For string data, it is better to use StringUtils.computeHash().? One is for bytes and one is for strings?

Does it matter performance-wise just picking one?

@jrwiebe
Copy link
Contributor Author

jrwiebe commented Jul 30, 2019

computeHash just converts the string to a byte array with s.getBytes.

@ruebot
Copy link
Member

ruebot commented Jul 30, 2019

@lintool any objection to getting rid of computeHash and replacing it with ComputeMD5? I'm happy to do the work if you're good with it.

@greebie
Copy link
Contributor

greebie commented Jul 30, 2019

I think I added computeHash as a quick (and hacky) way to match ids in networks before a better method was used in #289 (preferring .zipWithIndex). I cannot recall if we are still using the old way in any remaining functions -- I think maybe we deprecated them? Either way, I think it is a good job for a code reaper.

@ianmilligan1
Copy link
Member

I think it is a good job for a code reaper.

Agreed – and thanks so much for the context on this, @greebie! This all rings a bell now and a search of the code bears this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants