A very simple TODO app's domain model

Question

I am currently rewriting a todo app from java to clojure. This is my first "real" clojure project so I'm not sure that what I write is idiomatic clojure code. I have just finished rewriting the domain module and I'd like to know how can I improve on it to have something which is idiomatic.

I had some simple classes in my original java project:

• Box <-- Represents a Box in the EisenhowerMatrix
• BoxBlurbsProvider <-- provides name and description labels for a Box
• EisenhowerMatrix <-- represents the matrix itself
• Importance <-- enum holding the possible importance values
• Urgency <-- enum holding the possible urgency values
• Task <-- represents a Task in an EisenhowerMatrix Box

My original classes are these (note that I omitted documentation from my code):

@Getter
@Builder
public class Box {
 private final Urgency urgency;
 private final Importance importance;
 private final String name;
 private final String description;
 private final Set<Task> tasks = new LinkedHashSet<>();
 public void addTask(final Task task) {
 tasks.add(task);
 }
 public void removeTask(final Task task) {
 tasks.remove(task);
 }
}
public interface BoxBlurbsProvider {
 String getNameFor(Urgency urgency, Importance importance);
 String getDescriptionFor(Urgency urgency, Importance importance);
}
public class EisenhowerMatrix {
 @Value(staticConstructor = "of")
 private static class BoxKey {
 private final Urgency urgency;
 private final Importance importance;
 }
 private final Map<BoxKey, Box> boxes = new LinkedHashMap<>();
 @Getter
 private final String name;
 private EisenhowerMatrix(String name, BoxBlurbsProvider boxBlurbsProvider) {
 this.name = name;
 for (Importance importance : Importance.values()) {
 for (Urgency urgency : Urgency.values()) {
 boxes.put(BoxKey.of(urgency, importance), Box.builder()
 .importance(importance)
 .urgency(urgency)
 .name(boxBlurbsProvider.getNameFor(urgency, importance))
 .description(boxBlurbsProvider.getDescriptionFor(urgency, importance))
 .build());
 }
 }
 }
 public static EisenhowerMatrix of(String name, BoxBlurbsProvider boxBlurbsProvider) {
 return new EisenhowerMatrix(name, boxBlurbsProvider);
 }
 public List<Box> getBoxes() {
 return new ArrayList<>(boxes.values());
 }
 public void addTask(final Urgency urgency, final Importance importance, final Task task) {
 boxes.get(BoxKey.of(urgency, importance)).addTask(task);
 }
 public void removeTask(final Task task) {
 boxes.values().forEach(box -> box.removeTask(task));
 }
 public List<Task> getTasks() {
 return boxes.values().stream().flatMap(box -> box.getTasks().stream()).collect(Collectors.toList());
 }
}
public enum Importance {
 IMPORTANT, NOT_IMPORTANT
}
public enum Urgency {
 URGENT, NOT_URGENT
}
@Data
@RequiredArgsConstructor(staticName = "of")
public class Task {
 private final String name;
 private final String description;
}

My resulting clojure code is in two namespaces. One for records and one for protocols:

records.clj:

(ns eisentower.domain.records
 (:use [eisentower.domain.protocols]))
(def urgencies [:urgent :not-urgent])
(def importances [:important :not-important])
(defn- combine-keywords [& keywords]
 (keyword
 (reduce
 (fn [k0 k1] (str (name k0) (name k1)))
 keywords)))
