Improve Code Qualitiy

[RyanMcG]

I ran across a very cool static analysis tool called kibit. I ran it against our code and got the following. If you notice one of these in something you did then change it if you want. Or I might later... whatever.

EDIT: Fixed many issues and removed them from the list below. Only nerual and vector_convert are remaining.

Check failed -- skipping rest of file java.lang.RuntimeException: Map literal must contain an even number of forms At src/board_ultimatum/engine/neural.clj:52: Consider using:

  (vec (map (fn [game] (:bgg_id game)) (model/find-all)))

instead of:

  (into [] (map (fn [game] (:bgg_id game)) (model/find-all)))

At src/board_ultimatum/engine/neural.clj:54: Consider using:

  :bgg_id

instead of:

  (fn [game] (:bgg_id game))

At src/board_ultimatum/engine/neural.clj:60: Consider using:

  (vec (concat (get-vector id-A) (get-vector id-B)))

instead of:

  (into [] (concat (get-vector id-A) (get-vector id-B)))

At src/board_ultimatum/engine/neural.clj:66: Consider using:

  (.getData (. net compute (. pair getInput)) 0)

instead of:

  (. (. net compute (. pair getInput)) getData 0)

At src/board_ultimatum/engine/neural.clj:66: Consider using:

  (.compute net (. pair getInput))

instead of:

  (. net compute (. pair getInput))

At src/board_ultimatum/engine/neural.clj:66: Consider using:

  (.getInput pair)

instead of:

  (. pair getInput)

At src/board_ultimatum/engine/vector_convert.clj:42: Consider using:

  (set
    (remove
      nil?
      (map
        (fn [tag] (cond (= (:subtype tag) "category") (:value tag)))
        (:tags game))))

instead of:

  (into
    #{}
    (remove
      nil?
      (map
        (fn [tag] (cond (= (:subtype tag) "category") (:value tag)))
        (:tags game))))

At src/board_ultimatum/engine/vector_convert.clj:48: Consider using:

  (set
    (remove
      nil?
      (map
        (fn [tag] (cond (= (:subtype tag) "mechanic") (:value tag)))
        (:tags game))))

instead of:

  (into
    #{}
    (remove
      nil?
      (map
        (fn [tag] (cond (= (:subtype tag) "mechanic") (:value tag)))
        (:tags game))))

At src/board_ultimatum/engine/vector_convert.clj:201: Consider using:

  (vec (map (fn [game] (:bgg_id game)) (model/find-all)))

instead of:

  (into [] (map (fn [game] (:bgg_id game)) (model/find-all)))

At src/board_ultimatum/engine/vector_convert.clj:203: Consider using:

  :bgg_id

instead of:

  (fn [game] (:bgg_id game))

At src/board_ultimatum/engine/vector_convert.clj:206: Consider using:

  (vec (map (fn [game] (:data game)) (map to-vector (model/find-all))))

instead of:

  (into
    []
    (map (fn [game] (:data game)) (map to-vector (model/find-all))))

At src/board_ultimatum/engine/vector_convert.clj:207: Consider using:

  :data

instead of:

  (fn [game] (:data game))

At src/board_ultimatum/engine/vector_convert.clj:217: Consider using:

  (vec (map (fn [i] (sel components :cols i)) (range 10)))

instead of:

  (into [] (map (fn [i] (sel components :cols i)) (range 10)))

At src/board_ultimatum/engine/vector_convert.clj:222: Consider using:

  (vec (map (fn [i] (mmult full-data (nth pc i))) (range 10)))

instead of:

  (into [] (map (fn [i] (mmult full-data (nth pc i))) (range 10)))

[spankykopita]

This is really cool, though not all of the recommendations are better.

[RyanMcG]

@fu-manchu Which ones do you think?

[spankykopita]

Looking at it again, the only suggestions that I would ignore are those involving pos? and neg? functions. Thinking of a value as greater or less than 0 seems more meaningful in these contexts than thinking of a value as positive or negative.

Interestingly, this is essentially an expert system. We could have done something like this for the project.

Did you run this on all clojure files? It might be worth assigning David on it since most of these are from his files.

[RyanMcG]

I did run it on all files. I could see how one might like (> % 0) more than (pos? %). I'm not sure if I agree, but it's the least concerning of changes. Lots of these things are opinions anyways. Still, I didn't even know there was a pos? function before looking at this.

David seems like a reasonable candidate for this, but I'd be just as happy with everyone fixing their own parts. Really whatever is easiest. Also, I made this a TB6 issue since its not time sensitive.

Another code quality thing we might want to look at is indentation and formatting (removing tab characters and such).

On Mon, Nov 12, 2012 at 5:54 PM, Chris Powers notifications@github.comwrote:

Looking at it again, the only suggestions that I would ignore are those involving pos? and neg? functions. Thinking of a value as greater or less than 0 seems more meaningful in these contexts than thinking of a value as positive or negative.

Did you run this on all clojure files? It might be worth assigning David on it since most of these are from his files.

— Reply to this email directly or view it on GitHubhttps://github.com/DRSNJM/board-ultimatum/issues/5#issuecomment-10308162.

[RyanMcG]

@dalbz your turn.

[RyanMcG]

To you sublime users: @dalbz @fu-manchu @saterus

https://github.com/odyssomay/sublime-lispindent

[spankykopita]

Interesting. I'm using it now. It works how I'd expect overall, though it's annoying when used with noir. I'm trying to show an example, but github wants to remove the spaces.

[RyanMcG]

@fu-manchu, all comments are github flavored markdown. See the link in the comment composition box? Also the preview tab.

[RyanMcG]

@dalbz You gonna do this?

[dalbz]

@RyanMcG - yeah at some point later. honestly my code has been changing so much that I didn't find it productive to refactor

[RyanMcG]

It's not really re-factoring in the traditional sense. You are to simply find and replace. IMO, you benefit from doing this sooner rather than later so you learn what you should be writing. You are not doing a massive restructuring of design or renaming a bunch of things.

I thinking the learning aspect here is more important than actually replacing the code at this point.

[saterus]

This is probably not a worthwhile endeavor at this point. They don't care about our code quality, and we probably have better things to worry about.

[RyanMcG]

@saterus I don't think it was ever worthwhile for the scope of this project but that doesn't mean it isn't valuable. Best practices and what not...