Tests confusing

[RobThree]

What would be the use of these tests?

it('should change length properly', function() {
  assert.strictEqual('0001ARYZ6S41', ulid.encodeTime(time, 12))
})

it('should truncate time if not enough length', function() {
  assert.strictEqual('ARYZ6S41', ulid.encodeTime(time, 8))
})

The specs clearly read:

Specification

...

...

 Timestamp          Randomness
  10 chars           16 chars
   48bits             80bits

...

Components

Timestamp

• 48 bit integer

...

Randomness

• 80 bits

...

So there would (should?) never be a need for any other length than 10? I see others implement the same test:

• fvilers/ulid.net
• imdario/go-ulid
• Lewiscowles1986/ulid
• Lewiscowles1986/jULID

Also the len argument to encodeRandom() and encodeTime() pop up in all other implementations; again: since these are fixed per the spec they don't make sense since they will/should never be called with any other argument.

Don't get me wrong, I understand completely that the (original?) intent probably is/was "you may want to 'vary' with, for example 12 chars timestamp and 14 chars randomness" but then it wouldn't be a ulid anymore, would it? Then it (and a ulid) would simply be a base32 encoded representation of 128 bits of data.

I propose the referred tests are dropped, the arguments ditto and the lengths either hardcoded in their respective methods, or, for readability be put in some sort of const (like const TIMEPARTLENGTH = 10, RANDOMPARTLENGTH = 16).

If a ulid is to become as widespread as UUID/GUID's then, IMHO, it would be better to start off with people not 'tweaking' their ulid's to their need causing a 'proliferation' of all sorts of 'sorta compatible' ulids.

But that's just my $0.02

[darccio]

Just to add to the discussion: I found it strange too but I simply ported it as there would be no way to confirm my port implemented properly the algorithm (as done by @alizain).

Anyway, the algorithm looks like a proof-of-concept. Binary implementation should be the "production" one, IMO.

I agree ULID must settle to fixed lengths to avoid your scenario.

[alizain]

I understand completely that the (original?) intent probably is/was "you may want to 'vary' with, for example 12 chars timestamp and 14 chars randomness"

@RobThree Yes that's true. I was trying to figure out the number of bits to allocate to the timestamp to ensure we don't run into a Y2K problem whilst keeping the entropy high enough 😛

Binary implementation should be the "production" one, IMO.

@imdario Agreed

Also the len argument to encodeRandom() and encodeTime() pop up in all other implementations; again: since these are fixed per the spec they don't make sense since they will/should never be called with any other argument.

@RobThree While I do understand, it makes me somewhat more comfortable in the correctness of the implementation if I can change the length and get the expected output, purely from a testability perspective.

That being said, I have moved the length declarations up top as suggested!