Package manager in Clojure

I wrote a package manager in clojure that does 5 things:

-
depend a b //creates a and b (if they don't exist) and adds dependency on a

-
install a //installs a and its dependencies

-
list //prints out the install packages

-
sys //prints out all packages

-
remove or uninstall packages and dependencies that are no longer needed.

I did this as an exercise to learn Clojure. I'm not sure if this is idiomatic are not. I tried to avoid typedef since that seems like attempting OOP on Clojure. I'm just wondering if there are any very obvious non idiomatic code or code smells.

```
(use '[clojure.string :only [split, lower-case]])
(def DEPEND "depend")
(def INSTALL "install")
(def REMOVE "remove")
(def UNINSTALL "uninstall")
(def SYS "sys")
(def LIST "list")
(def INFO "info")
(def EXIT "exit")
(def END "end")
(def all-packages (atom {}))
(def installed-packages (atom (sorted-set)))

(defn in? [seq elm]
(some #(= elm %) seq))

(defn create
"makes a package element with provided name, option list of providers
are packages requried by package, optional list of clients are packages using
package"
([name]
(create name #{} #{}))
([name providers clients]
{:name name, :providers providers, :clients clients}))

(defn add-client
[package client]
(create (:name package) (:providers package) (conj (:clients package) client)))

(defn remove-client
[package client]
(create (:name package) (:providers package) (disj (:clients package) client)))

(defn add-provider
"add a provided to package"
([package] package)
([package provider]
(create (:name package) (conj (:providers package) provider) (:clients package)))
([package provider & more-providers]
(reduce add-provider package (cons provider more-providers))))

(defn get-providers [package]
(get (get @all-packages package) :providers ))

(defn get-clients [package]
(get (get @all-packages package) :clients ))

(defn get-package [package]
(get @all-packages package))

(defn exist

OK, bear with me. I got really into this, so I hope you don't mind that this is super long! :) Here are my thoughts.

Major things:

-
I would consider using refs instead of atoms to represent your package list. The difference is that refs are used for coordinated access to multiple entities, whereas atoms provide uncoordinated access to a single entity. Because you're working with two related lists, all-packages and installed-packages, it would be safer to use refs to represent them.

-
A simpler way to create a command-line utility like this is to make use of a CLI library. Clojure has a few good ones. See this question on Stack Overflow for a few good methods.

-
You used def inside of two function definitions, which is generally considered incorrect in Clojure. Usually you will only use def, defn, etc. on the top level of your program. When you're inside of a def, defn, etc., you should use let instead to do what you're trying to do. See below:

(defn not-needed? [package self-uninstall]
  (let [clients (if self-uninstall
                  (disj (get-clients package) package)
                  (get-clients package))]
    (empty? clients)))

 

(defn runprog []
  (println "starting")
  (reset! run true)
  (while (true? @run))
    (let [line (read-line)
          [command & args] (split line #"\s+")]
      ...

Notice, also, how I used destructuring to simplify command (first (split line #" +")), args (rest (split line #" +")) to just [command & args] (split line #"\s+"). Destructuring is a very powerful tool that can make your code more concise and easier to read.

Minor things:

-
For your in? function, you can simplify (some #(= elm %) seq) to (some #{elm} seq). The #{} reader is a shorthand notation for a set, and a set in Clojure can be used as a function that looks up its argument in the set and returns either the argument if it is found, or nil if it's not. Because any value that isn't false or nil is considered "truthy" in Clojure, that means you can use a set as a function that tells you whether or not an element is contained in a collection. By the way, I would name the argument in this function coll rather than seq, as seq already refers to the clojure.core/seq function.

-
You can simplify the (get (get ... form in your get-providers and get-clients functions by using get-in:

(defn get-providers [package]
  (get-in @all-packages [package :providers]))

(defn get-clients [package]
  (get-in @all-packages [package :clients]))

-
You can omit the do in your install-new function definition. Any time you're doing something like defining a function using defn, there is already an "implicit do":

(defn install-new [package]
  (println "\t installing" package)
  (swap! installed-packages conj package))

-
You have a few functions (install, not-needed? and uninstall) that take a parameter called either self-install or self-uninstall, which is expected to be either true or false. It would be more idiomatic to make these keyword arguments, like this:

(defn install [package & {:keys [self-install]}]
  ; the rest of the function would still be the same

(defn not-needed? [package & {:keys [self-uninstall]}]
  ; etc.

Then you would call the functions like this, for example:

(install "clementine" :self-install true)

It is a little more verbose, but I think it's more elegant and it makes it clearer what your code is doing.

-
Your uninstall function smells of OOP practices, in particular in the way that you have nested if structures. These aren't always bad form in Clojure, but generally it's better to find a more functional way of expressing the flow of your program. Consider using cond as an alternative:

(defn uninstall [package & {:keys [self-uninstall]}]
  (cond 
    (not (installed? package))
    (println "\t" package "is not installed.")

    (and (installed? package)
         (not (not-needed? package :self-uninstall self-uninstall)))
    (println "\t" package "is still needed.")

    :else
    (do
      (doseq [provider (get-providers package)]
        (update-sys (remove-client (get-package provider) package)))
      (uninstall-package package)
      (doseq [provider (filter #(not-needed? % :self-uninstall false)
                               (get-providers package))]
        (uninstall provider :self-uninstall false)))))

This is also longer than your code, but I find it clearer and easier to read.

Nitpicky things:

-
I would consider renaming your create function to package, simply because create sounds like a function that would change the state of something, like perhaps it would create a package within one of your package lists. I understand that that's not what your function does, but I feel like if you called it package instead, it would make it clearer that (package "foo" bar baz) just represents a package named "foo" with providers `

StackExchange Code Review Q#45788, answer score: 8