clojure - How to write idiomatic clojure (+ functional) code?

Question

I just started hacking around with Clojure, and although I adore the language, I can't understand how to do certain things idiomatically.

Writing a web-app using compojure, here's one of my controller actions:

(defn create [session params]
  (let [user (user/find-by-email (params :email))]
    (if user
        (if (user/authenticate user (params :password))
            (do (sign-in session user)
                (resp/redirect "/home?signed-in=true"))
            (resp/redirect "/?error=incorrect-password"))
        (let [new-user (user/create params)]
          (sign-in session new-user)
          (resp/redirect "/home?new-user=true")))))

I'm writing this in a very imperative way. Using so many lets/ifs/dos, I can't help but think I'm doing something very wrong. How would I write this functionally?

Here's the psuedocode for what I'm trying to do

look if user exists
  if user exists, try to sign user in using password provided
    if password is wrong, redirect to "/?error=incorrect-password"
    if password is correct, sign user in and redirect to "/home?signed-in=true"
  else create user, sign user in, and redirect to "/home?new-user=true"

Thanks so much!

Answer 1

There isn't anything non-functional about if - it's a perfectly good, purely functional construct that evaluates an expression conditionally. Too many ifs in one place might be a warning sign though, that you should be using a different construct (e.g. cond? polymorphism with protocols? multimethods? a composition of higher order functions?)

do is more tricky: if you have a do then it implies that you are doing something for a side effect, which is definitely non-functional. In your case sign-in and user/create appear to be the side-effectful culprits here.

What should you do about side-effects? Well they are sometimes necessary, so the challenge is how do your structure your code to ensure that side effects are controlled and manageable (ideally refactored out into a special state-handling danger zone so that the rest of your code can stay clean and purely functional).

In your case you might consider:

• Passing the "user/authenticate" function as part of your input (e.g. in some form of context map). This will allow you to pass in test authentication functions, for example, when you don't want to use the real database.
• Return a "successfully authenticated" flag as part of the output. This would then be caught by a higher level handler function that could be responsible for performing any side effects related to signing in.
• Alternatively return a "new user" flag as part of the output, that the handler function would likewise recognise and perform any required user setup.

Answer 2

Functional programming styles encourage using higher level functions like map, reduce and filter, also they force you to deal with immutable data structures most of the time. Nothing is wrong with your code so far, since you haven't broken any single rule in functional programming. However, you can improve your code a little bit, like combining let and if using if-let.

Answer 3

This mostly looks reasonable, and since your only side effects are "necessary", ie adding to the session, I wouldn't call it terribly imperative. There are three changes I would make, though the third one is optional:

• As another answer suggested, change the if/let pair into a single if-let.
• Use (:foo bar) for keyword lookups in a map, not (bar :foo). That's just the standard way to do it.
• Don't bother creating a local for new-user, since you only use it once; you can just inline it.

There's another change I wouldn't make, since I think it would reduce readability. However, that's a matter of style and judgment, so I'll mention it as something for you to think about. Notice how every branch of the if maze ends in a call to resp/redirect: you could pull all those calls out to the top-level. and then decide what arguments to pass to it. Combined with the other changes, it would look like:

(defn create [session params]
  (resp/redirect (if-let [user (user/find-by-email (:email params))]
                   (if (user/authenticate user (:password params))
                     (do (sign-in session user)
                         "/home?signed-in=true")
                     "/?error=incorrect-password")
                   (do (sign-in session (user/create params))
                       "/home?new-user=true"))))