5
\$\begingroup\$

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?
\$\endgroup\$
3
\$\begingroup\$

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)
\$\endgroup\$
  • \$\begingroup\$ Consider (group-by #(select-keys % [:urgent? :important?]) tasks)). I'd be forever forgetting the order in the vector. \$\endgroup\$ – Thumbnail Sep 26 '16 at 9:31
  • \$\begingroup\$ ::box-key defines this, but extracting a box-key function would help. \$\endgroup\$ – Sulik Szabolcs Sep 27 '16 at 15:53
  • \$\begingroup\$ I think my suggestion resolves the problem you mention of " inconsistent box-keys and tasks". \$\endgroup\$ – Thumbnail Sep 28 '16 at 0:53
  • \$\begingroup\$ 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. \$\endgroup\$ – Sulik Szabolcs Sep 29 '16 at 11:35
  • \$\begingroup\$ 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. \$\endgroup\$ – Thumbnail Sep 29 '16 at 16:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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