[chriswarbo]

I'm generally in favour of introducing applicatives, but I don't think the code in the article is a good example:

    (bid, now) <- liftIO $ do
        b <- randomIO
        n <- getCurrentTime
        return (b, n)

    (bid, now) <- liftIO $ (,)
        <$> randomIO
        <*> getCurrentTime

The applicative code could be considered better, since it avoids one-shot variables. However, that's not the biggest smell with this code. To me, the problems are:

- Constructing-then-destructing a pair; is it really necessary?

- Use of `$`; is there a way to avoid having to alter the precedence?

- Combining two seemingly unrelated actions into one; `randomIO` should be completely uncorrelated to the current time, so why put them in the same action?

- Potential laziness issues; it looks like we're using the `(bid, now)` combination to ensure the two IO actions are executed together, but will they be? WHNF will give us `(_ , _)`; if we force `bid`, will `now` also be forced?

Not saying I have answers to these, but I would say those are more "smelly" than the do-notation with a return

[tome]

Code review 4 lines: hundreds of comments

Code review 1000 lines: "Looks fine"

[chriswarbo]

Well, my main comment is that the most pungent smells in this code depend on its context and what it's trying to achieve. If I encountered it, I would immediately look at the surrounding context and usage to see if it's a reasonable approach to the problem.

Without that context, all that can be done are superficial changes like monad/applicative; which in this case are very minor. In other words, the original code wasn't a bad approach to the problem; but is it solving the right problem?

[reagency]

How would you write it? Creating a pair does seem weird, unless those two values are going to be passed around through multiple method calls together for use in multiple locations.

[chriswarbo]

> How would you write it?

As I said, I don't know; I'd have to see what the context is. As I've written in another comment, these few lines don't seem worth "fixing"; they're not bad. Yet they might be part of some larger arrangement that's overly complex, redundant, etc.

> Creating a pair does seem weird, unless those two values are going to be passed around through multiple method calls together for use in multiple locations.

Yet the pair is destructed immediately into two variables `bid` and `now`; if we want to pass a pair around, we'd need to create a new one using `(bid, now)`, just as if they were created separately.

If we need to use the pair, we should keep it; eg. `bidNow = liftIO $ ...`.