0

I'm working on a Clojure algorithm to solve the problem posed here: http://spin.atomicobject.com/2011/05/31/use-clojure-to-move-drugs-a-programming-challenge/ and I've run into a hiccup.

I'm using a recursive algorithm (perhaps not the right choice to begin with), to walk over a vector of "doll" structs that are ordered by highest to lowest value-to-weight ratio. The relevant code is:

(defn get-carryable-dolls 
  [dolls carryable-dolls]
  (def doll (first dolls)) ;initializing for use in multiple places
  (def rest-dolls (rest dolls)) ;initializing for use in multiple places
  (
    if (will-fit? doll (get-weight-sum carryable-dolls)) 
    ( ;will fit
      (
        if 
        (= carryable-dolls {})
        (def new-doll-set [doll]) ;First trip, get rid of empty set by initializing new
        (def new-doll-set (flatten [doll carryable-dolls])) ;otherwise, flatten set into vector of structs
      )
      ;tests to see if we have any more dolls to test, and if so, recurses. Otherwise, should pass the resultant vector
      ;up the stack. it appears to be the "else" potion of this if statement that is giving me problems.
      (if (not= () rest-dolls) (get-carryable-dolls rest-dolls new-doll-set) (vec new-dolls))
    )
    ( ;will not fit
      ;gets the rest of the dolls, and sends them on without modifying the vector of structs
      ;it appears to be the "else" potion of this if statement that is giving me problems.
      (if (not= () rest-dolls) (get-carryable-dolls rest-dolls carryable-dolls) (vec carryable-dolls))
    )
  )
)

That code is working correctly; returnable-dolls contains the desired vector of doll structs to return as a solution. Unfortunately, when I attempt to return the returnable-dolls vector to the calling position, I'm receiving the following error:

CompilerException clojure.lang.ArityException: Wrong number of args (0) passed to: PersistentVector,compiling:(drugmover\tests.clj:83)

Line 82-83 read:

(def empty-dolls {})
(def designated-dolls (get-carryable-dolls sorted-values empty-dolls))

I'm stumped as to what might be causing the issue in the compiler error, and since Clojure seems to prefer terse error messages over stack traces (or at least, the REPL functionality in Clooj does), I'm at a loss to how to fix it. If anyone has any suggestions, I'd greatly appreciate them!

Thanks in advance.

EDIT:

I've modified the code with the suggested fixes by those in the answers and the comments and provided a handful of comments to help illustrate the flow control that's going on. Hopefully, by illustrating my thinking, someone will be able to give me an idea where I'm going wrong.

Improve this question
6
  • 1
    You're sprinkling parenthesis all over the code, as if they were curly brackets. Commented Apr 22, 2012 at 4:25
  • Can you provide a specific example of somewhere that I could remove the parentheses and still receive the same result? I'm very, very new at this. Commented Apr 22, 2012 at 4:29
  • For example: right after the condition of the first if there are two opening parenthesis; the first one is unnecessary, the second is correct, because it's the opening parenthesis of the second if. Commented Apr 22, 2012 at 4:37
  • @Óscar López Thanks, I've updated the code with changes that I've made and some comments showing flow control to hopefully illustrate my thought process. Commented Apr 22, 2012 at 4:48
  • You still have extra parens around your if statements. You also don't need the first if at all, because an empty sequence is still a sequence so you don't need to treat it specially; just use (conj carryable-dolls doll). And if you really need local variables - and you don't need most of them - use let not def as def defines a global variable. Commented Apr 22, 2012 at 7:35

3 Answers 3

Reset to default
4

The following code incorporates most of the suggestions you already received in other answers and others, namely:

  • getting rid of the superflous (and wrong) parenthesis
  • formatting the code in a more idiomatic way
  • use loop/let instead of def for local name bindings
  • use seq for checking empty lists
  • remove the unnecessary check for an empty carryable-dolls seq before adding an element

I can't test it without having the definition of the ancillary functions you have (e.g. will-fit?), but at least should fix a couple of issues (and bring the code in a more readable form):

(defn get-carryable-dolls 
  [dolls carryable-dolls]
  (loop [doll (first dolls)
         rest-dolls (rest dolls)]
    (if (will-fit? doll (get-weight-sum carryable-dolls))
      (let [new-doll-set (if (seq carryable-dolls)
                           (cons doll carryable-dolls)
                           [doll])]
        (if (seq rest-dolls)
          (recur rest-dolls new-doll-set)
          (vec new-dolls)))
      (if (seq rest-dolls)
        (recur rest-dolls carryable-dolls)
        (vec carryable-dolls)))))

The following is a complete refactoring of the code that leverages the standard reduce function and defines a function that provides the core decision logic whether a doll has to be included or not in the result:

(defn add-if-fits [dolls doll]
  (if (will-fit? doll (get-weighted-sum dolls))
    (cons doll carryable-dolls)
    carryable-dolls))

(defn get-carryable-dolls [dolls carryable-dolls]
  (reduce add-if-fits carryable-dolls dolls))
Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

skuro, thanks for your help. Your second solution provides the solution that I was looking for, and is obviously much more succinct than the one I originally posted. You're a life saver!
3

There are way, way too many parens in this code, and they are causing problems. I strongly recommend you format your code the way everyone else does, which will make errors like this easily stand out. I can't even guess what you're trying to do, so I can't rewrite the whole snippet, but the relevant thing to note is that the syntax for if is:

(if test then else)

No extra parens are allowed around any of these things: (if true 1 2) is fine, for example, but (if (true) 1 2) would try to call true as a function, and fail because it's a boolean. If you want to "group" expressions together and evaluate them for side effects, you want (do expr1 expr2), not (expr1 expr2).

Improve this answer

6 Comments

Thanks for the suggestions. I'll look into reducing the parentheses and see if that can provide any hints. Such is the danger of trying to solve a code problem like this one within 24 hours of using the language for the first time.
So if this is already computing the right value but then failing because it's accidentally calling something as a function, you can probably fix it by replacing each instance of ((a) (b)) with (do (a) (b)). Once you get more familiar with clojure you can go back and remove all the defs from inside this function, which are a holdover from a very imperative style.
I'm not sure that it is failing because it's accidentally calling something as a function. And it's very likely my inexperience with the language talking, but I honestly can't see a single example of parentheses in the pasted statements that can be removed. Could you provide a more specific example?
The pair of parentheses starting on line 3 are superfluous, for example. A function body doesn't have parentheses around it like curly braces.
@neveu: Thanks for the example; I've removed the superfluous parentheses, but it unfortunately didn't solve the problem.
|
3

You have extra parens around your if code and that is the reason for that error:

The below code will produce the same error as it has extra parens (same case in your if when the else part is called to create the vector:

((vec {:a 10 :b 100}))

Try to execute this in REPL and you will see the same exception:

java.lang.IllegalArgumentException: Wrong number of args (0) passed to: PersistentVector (NO_SOURCE_FILE:0)
Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.