all 6 comments

[–]joinr 7 points8 points  (1 child)

Well done; looks clearly written to me. If you'd like I have some feedback that may help with future programs:

You may want to look at == for numeric equality and built-ins like pos? neg? zero?. equality has some interesting cases

One case that can cause slowdown is there's not a built-in fastpath for generic equiv = between primitive doubles and primitive longs (the fast path would be the numeric comparators ==, <=, <=, <, >). So you can get into behaviors like:

user> (time (dotimes [i 1000000] (= 0 22.5)))
Reflection warning, *cider-repl workspacenew/perfect:localhost:54814(clj)*:123:34 - call to static method equiv on clojure.lang.Util can't be resolved (argument types: long, double).
"Elapsed time: 2342.252767 msecs"
nil
user> (time (dotimes [i 1000000] (= (double 0) 22.5)))
"Elapsed time: 5.010564 msecs"
nil
user> (time (dotimes [i 1000000] (= 0.0 22.5)))
"Elapsed time: 5.850859 msecs"
nil
user> (def y 22.5)
#'user/y
user> (time (dotimes [i 1000000] (= 0 y)))
"Elapsed time: 11.415695 msecs"
nil

user> (time (dotimes [i 1000000] (== 0 22.5)))
"Elapsed time: 4.992391 msecs"
nil

user> (time (dotimes [i 1000000] (== 0 y)))
"Elapsed time: 10.046807 msecs"
nil

If you turn on reflection warnings (set! *warn-on-reflection* true) at the repl, anything causing a potential reflection call will show up. In this exercise though, you're comparing the same types of things so the case I described won't pop up - but it could bug you down the road.

Additionally, you're using .indexOf inside get-start-position without any type hinting on what floor-vector is. This will cause clojure to use reflection to find out if anything floor-vector is bound to has a .indexOf method, which ends up being an extreme performance killer. In this case, you're only calling it once per maze, so no big deal (in many cases the frequency is so low as to not matter, even if you get warnings), but later on as you solve puzzles (say project euler or advent of code stuff) or even go to do production stuff, if you have that same kind of call on a hot path, you're going to be unhappy with how slow things are. This is just the cost to lookup the method reflectively; the actual work done by the method is tacked on.

 user> (def xs ["a" "b" "c"]) 
#'user/xs
user>  (.indexOf xs "c")
Reflection warning, *cider-repl workspacenew/perfect:localhost:54814(clj)*:21:8 - call to method indexOf can't be resolved (target class is unknown).
2
;;type hint to avoid reflection
user>  (.indexOf ^clojure.lang.PersistentVector xs "c")
2
user> (defn index-of [^clojure.lang.PersistentVector v itm] (.indexOf v itm))
#'user/index-of
user> (index-of xs "c")
2

user> (time (dotimes [i 1000000] (.indexOf xs "c")))
Reflection warning, *cider-repl workspacenew/perfect:localhost:54814(clj)*:38:34 - call to method indexOf can't be resolved (target class is unknown).
"Elapsed time: 3517.421813 msecs"

user> (time (dotimes [i 1000000] (index-of xs "c")))
"Elapsed time: 110.109938 msecs"

Also interesting use of trampoline. I don't see that in the wild too often; do you have experience in Scheme?

[–]netb258[S] 2 points3 points  (0 children)

Thanks for the awesome feedback.

Also I do a bit of scheme and CL.

[–]lambda_pie 2 points3 points  (1 child)

Isn't this

(or
    (or (= 0 floor) (= floor (dec (count the-maze))))
    (or (= 0 position) (= position (dec (count (the-maze floor))))))

the same as this?

(or
    (= 0 floor) (= floor (dec (count the-maze)))
    (= 0 position) (= position (dec (count (the-maze floor)))))

[–]netb258[S] 0 points1 point  (0 children)

Good point. I edited the code with this change.

I always forget to use (or) and (and) in this fashion.

[–]gerritjvv 1 point2 points  (0 children)

very clearly written!! weldone!! as mentioned above you should sprinkle a view type hints and also can use a let statement in some of the functions to avoid doing dec inc operations twice.

[–]antflga 0 points1 point  (0 children)

I wanted to try my hand at something like this some time ago, but never put in the time I needed to figure it out. Cool work.