Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
  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 Overflowthis 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.

  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.

  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.

Source Link
Dave Yarwood
  • 1.1k
  • 6
  • 13

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.