search

OCFL / ocfl-java

A Java OCFL implementation
MIT License
16 stars 12 forks source link

misc cleanup #80

Closed pwinckles closed 1 year ago

pwinckles commented 1 year ago
  1. Updates all dependencies to their latest versions
  2. Precompiles a few regexes used for splitting
  3. Corrects my info

As far as I can think of right now, these are the last changes that I wanted to get in before making the 1.5.0 release.

pwinckles commented 1 year ago

Can one of you approve the db tests? Again, I'm not sure why it's wanting approval for these. I upgraded the drivers here, so it would be good to run them.

sprater commented 1 year ago

Will do

On Sat, Oct 8, 2022 at 6:59 PM Peter Winckles @.***> wrote:

Can one of you approve the db tests? Again, I'm not sure why it's wanting approval for these. I upgraded the drivers here, so it would be good to run them.

— Reply to this email directly, view it on GitHub https://github.com/UW-Madison-Library/ocfl-java/pull/80#issuecomment-1272415641, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAD5W55GYF4E43WLUWBK4DTWCIDFVANCNFSM6AAAAAARAOPEBU . You are receiving this because your review was requested.Message ID: @.***>

MormonJesus69420 commented 1 year ago

Hi, sorry to comment on a closed merge request, but I was wondering what's the reasoning behind changing the split:

var parts = sidecarContents.split("\\s"); // this
var parts = WHITESPACE.split(sidecarContents); // to this

Does it offer some performance benefit, better code safety, or just stylistic choice?

pwinckles commented 1 year ago

Hi, sorry to comment on a closed merge request, but I was wondering what's the reasoning behind changing the split:

var parts = sidecarContents.split("\\s"); // this
var parts = WHITESPACE.split(sidecarContents); // to this

Does it offer some performance benefit, better code safety, or just stylistic choice?

@MormonJesus69420, it is for performance. I perhaps should have put a limit on the split too, thinking about it again. You have to be careful with Java's String.split() and String.replaceAll(). They both take regex strings which they will compile to Patterns on every invocation. split() does inspect the regex to see if it can cheat an not compile it if it's just a split on a literal character, but replaceAll() does not have a similar optimization. So, long story short, if you want to split/replace on a regex, you should precomile the regex like this PR does, and if you want to replace string literals you should use String.replace(), which will still replace all matches -- you'd only ever want to use String.replaceAll() if you don't know the regex you're going to replace on before hand.