Clojurians Log v2

Would anyone mind taking a quick look at my small temperature converter program? The actual "business logic" is only a couple of lines but I'm more curious on how you would approach taking in the user input and validating it. That takes so much more code. It's still half the LOC of my Rust implementation of the same exercise. https://github.com/Chase-Lambert/temperature-converter/blob/master/src/temperature_converter/core.clj

I see multiple places where you can move forms to eliminate duplicate code

never use a namespace by full name - always require and provide an alias

even if only using once huh? Ok, ok, I can dig it.

some clojure environments preload clojure.string, but that's not a guarantee that clojure makes, and your code can easily break if you don't require it

Ahh

dedupe example: change

(if (= x "y")
  (println "foo" (frob x) "y")
  (println "foo" (grub x) "z"))
vs.
(println "foo"
         (if (= x "y") (frob x) (grub x))
         x)

also at the end you can replace (if cond? (do (f) (recur)) (recur)) with (when cond? (f)) (recur) since let already has a do block

Ahh! You mean when is like having a do block?

it is, but what I mean, is that both branches of if called recur, so you can take the recur out of the if

and let already has a do block, so you can just put the recur after the if

and an if with only one branch can be replaced with when

(I hope that shows all my work...)

Ok, I get it. Thanks so much!

some more suggestions: in clojure 1.0 is a double, not float, and there are few reasons for using Float/parseFloat instead of Double/parseDouble

all clojure math functions already promote float to double as well

(drop-last temp) - using collection operations on strings works in simple examples like this, but it's a code smell and extremely inefficient

also why are you even dropping anything from what you read?

Well the user input comes in like "100C" so I want to split that into it's two relevant parts

oh! you are extracting a digit

How would you approach parsing that string?

(ins)user=> (re-matches #"(\d+)(.)" "100C")
["100C" "100" "C"]

with a little fiddling that also helps you handle optional whitespace in a human friendly way

(bonus)

ahhh, ok. I remember doing something like that on an exercism problem I did back in the day. Regex's are still difficult for me.

fixed - yea res are weird

(ins)user=> (re-matches #"([\d\.]+)(.)" "100.3C")
["100.3C" "100.3" "C"]

so then in my let block I could use destructuring to bind to degrees and scale like [[_ degrees scale] (re-matches ...)] and continue on with my try/catch checks?

right - that sounds correct to me

but what if the re-matches fails because they didn't input it right... hmmm

also (or (= x y) (= x z)) can be replaced by (#{y z} x) if both y and z are non nil and non-false

@chase-lambert in that case, re-matches returns nil, the destructure offers nil, and the behavior is the same as your current code

oh perfect, which they should be since they are at the end of the and statement that already checks if they are nil and false

oh I meant (and degrees scale (or (= scale "C") (= scale "F"))) can be (and degrees (#{"C" "F"} scale))

but maybe that's what you meant too :D

yup except I still was double checking that scale was there. haha. this is great

right - the check that scale is either "C" or "F" already checks that it wasn't nil or false, implicitly

I should do my refactor suggestions in smaller steps

@chase-lambert often a clojure refactor feels like solving a sudoku or something

you should notice your code becoming smaller and easier to understand at each step

I just watched a great sudoku solving video, you might have seen the same.

oh the one with the knights moves?

yup!

yeah, that sort of puzzle solving and simplifying clojure code scratch the same spot in my brain

I like all the changes but the regex still concerns me. I can see what it does but could I recreate it myself. Could you parse your regex into an English sentence for me? "Ok, so once you hit a digit, divide this into blah blah blah"?

I definitely felt icky doing it my way with the drop-last and such though. I just sensed that it wasn't efficient.

find 1 or more of "digit or .", followed by a single token

[] - group, matches any item in the group \d - any digit \. - matches ".", since . is special + - one or more of preceding . - any token () - another kind of group - return string matched by this group separately

I think that covers it

Ahh ok, I'm actually writing these in a notebook by hand right now. Haha. Old school

one more refactor: since (System/exit 0) already causes the rest of the function body to be skipped, you can remove the let from the if, and reducing nesting makes things more readable

How so?

(if cond?
  (exit)
  (f)) 
;; vs
(when cond? (exit))
(f) 

ok, ok. so then my code would be

(let [temp (read-line)] 
  (when blah blah) 
  (let [[_ degrees scale blah blah 

right

the same thing I suggested with pulling the two recur calls out of the if, I think

Good stuff. Ironically it all ends up at the same LOC but much cleaner. Did I misinterpret anything? https://github.com/Chase-Lambert/temperature-converter/blob/master/src/temperature_converter/core.clj

I think the "F" and "C" in the conert-temp defn should be moved out and just be scale as an arg to str

But it's gotta be the opposite scale

I'm printing "32F = 0.00C"

oh! I missed that

I'd use ({"C" "F" "F" "C"} scale) and call that inside str, instead of hard-coding

and using an extra arg to format like that is odd, I'm surprised it works

otherwise yes, that looks right - simplified significantly even if the line count hasn't changed

my repl shows that the format call doesn't break but the C or F is silently ignored

of course instead of the lookup above, the F or C could be inside the format string

I'm just not sure on how I want to format the expression now. Whenever I throw an if statement or whatever into the middle of a string formation I start to struggle to see what is happening. But this seems funky too:

(defn convert-temp [degrees scale]                                                
  (str degrees                                                                    
       scale                                                                      
       " = "                                                                      
       (if (= scale "C")                                                          
         (format "%.2f" (+ 32.0 (* degrees 1.8)))                                 
         (format "%.2f" (/ (- degrees 32.0) 1.8)))                                
       ({"C" "F" "F" "C"} scale))) 

Maybe a little less vertical:

(defn convert-temp [degrees scale]                                                
  (str degrees scale " = "                                                        
       (if (= scale "C")                                                          
         (format "%.2f" (+ 32.0 (* degrees 1.8)))                                 
         (format "%.2f" (/ (- degrees 32.0) 1.8)))                                
       ({"C" "F" "F" "C"} scale)))

I think my original might still be cleaner because it's such a simple either/or conversion

yeah - that's definitely funky

a let block would help I think

ahh, yeah that could be nice. let me try that

(let [degrees (if (= scale "C") ...)
      degree-string (format ...)
      degree-indicator ({"C" "F" "F" "C"} scale)]
   (str degrees scale " = " degree-string degree-indicator))

something like that

also, if you held onto the original read-in line, you could use it in your output

instead of piecing it back together manually

ooh, true. Waste not

Alrighty, we added 2 lines but I think we can say mission accomplished. https://github.com/Chase-Lambert/temperature-converter/blob/master/src/temperature_converter/core.clj

oh - btw. functions act as recur targets, so you can literally just remove the (loop [] ...) and replace it with the ... that was in it, without changing the behavior

almost forgot to mention that one

but won't I re-print my beginning messages that way?

oh right, never mind then

Yup. I thought of that one originally. Thanks for all your help!

I'm hooked on this code reviewing so you might have awakened a dragon here. Haha. I'm just solo learning so this slack is where it's at.