(defrecord Box [urgency importance name description tasks]
 BoxOperations
 (add-task-to-box [box task]
 (update-in box [:tasks] conj task))
 (remove-task-from-box [box task]
 (assoc box
 :tasks
 (remove #(= (:name %) (:name task)) (:tasks box)))))
(defn- create-boxes []
 (for [u urgencies i importances]
 (->Box u i (get-name-for u i) (get-description-for u i) [])))
(defrecord Task [name description])
(defrecord EisenhowerMatrix [boxes name]
 MatrixOperations
 (get-tasks [matrix]
 (into [] (mapcat #(:tasks (val %)) boxes)))
 (get-boxes [matrix]
 (:boxes matrix))
 (add-task-to-matrix [matrix urgency importance task]
 (let [box-key (combine-keywords urgency importance)
 box (get-in matrix [:boxes box-key])]
 (assoc-in matrix [:boxes box-key] (add-task-to-box box task))))
 (remove-task-from-matrix [matrix task]
 (loop [box (:boxes matrix)]
 (if (contains? (:tasks box) task)
 (let [box-key (combine-keywords (:urgency box) (:importance box))]
 (assoc-in matrix [:boxes box-key] (remove-task-from-box box task)))))))
(defn create-matrix [name]
 (->EisenhowerMatrix
 (into {} (map
 (fn [box]
 {(combine-keywords (:urgency box) (:importance box)) box})
 (create-boxes)))
 name))

protocols.clj:

(ns eisentower.domain.protocols)
(defprotocol BoxBlurbOperations
 (get-name-for [urgency importance])
 (get-description-for [urgency importance]))
(defprotocol MatrixOperations
 (get-tasks [matrix])
 (get-boxes [matrix])
 (add-task-to-matrix [matrix urgency importance task])
 (remove-task-from-matrix [matrix task]))
(defprotocol BoxOperations
 (add-task-to-box [box task])
 (remove-task-from-box [box task]))

Some points I'm a bit puzzled with:

• Can it be less verbose?
• Can it be more readable?
• Can I clean it up somehow?
• Am I doing something wrong?
• Isn't it too OOPy?
• How can it be more idiomatic?

• Sulik Szabolcs: I assembled a "microblog" for you about this as a gist.
• Thumbnail: Since you've used LinkedHashMaps and LinkedHashSets in the Java, you might consider for the Clojure version amalloy/ordered sets and maps. They too maintain the insertion order of their contents.

Answer 1

I started to write the java example as one-to-one mapping to clojure, but shortly realized that it's not entirely possible. The main reason that the original design is not sound for me.

So I was starting to think about the core concept and that can be summed up in one domain entity: the task.

Let's create a task.

(defn create-task
 [name desc urgent? important?]
 {:name name, :description desc, :urgent? urgent?, :important? important?})

Of course, it's a bit sloppy, so let's specify it properly. I'm using clojure.spec for this.

(s/def ::name (s/and string?
 (comp not empty?)))
(s/def ::description string?)
(s/def ::urgent? boolean?)
(s/def ::important? boolean?)
(s/def ::task (s/keys :req-un [::name ::description ::urgent? ::important?]))
And let's specify the constructor function itself.
(s/fdef create-task
 :args (s/cat :name ::name, :description ::description, :urgent? ::urgent?, :important? ::important?)
 :ret ::task)

The box concept is not necessarily need any implementation. We are interested in the grouped tasks by boxes and this is what the group-by function is for.

(defn box-tasks
 [tasks]
 (group-by (fn [{:keys [urgent? important?]}] [urgent? important?]) tasks))
(s/def ::tasks (s/coll-of ::task))
(s/def ::box-key (s/tuple ::urgent? ::important?))
(s/def ::box-tasks (s/map-of ::box-key ::tasks))
(s/fdef box-tasks
 :args (s/cat :tasks ::tasks)
 :ret ::box-tasks)

Side note: the ::box-tasks definition is odd, the default generator will generate inconsistent box-keys and tasks for it.

If you really need a matrix, you can define a simple constructor function for it.

(defn create-matrix
 [name]
 {:name name, :tasks #{}})
(s/def ::matrix (s/keys :req-un [::name ::tasks]))
(s/fdef create-matrix
 :args (s/cat :name ::name)
 :ret ::matrix)

and some basic operations

(defn add-task [m t] (update m :tasks conj t))
(defn remove-task [m t] (update m :tasks disj t))
(s/fdef add-task
 :args (s/cat :matrix ::matrix :task ::task)
 :ret ::matrix)
(s/fdef remove-task
 :args (s/cat :matrix ::matrix :task ::task)
 :ret ::matrix)

• Thumbnail: Consider (group-by #(select-keys % [:urgent? :important?]) tasks)). I'd be forever forgetting the order in the vector.
• Sulik Szabolcs: ::box-key defines this, but extracting a box-key function would help.
• Thumbnail: I think my suggestion resolves the problem you mention of " inconsistent box-keys and tasks".
• Sulik Szabolcs: Nope. It doesn't matter how you create your keys as far as the spec has no explicit knowledge about the connection between ::box-key and the related ::tasks described in the ::box-tasks spec. I have no idea it is even possible to describe this situation with spec or your only chance is to simple define a custom generator for it.
• Thumbnail: I misunderstood what you meant by " inconsistent box-keys and tasks". And I thought, wrongly, that (s/fdef box-tasks ... :ret ::box-tasks) was invalid: I had misread the ::box-key spec. I was able to use the specs to discover my error.