Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Interested in a review of this code, in particular the (hopefully monadic) bind. I actually put this to good use. It was a nice complement to Guava's Optional, and shorter than:

Optional.fromNullable(x).or(y);

And I threw in a bind method, which I think lifts it up into monad space.

One question I wasn't sure of the the semantics of multiple calls. Should I carry along the original default value, or does each new value become the default? So in psuedo code, should

Either.with("A").or("B").or(null)

result in "A" (the original default) or "B"? I thought "B" made more sense.

I'm interested in if I got the monad signature & semantics right.

public class Either<A> {

    private final A value;

    /**
     * constructor sets the default & value the same
     * @param defaultValue
     */
    Either(A value) {
        if (value == null) {
            throw new IllegalArgumentException("Either cannot be null");
        }
        this.value = value;
    }

    /**
     * constructor sets the default & value the same
     * @param defaultValue
     */
    Either(A defaultValue, A value) {
        this.value = (value == null) ? defaultValue : value;
    }

    /**
     * the unit method lifts the value into the monadic space
     * 
     * @param value
     * @return an either with the value as default
     */
    public static <A> Either<A> with(A value) {
        return new Either<A>(value);
    }

    /**
     * create a new either using the previous default
     * 
     * @param value
     * @return
     */
    public Either<A> or(A value) {
        return new Either<A>(this.value, value);
    }

    /**
     * @return the value, or the default
     */
    public A get() {
        return value;
    }

    /**
     * completes this class as a monad
     * 
     * @param f
     * @return an either of the new type
     */
    public <B> Either<B> bind(Function<A, Either<B>> f) {
        return f.apply(value);
    }
}

And here are the unit tests for bind (and sample usage):

static class ToInt implements Function<String, Either<Integer>> {

    private final int defaultInt;

    /**
     * lol closures in java
     * 
     * @param defaultInt
     */
    public ToInt(int defaultInt) {
        this.defaultInt = defaultInt;
    }

    @Override
    public Either<Integer> apply(String s) {
        try {
            return Either.with(Integer.parseInt(s));
        }
        catch (NumberFormatException nfe) {
            return Either.with(defaultInt);
        }
    }
};

private static final Function<Integer, Either<Double>> SQRT = new Function<Integer, Either<Double>>() {
    @Override
    public Either<Double> apply(Integer i) {
        return Either.with(Math.sqrt(i));
    }
};

private static final int TEN_THOUSAND = 10000;
private final ToInt TO_INT = new ToInt(TEN_THOUSAND);

// ~=~=~=~=~=~=~=~=~=~=
// Tests


public void bindToIntEmptyEXPECTDefault() {
    // Arrange
    Either<String> a = Either.with("");

    // Act
    Either<Double> b = a.bind(TO_INT).bind(SQRT);

    // Assert
    assertEquals(b.get(), 100.0);
}

public void bindToIntFormatEXPECTDefault() {
    // Arrange
    Either<String> a = Either.with("zoom");

    // Act
    Either<Double> b = a.bind(TO_INT).bind(SQRT);

    // Assert
    assertEquals(b.get(), 100.0);
}

public void bindToIntEXPECTInt() {
    // Arrange
    Either<String> a = Either.with("144");

    // Act
    Either<Double> b = a.bind(TO_INT).bind(SQRT);

    // Assert
    assertEquals(b.get(), 12.0);
}
share|improve this question
1  
For what it's worth, in Haskell, the types for Either are Left and Right. –  cimmanon Feb 25 at 4:17
    
ack. Thank you. I've been really bad at translating the Haskell functionality over. Well, it's still proven to be useful, though not correct. I should rename it and take another crack at Either, sticking to the Either spec more closely. Oh, well. It's still funny -- I feel like I'm learning a lot from these patterns, even if it's not the intended lesson. –  Rob Y Feb 25 at 5:05
1  
I figured you got the A and B from the type signature, but overlooked the important part. When you see Either a b in Haskell, the return type is either Left a or Right b. In this case, a and b signal that the actual type doesn't matter. The fact that they're different letters indicates that they don't have to be the same type (but they can be). –  cimmanon Feb 25 at 13:29
    
@cimmanon I changed the class name to Default (I balked at naming it DefaultMonad as being too pretentious). The usage is Default.with("1").or("2").get(), and it supports bind :) But I have need now for an actual Either, so I may give it another go. Thank you for pointing that out! –  Rob Y Feb 25 at 23:08

