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-packagesandinstalled-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 Overflowthis question on Stack Overflow for a few good methods.
You used
definside of two function definitions, which is generally considered incorrect in Clojure. Usually you will only usedef,defn, etc. on the top level of your program. When you're inside of adef,defn, etc., you should useletinstead 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.
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-packagesandinstalled-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
definside of two function definitions, which is generally considered incorrect in Clojure. Usually you will only usedef,defn, etc. on the top level of your program. When you're inside of adef,defn, etc., you should useletinstead 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.
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-packagesandinstalled-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
definside of two function definitions, which is generally considered incorrect in Clojure. Usually you will only usedef,defn, etc. on the top level of your program. When you're inside of adef,defn, etc., you should useletinstead 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.
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-packagesandinstalled-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
definside of two function definitions, which is generally considered incorrect in Clojure. Usually you will only usedef,defn, etc. on the top level of your program. When you're inside of adef,defn, etc., you should useletinstead 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, ornilif it's not. Because any value that isn'tfalseornilis 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 functioncollrather thanseq, asseqalready refers to theclojure.core/seqfunction.You can simplify the
(get (get ...form in yourget-providersandget-clientsfunctions 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
doin yourinstall-newfunction definition. Any time you're doing something like defining a function usingdefn, there is already an "implicitdo":(defn install-new [package] (println "\t installing" package) (swap! installed-packages conj package))You have a few functions (
install,not-needed?anduninstall) that take a parameter called eitherself-installorself-uninstall, which is expected to be eithertrueorfalse. 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
uninstallfunction smells of OOP practices, in particular in the way that you have nestedifstructures. 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 usingcondas 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
createfunction topackage, simply becausecreatesounds 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 itpackageinstead, it would make it clearer that(package "foo" bar baz)just represents a package named"foo"with providersbarand clientsbaz. 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.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
:providersor:clientsat 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.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!anduninstall!.I would do the same thing with your
stop!andexit!functions, and would also consider renamingruntorun?since it represents a boolean value.