Brett Porter wrote:
Looks good to me. I say roll it into trunk.
A couple of uncertainties:
- isn't doFinal required?
doFinal ?
- I think it can use more tests
Agreed. Have any special use cases you want fleshed out?
Here's some nitpicking that might reduce the amount of code and make
it clearer:
- rename Hash to ChecksumAlgorithm (might as well be consistent and
use checksum everywhere instead of alternating through Hash, Digest,
Checksum?)
- likewise Hasher -> Checksum
Name changes make sense, I'll make these changes this weekend.
- there some duplicated code in isValidChecksums and fixChecksums that
can be factored out
Good catch, I'll put that on my todo's too.
- what about passing referenceFile as a final field, since every
method uses it?
I started ChecksumFile as a utility class, but that might not be the
best decision.
Any opinions if I make ChecksumFile an instantiated class?
ChecksumFile cf = new ChecksumFile(new File("random.jar"));
if( cf.valid() == false ) {
cf.createChecksum( ChecksumAlgorithm.SHA1 );
}
wdyt?
- but if not, it'd be better if the arg order of getChecksumFile and
createChecksum and calculateChecksum were consistent (I would go with
File first)
- I'd rename ChecksumFile as ChecksummedFile
- parseChecksum can be private?
I left that as public just to have it be easily unit tested. ;-)
- the two isValidChecksum(s) methods do different things.
isValidChecksum should be isValidChecksum(Hash hash) { return
isValidChecksums( new Hash[] { hash } ); }. Maybe another static
convenience method is needed for the checksum file checkChecksumFile(
File checksumFile ).
Legitimate fix. Adding to my todo.
- Brett
On 09/04/2008, at 2:39 PM, Joakim Erdfelt wrote:
I've been taking a stab and removing some of our dependencies on
various plexus components.
First up, plexus-digest.
I've taken the varied code from many locations and come up with a
stand alone archiva-checksum lib/component that I hope to be able to
integrate into archiva/trunk.
So ... uhm ... consider this a call for discussion. ;-)
- Joakim
--
Brett Porter
[EMAIL PROTECTED]
http://blogs.exist.com/bporter/