2 Answers 2

up vote 6 down vote accepted

I have a nit-pick with the logic in your Either.or(A) method. You have:

public Either<A> or(A value) {
    return new Either<A>(this.value, value);
}

but, since Either is immutable (oh, should Either be a final class? ... I think so...), it makes sense that the or method can return the exact instance if it is valid.... why does it have to create a new instance each time? In a stream that can create a lot of GC churn....

I would write that method:

public Either<A> or(A value) {
    return value == null ? this : new Either<A>(value);
}

I know that essentially duplicates the constructor, but, doing it this way avoids the constructor in (hopefully) many places entirely.

This leads on to your question:

One question I wasn't sure of the the semantics of multiple calls. Should I carry along the original default value, or does each new value become the default? So in psuedo code, should

Either.with("A").or("B").or(null)

result in "A" (the original default) or "B"? I thought "B" made more sense.

If the or(A) method does not need to construct a new instance, then it makes sense to me that the semantics should return "A", since that allows skipping new instances for "B" and null.

share|improve this answer
1  
Good call! Fixed. This logic does preserve the "B" response semantics, but the way I'm actually using it is this: public static final Either<String> DEFAULT_STRING ... and then applying DEFAULT_STRING.or(s) over and over. Your fix will spare the creating of a bunch of duplicates. (If I did want to return "A" every time it sees null, I could carry a Either<A> defaultValue in addition to the value. Then or would return defaultEither or new Either... The first implementation actually carried defaultValue forward each time, but just as the plain type.) –  Rob Y Feb 25 at 0:28
    
actually, it's even better... I can get rid of the 2nd overloaded constructor altogether. That's an overload (which I don't like but find handy) and 3 unit tests that just disappear. So that's a big win. –  Rob Y Feb 25 at 0:46

Documentation

I don't know if you're using this class as some kind of api are not, but if you're providing documentation, make sure it's useful and clear.

/**
     * constructor sets the default & value the same
     * @param defaultValue
     */
    Either(A value) {
        if (value == null) {
            throw new IllegalArgumentException("Either cannot be null");
        }
        this.value = value;
    }    
/**
     * constructor sets the default & value the same
     * @param defaultValue
     */
    Either(A defaultValue, A value) {
        this.value = (value == null) ? defaultValue : value;
    }   

Two different constructor, same documentation, one of them does not even specify the second argument. You could add valuable information since you have documentation. In your first constructor, you could specify that you're throwing an IllegalArgumentException if value is null.

Documentation should help to know how to use the class, behavior that will bring exception and others sometimes some examples of uses.

Unit test

I'm glad to see that you have unit tests with good method names. The names don't respect camel-Case convention, but in general you know by the name what it's suppose to test.

I'm wondering why you don't have a more "standard" test class. I don't see the @Test annotation. Your method seems to be included within another class, and not on his own test class.

You don't need to always add comment about the structure of your test. If you keep the newlines between all three parts we can understand that you're doing the test in three steps : arrange, act, assert.

Naming

private static final Function<Integer, Either<Double>> SQRT;

This is more of an personal taste here, but I find SQRT very unreadable. I would find SQUARE_ROOT more readable in general. I don't like abbreviation, even commons one.

share|improve this answer
1  
Excellent comments, thank you! Sorry, I severely truncated the unit tests because the tests for "with" and "or" were pretty boring. This is just the "bind" coverage. I actually aggree (strongly!) with you about abbreviations; if SQRT were in the production code rather than tests, I'd absolutely spell it out. Hm. I think I will here, too. Thank you! –  Rob Y Feb 24 at 22:58
1  
@RobY I was wondering how do you run your tests? –  Marc-Andre Feb 25 at 2:41
    
I've been using testng lately. In eclipse, I just right-click at the package level and run them there, or else through the maven test cycle. I trimmed out almost everything in my code sample except the actual tests. Is that what you use? –  Rob Y Feb 25 at 4:54
    
@RobY It's exactly what I have. But as I said in my answer, you were missing the @Test annotation for JUnit (the most common library) so I was wondering if you were doing something different. –  Marc-Andre Feb 25 at 13:40
    
I don't find SQRT (square root) to be any different from INT (integer) in terms of readability, especially when a quick search will turn up nothing but results for all sorts of topics on square roots. –  cimmanon Feb 25 at 18:09

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.