Hot answers tagged

17

First of all, in my opinion, both the algorithm is nice and interesting, and the code is really well-written, broken down into easily understandable functions! Well done! The only addition that I can make are corner-cases (and their possible fixes) for some of the helper functions. However, please note, that from the main entry point, I did not find any way ...


16

This is two and a half years old, but for the sake of future viewers: Overall your coding style looks pretty good, especially for someone new to Clojure. I don't have any comments on the AI engine itself, but here are a few structural notes: not-nil? already exists in clojure.core as some?. Docstrings are usually phrased actively: e.g. "Returns the result ...


10

As you wrote in your comments, (.contains "str" "sub") is perfectly fine. it is indeed java interop - it runs the method contains on String object "str". two more comments, first, passing str as a var name isnt so good, since str is a function, so you should consider giving it a different name. Second, in your implementation, its quite redundant to write (...


9

I'm the author of core.matrix, so hopefully I can give you some tips from that perspective. If you want to improve performance, it's much better to use vectors in an optimised format throughout (vectorz-clj is a fine choice) rather than mixing in Clojure vectors everywhere. This saves the overhead of converting to/from Clojure vectors all the time, which is ...


9

@abuzittingillifirca made a good point in that you can leave the parentheses off of the -> forms that are just a single element. html/html-resource is actually quite flexible; it can take a variety of different kinds of arguments, including strings, URLs, input streams, etc. Assuming the url argument to your function is a string, you can take out the ...


9

Wow! If this is you after one day, we hope for great things. However, you can make your program simpler. Let's start with factorize. It has a couple of defects: It uses proper recursion where tail recursion with recur would suffice. So it would overflow the stack for a number having too many factors. However, since it uses long arithmetic, no number is ...


8

General Questions Yes. Your tests are fine. I found the code hard to follow without the requirements. To some degree, that's to be expected. Even once I read the requirements, there were things that I found hard to follow. More on that later. How idiomatic is my code? Never use camel case for your clojure-defined symbols. You should always use lower-cased,...


7

Don't use the multiple-arity syntax for defn unless you actually need to: (defn url-decode [text] (URLDecoder/decode text "UTF-8")) Your code could use some line breaks here and there. This is how clojure is usually indented: (defn translate [term src-lg tgt-lg] (let [translations (get-translations (fetch-url (get-url src-lg term)))] (if (...


7

Don't always create new atoms My first comment is that your use of atom seems quite wrong to me: atoms are mutable entities which you're supposed to mutate via e.g. swap! or reset!, which alter the value the atom points to without changing the atom reference itself. What your code is doing is to create new atoms at every call, i.e. you change the atom ...


7

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


7

Yes. I think you still miss a very useful part of clojure - using its various collection types to model your solution. In your problem, you could use the fact that a string is a seq and a map acts as a function to calculate the cost of a word: ;; this map will act as a function from a name to its ordinal (def name->position (zipmap names (map inc (range))...


7

I'm pretty new at Clojure myself, and haven't studied the collection algorithms very much yet, so this may not address your performance concerns, but I did find a few things that could be improved. Potential problem with "real" document input As I started going through your functions and how they work together, I noticed that your logic makes the ...


7

Is this "good clojure?" I tried to stick to sort of basic functional programming practice, composing lots of short functions with discrete behavior and such. I think you're doing more work (writing more functions and doing more data transformations) than necessary. Two-and-a-half points in particular: In general I'd recommend writing functions that ...


6

First of all, good job! This is obviously a complex algorithm and it looks like it's working. I'm going to do this incrementally. So I'll save this and keep editing as I go. And since it's so long, I won't get to everything. Plus I don't understand the algorithm too well. First of all, doc strings in Clojure go before the arguments. I used to make this ...


6

I don't know if you're still at it (given it's been a few months) ; in any case, here's my answer (it could be useful to someone, who knows). speaking of standard functions, maybe you could leverage the Clojure reader e.g.: (def to-int (comp read-string #(apply str %))) ;; the str bit is ugly, but necessary (read-string consumes Strings) Depending on what ...


6

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


6

There are a few minor improvements you can make here that lead to a fairly elegant implementation. First, using flatten is rarely a good idea, because rather than just dealing with the top-level structure of the thing you hand it, it reaches down into its inner structure, thus making it brittle when you change the representation of your data. Therefore, you ...


5

I wrote the modified version of your findpath function using recursion: (defn- dfs [graph goal] (fn search [path visited] (let [current (peek path)] (if (= goal current) [path] (->> current graph keys (remove visited) (mapcat #(search (conj path %) (conj visited %)))))))) (defn findpath "...


5

This is a huge file to comment on, so maybe just some general tips: map and mapv (a variant of map returning a vector) can take any number of sequences as argument, each element of the nth sequence being used as the nth argument of the function given to map. So plus-pair could be written: (defn plus-vectors [v1 v2] (mapv + v1 v2) loop can be replaced by ...


5

My suggestions: Remove the values function and, as mikera suggests, do these types of things inline. Most clojure programmers will know what you mean. There is no need to reduce with the max function when max is a variable-arity function. (reduce max [1 2 3 4]) ;; 4 can be (apply max [1 2 3 4]) ;; 4 When you reduce, you are effectively doing the ...


5

Some quick points / ideas: The code looks pretty decent overall for a first attempt at Clojure, and you have made good use of the "functional" style. Nice work! You have used a defrecord for the cells data structure. This isn't really buying you anything, since you can just use a vector e.g. [1 2] or a map e.g. {:x 1 :y 1} directly. I'd probably use a ...


5

Another suggestion would be to use maps as method arguments. instead of : (defn viterbi-step [prior obs states trans-p emit-p] using maps: (defn viterbi-step [{:keys [prior obs states trans-p emit-p] :as m}] So when viterbi-step is called, you can avoid passing all the arguments, instead you can assoc the new arguments onto the map passed to the ...


5

Well, the most obvious fix is indeed map-invert-preserving-dups - the whole thing could be more easily written as: (defn map-invert-preserving-dups [m] (apply merge-with into (for [[k v] m] {v [k]}))) The for expression yields a sequence of maps like [{a [1]} {b [2]} {a [5]}]. Apply calls merge-with into on all of those maps. If you ...


5

I will point out stuff that I'd do differently, though I am by no means a clojure expert, I do have some experience in it. Numbered items will reappear inside the code as comments. no DEF inside defn body def will define the var for your entire namespace as such, you will pollude your namespace with temporaries use LET instead no DO necessary inside the ...


5

I tried keeping your approach, although I would've made it an infinite lazy sequence. Yours is probably a better idea, since you still have the infinite sequence (mapping to range as you did) and you can map it to other non-linear sequences too. (def targets ;; This is more readable for me (sorted-map 3 "fizz", 5 "buzz", 7 "baz")) (defn ...


5

It's not bad, but we can make it a bit more idiomatic. Let's look at this function first: (defn- matches-attributes [current-attributes required-attributes] (reduce #(and %1 (= (second %2) ((first %2) current-attributes))) true required-attributes)) The use of reduce is a bit odd, instead let's use every? Secondly, if current-...


5

First of all, you could use abstract structural binding (a.k.a. destructuring) to reduce the number of let bindings: (defn- create-counts_org [[_ & present :as coll]] "Computes how many times did each 'next state' come from a 'previous state'. The result type is {previous_state {next_state count}}." (let [zipped (map vector coll present) ...


5

Cool idea! I could definitely see this coming in handy in a variety of domains. I noticed that you have some def and defn statements within the function definition of process-match. It's generally considered un-idiomatic to use def/defn anywhere except for at the top level of the namespace you're in. Usually you would use let instead if what you're trying ...


5

How can I refactor this function to remove the defn? What you are looking for is letfn. It can be used to locally define a recursive function or corecursive functions. In this case: (fn [numberOfFibs] (letfn [(lazyFib [a b] (cons a (lazy-seq (lazyFib b (+ b a)))))] (take numberOfFibs (lazyFib 1 1)))) And what are the problems with style and ...


5

Warning: The following is opinionated at times. At other times it is more opinionated. Channeling PG I've heard on the internet so it must be true, that Paul Graham will get right to the point when interviewing applicants to Y-Combinator with "What problem does this solve?" It's hard to understand code without understanding what it is supposed to do. And ...


Only top voted, non community-wiki answers of a minimum length are eligible