Package manager in Clojure

Question

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

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

2. install a //installs a and its dependencies

3. list //prints out the install packages

4. sys //prints out all packages

5. 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 exists? [package]
 (contains? @all-packages package))
(defn dependent? [first second]
 (if (in? (cons first (get-providers first)) second)
 (do (println (str "\t" first) "depends on" second) true)
 (some #(dependent? % second) (get-providers first))))
(defn update-sys [package]
 (swap! all-packages assoc (:name package) package))
(defn add-sys-package
 "adds a package to all-packages"
 [package & deps]
 (doseq [dep deps]
 (if-not (exists? dep) (update-sys (create dep))))
 (if (not-any? #(dependent? % package) deps)
 (update-sys (apply add-provider (cons (create package) deps)))
 (println "Ignoring command")))
(defn print-sys []
 (doseq [[k,v] @all-packages] (println "\t" v)))
(defn print-installed []
 (doseq [v @installed-packages] (println "\t" v)))
(defn installed? [package]
 (contains? @installed-packages package))
(defn install-new [package]
 (do (println "\t installing" package)
 (swap! installed-packages conj package)))
(defn install
 [package self-install]
 (if-not (exists? package) (add-sys-package package))
 (if-not (installed? package)
 (do (doseq [provider (get-providers package)] (if-not (installed? provider) (install provider false)))
 (doseq [provider (get-providers package)] (update-sys (add-client (get-package provider) package)))
 (install-new package))
 (do
 (if self-install (update-sys (add-client (get-package package) package)))
 (println "\t" package "is already installed."))))
(defn not-needed? [package self-uninstall]
 (def clients
 (if self-uninstall
 (disj (get-clients package) package)
 (get-clients package)))
 (empty? clients))
(defn uninstall-package [package]
 (println "\t uninstalling" package)
 (swap! installed-packages disj package))
(defn uninstall
 [package self-uninstall]
 (if (installed? package)
 (if (not-needed? package self-uninstall)
 (do (doseq [provider (get-providers package)] (update-sys (remove-client (get-package provider) package)))
 (uninstall-package package)
 (doseq [provider (filter #(not-needed? % false) (get-providers package))] (uninstall provider false)))
 (println "\t" package "is still needed"))
 (println "\t" package "is not installed")))
(def run (atom true))
(defn stop-run []
 (reset! run false))
(defn exit []
 (println "goodbye") (stop-run))
(defn runprog []
 (println "starting")
 (reset! run true)
 (while (true? @run)
 (def line (read-line))
 (def command (first (split line #" +")))
 (def args (rest (split line #" +")))
 (condp = (lower-case command)
 DEPEND (apply add-sys-package args)
 LIST (print-installed)
 INSTALL (install (first args) true)
 INFO (println (get-package (first args)))
 [REMOVE UNINSTALL] (uninstall (first args) true)
 UNINSTALL (uninstall (first args) true)
 SYS (print-sys)
 EXIT (exit)
 END (exit)
 ())))
(runprog)

Edit: After incorporating almost all suggestions github gist

Answer 1

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:

1. 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.

2. 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.

3. 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:

1. 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.

2. 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]))
   

3. 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))
   

4. 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.

5. 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:

1. 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 bar and clients baz. You do have a handful of functions that change the state of your package lists, so I think it pays to be careful about what you name your functions, so you can make it easier to know which functions will mutate the state of your lists, and which ones are pure functions.

2. While you're at it, you might consider making your package (a.k.a. create) function use keyword args too:

   (defn package
    "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 & {:keys [providers clients] 
    :or {:providers #{} :clients #{}}]
    {:name name, :providers providers, :clients clients})
   

   You could then use it either like this: (package "foo")
   which returns {:name "foo" :providers #{} :clients #{}}

   or like this: (package "foo" :providers #{"bar"} :clients #{"baz"})
   which returns {:name "foo" :providers #{"bar"} :clients #{"baz"}}.

   You can even leave out :providers or :clients at will without having to worry about the order of arguments in your function call. This is another thing that is more verbose, but more readable if you don't mind the extra typing every time you call the function.

3. I mentioned in #1 the idea of carefully naming your functions so that you can tell which ones are mutating state vs. which ones are just pure functions. I would consider naming your non-pure functions (i.e. the ones that are changing the value of refs/atoms) with a ! at the end. This is idiomatic in Clojure. I would do that with the following functions: update-sys! add-sys-package! install-new! install! uninstall-package! and uninstall!.

   I would do the same thing with your stop! and exit! functions, and would also consider renaming run to run? since it represents a boolean value.

• rileyfreeman: Dave, thanks for your detailed analysis. These are exactly the sort of idiomatic best practices I was looking for.
• Dave Yarwood: You're quite welcome! Hope you're enjoying learning Clojure!

Answer 2

You can use clojure sets to group like outcomes in a condp.

(condp get (lower-case command)
 #{DEPEND} (apply add-sys-package args)
 #{LIST} (print-installed)
 #{INSTALL} (install (first args) true)
 #{INFO} (println (get-package (first args)))
 #{REMOVE UNINSTALL} (uninstall (first args) true)
 #{SYS} (print-sys)
 #{EXIT END} (exit)
 nil)

In the case of the cond you have written, a case statement is probabaly the best bet. Not only do you get better performance (constant time), but you can group common values. Watch out though, like in Java, you can only have constants in the case statement. This means that using symbols in the case (as you have written in your condp) will make the statement look for the actual symbol, not the value assigned to the symbol. A common practice is to use keywords when comparing in case statements.

(case (-> command lower-case keyword)
 :depend (apply add-sys-package args)
 :list (print-installed)
 :install (install (first args) true)
 :info (println (get-package (first args)))
 (:remove :uninstall) (uninstall (first args) true)
 :sys (print-sys)
 (:exit :end) (exit)
 nil)

• rileyfreeman: Thanks. Coming from Java, case was the first thing I tried, but I ran into the problem that you mentioned about it only matching symbols. In my code DEPEND, LIST, etc are all "def-ed" at the beginning of the file to reference their corresponding string so condp matches the value of the reference. How could I get a user input from read-line to match the symbol?
• rileyfreeman: I see, the keyword argument. Thanks. I somehow glossed over that.