for me: 3️⃣ this resembles the interceptor pattern: • have an aggregating data structure • have a number of functions that add stuff to the data structure, you could extract the checks in the let-block into 1st class functions • make a decision at the end of the chain

Reworded 3️⃣:

(defn bid
  [auction-storage, clock, auction-id, user-id]
  (let [auction (find auctions-storage auction-id)]
    (or (not-empty (cond-> []
                     (nil? auction),,,,,,,,,,,,,,,,,,,,,,, (conj :auction-not-found)
                     (not (open? auction)),,,,,,,,,,,,,,,, (conj :auction-not-open)
                     (not (user-can-bid? auction user-id)) (conj :bid-not-allowed)))
        (save auction-storage
              (-> auction
                  (assoc :current-bid {:user-id user-id
                                       :bidded-at (LocalDateTime/now clock)})
                  (update :price-in-cents inc))))))

I think it depends on the situation. I'd approve any of those in a code review given the right conditions. • 1 if: ◦ validations are expensive so you should short circuit ◦ validations make sense to be entwined with the code. • 2 if: ◦ Validations should be got out of the way up front ◦ You want to get all validations back. • 3 if ◦ Same as 2, but you like using lots of when clauses a lot. Usually I'm keen on separating validation from business logic when possible. I'm also keen on getting multiple failures back at once (so I can fix all in one go) - so something similar 2 is probably my preferred.

I would normally do something like this with cond:

(defn bid [auction-storage clock auction-id user-id]
  (let [auction (find auctions-storage auction-id)]
    (cond

      (nil? auction)
      :auction-not-found

      (not (open? auction))
      :auction-not-open

      (not (user-can-bid? auction user-id))
      :bid-not-allowed

      :else
      ;; business logic here
      ))

The amount of possibilities are endless! Eager to see more approaches 😌

So far, I enjoyed all suggestions.

If I'm reading it right, there are two parts, viz. 1. Deciding whether an auction is open, and 2. Executing an allowed bid. Only the latter seems to require effects (read from DB / write to DB). I'll probably separate out the effectful code from the purely functional choice-making.

(defn auction-status  [auction]
  (cond
    (empty? auction) :auction/not-found
    (not (open? auction)) :auction/not-open
    :else :auction/open))

(defn bid! [auction-storage auction-id user-id clock]
  (let [auction (find auctions-storage auction-id)]
    (if (and (= (auction-status auction) :auction/open)
             (user-can-bid? auction user-id))
      (save auction-storage ;; presumably returns a map of the saved auction
            (-> auction
                (update :price-in-cents inc)
                (assoc :current-bid {:user-id user-id
                                     :bid-at (LocalDateTime/now clock)})))
      :bid/not-allowed)))

I like your suggestion Adi! It even reduces the cognitive load, because if we don't care about the validation itself and only the result, we can safely read only "If we met this conditions, accept the bid and save it. Otherwise the bid is not allowed".

Yes, that. Although the value of pulling out a function is debatable. For that to be acceptable, the function should be generally useful. Otherwise, one can just let-bind auction-status in bid!. Further, if the state details (like ::auction/not-open) are not generally useful in the overall scheme of things (i.e. across the program), then the function can be just:

(defn bid! [auction-storage auction-id user-id clock]
  (let [auction (find auctions-storage auction-id)
        auction-open? (and (not-empty auction) (open? auction))
        bid-allowed? (and auction-open?
                          (user-can-bid? auction user-id))]
    (when bid-allowed?
      (save auction-storage ;; presumably returns a map of the saved acution
            (-> auction
                (update :price-in-cents inc)
                (assoc :current-bid {:user-id user-id
                                     :bid-at (LocalDateTime/now clock)}))))))

This was my favorite one! It is really clean. The only trade-off is not knowing why the bid didn't complete as we are swallowing the validations.

my previous example generalized (but not tested)

(defn validation-errors
  [entity rules]
  (->> rules 
       (into [] (keep (fn [[pred err]] 
                        (when-not (pred entity) err))))
       (not-empty)))

(def auction-rules-all
  [[nil?,,,,,,,,,,,,,,,,,,,,,, :auction-not-found]
   [(complement open?),,,,,,,, :auction-not-open]
   [(complement user-can-bid?) :bid-not-allowed]])

(def auction-rules-short-circuit
  [[nil?,,,,,,,,,,,,,,,,,,,,,, (reduced :auction-not-found)]
   [(complement open?),,,,,,,, (reduced :auction-not-open)]
   [(complement user-can-bid?) (reduced :bid-not-allowed)]])

(defn bid
  [auction-storage, clock, auction-id, user-id]
  (let [auction (find auctions-storage auction-id)]
    (or (validation-errors auction auction-rules-short-circuit)
        (save auction-storage
              (-> auction
                  (assoc :current-bid {:user-id user-id
                                       :bidded-at (LocalDateTime/now clock)})
                  (update :price-in-cents inc))))))

Probably the real rules should be like

(def auction-rules
  [[nil?,,,,,,,,,,,,, (reduced :auction-not-found)]
   [(complement open?),,,,,,,, :auction-not-open]
   [(complement user-can-bid?) :bid-not-allowed]])

> The only trade-off is not knowing why the bid didn't complete as we are swallowing the validations. Yes, as I said, it all depends on whether the information is material, e.g. must log validations for audit purposes, and/or must use validation data for some decision making / logic downstream of the bid function.

> Probably the real rules should be like @U0HJNJWJH I wrote a similar concept of functions-as-rules features in a Clojure workshop I teach :) https://github.com/adityaathalye/clojure-by-example/blob/master/src/clojure_by_example/ex03_data_and_functions.clj#L216

(def minimal-good-conditions
  "A collection of functions that tell us about the  good-ness of planetary conditions."
  [co2-tolerable?
   gravity-tolerable?
   surface-temp-tolerable?])

Something like this (untested)...

(defmulti auction!
 [{:keys [auction user-id] :as bid-data}]
  (cond
    (empty? auction) :auction/not-found
    (not (open? auction)) :auction/not-open
    (user-can-bid? auction user-id) :auction/open
    :else :auction/bid-not-allowed))

(defmethod auction! :auction/not-found
  [bid-data]
  (log audit-db auction-data))

(defmethod auction! :auction/not-open
  [bid-data]
  (log audit-db auction-data))

(defmethod auction! :auction/open
  [{:keys [auction auction-storage user-id clock] :as bid-data}]
  (save auction-storage
        (-> auction
            (update :price-in-cents inc)
            (assoc :current-bid {:user-id user-id
                                 :bid-at (LocalDateTime/now clock)}))))

(defmethod auction! :auction/bid-not-allowed
  [bid-data]
  (alert live-trading-monitor "Illegal bid." auction-data)
  (log audit-db auction-data))

(defmethod auction! :default
  [bid-data]
  (alert live-trading-monitor "Unknown bid payload." auction-data)
  (log audit-db auction-data))