Added methods for parsing time ranges in blocks

[Tylerwbrown]

No description provided.

[jtnelson]

don’t use Long instead of long if possible. Java will auto box the primitives in cases where they need to be treated as an Object

In your unit test you should add revisions to the block instead of letting time elapse

Covers should return true if there is any overlap. Right now, I believe you check only returns true if the input range is completely within the bounds of the block. Maybe the name covers is misleading. Perhaps “overlaps” is a better indicator of what the method is returning

Personal style preference: id call the Params start and end

Is there any reason why the end is exclusive instead of inclusive?

[jtnelson]

Also please add javadoc

[Tylerwbrown]

@jtnelson when I used long Intellij warned that it could produce a null pointer exception, as longs can't be null. Is this something that should be ignored?

I figured it was supposed to cover the time range since we were calling it cover, I agree if the purpose is for it to overlap we should call it overlap. I'll change the name and the functionality.

The inclusive start/exclusive end is an idiom I saw in other java standard library functions (generally ranges). Was just following a pattern. If something else is preferred, we can change it. I know Haskell/Kotlin generally do inclusive/inclusive for ranges, and I actually prefer that.

Usually Java will have "range" and "rangeClosed", so the default will be inclusive/exclusive and the inclusive/inclusive is explicitly called out.

[Tylerwbrown]

I'll add a javadoc because that's what we've been doing, but in my view self documenting code is always superior.

For example, take this method header:

/**
     * Insert a revision for {@code key} as {@code value} in {@code locator} at
     * {@code version} into this Block.
     *
     * @param locator
     * @param key
     * @param value
     * @param version
     * @param type
     * @throws IllegalStateException if the Block is not mutable
     */
    public Revision<L, K, V> insert(L locator, K key, V value, long version,
            Action type) throws IllegalStateException

In the best case scenario, the params list in the docs is redundant. In the worst case scenario it's wrong and misleading.

On top of that, we could simply rename the function and add a bit of code to document just as much, with the benefit of more confidence.

public Revision<L, K, V> insertRevision(L locator, K key, V value,
         long version, Action type) throws IllegalStateException {
    if (blockIsNotMutable())
        throw new IllegalStateException();
    // Rest of code
}

[jtnelson]

The benefit of Javadoc is being able to highlight over a method in an IDE and get a summary of the method's contract (see example below)

Code should be self documenting, but consumers of a class/etc should have documentation so they don't have to dig into the class because the implementation details don't matter to the consumer.

[Tylerwbrown]

That's a good point, however I think if a method is doing so much that its name isn't clearly documenting what it's suppose to do, then it should be split up into multiple methods, rendering (IMO most if not all) descriptive documentation obsolete.

In the above, for example, instead of Transformers.transformSet, where transformSet really does transformSetIntoAnotherSetWithPossibleDataLoss (which would admittedly be far too verbose for a method name), it'd be better to encapsulate this behavior in shorter/more standard methods with "documentation" in the types.

For example, this code in our codebase:

Transformers.transformSet(mySet, obj -> obj.func());

would be more explicit in Kotlin with their std lib (which we could obviously mimic)

mySet.map { it.func() }.toSet()

this is because map works the same on every iterable, and that's showcased in the types. Or more specifically, map is on Iterable<T> and returns List<R>. toSet() then explicitly turns the list into a set, which removes duplicates (i.e the data loss).

Obviously our type systems aren't perfect yet, and they can't fully describe everything that English can, but that's what method names & type names are for. I'm sure there are some special scenarios where documentation can be helpful, but I think in the general scenario descriptive names + explicit types are better.

[jtnelson]

@Tylerwbrown this unit test still doesn't do anything. Please see my previous note about adding revisions to the block.

[jtnelson]

The logic of this method should return true if any part of the range is covered by any part of the Block's revision range. Currently it only returns true if the entire range is covered by the Block's range

[jtnelson]

As a style convention, please use braces for all if statements, even if the resulting block is just one line