code-reviews 2020-07-30

I'm curious about my Caesar cipher solution. Is this too obscure an approach to it? I felt kind of nifty skipping the whole ASCII thing and going with the cycled, lazy seq of letters but maybe I'm harming readability. https://github.com/Chase-Lambert/CS50/blob/master/src/cs50/caesar.clj

Some of the weird input checks in the bottom caesar function are just to adhere as close as possible to the problem specs so may look pretty ugly

looks pretty good. I would probably create a map that maps letter -> cypher letter and use that.

oh interesting. I might take this approach in the next exercise, substitution, because it is like a caesar cipher on steriods where every letter will have it's own cipher key

the seq call in the defs of lower and upper are superfluous

Ahh! I always do that

😁 1

since you're working with characters in encrypt character, you might not need the (map str for the lower and upper defs

seems fine either way though

.indexOf requires Strings so I was forced to go that route if I wanted to use it

lower and upper aren't strings

> (type lower)
clojure.lang.Cycle

which apparently has an indexOf method

Ah, I meant it gives me a seq of "a" "b", etc instead of \a \b

> (def lower (cycle "abcdefghijklmnopqrstuvwxyz"))
#'lower
> (.indexOf lower \f)
5

huh... how did this not work for me before

that's bizarre but hey, I'll run with it.

This is great, thank you! I'll wrap my head around that map suggestion of yours.

I'm not sure how to think about depending on .indexOf for cycle

well I only need it for the initial check so it's just checking 26 letters right (or the initial chunk of 32 elements or whatever lazy seqs do)

When I try .indexOf with a char it hangs my repl...

that's because your cycle is doesn't have chars in it yet

if you still have lower and upper defined in terms of one character strings

oh! Of course. eesh

i did the same thing 😄

I'm also finding a clojure.string/index-of that does the same thing. I wonder if it wraps .indexOf

I'm sure it's fine. the only concern is I'm not sure when it was added, so it might not work on very old versions of clojure

but I wouldn't worry that it would break in future versions.

and it's most likely that it's always been there and I never knew about it

the source of that clojure string function shows them coercing a character to string anyways. hahaha

and then using .indexOf

would it be out of place to present an alternative version using Clojure idioms?

it doesn't borrow from any of the existing code

I would love to see whatever you've got

(defn caeser
  "Takes a number (or no number, defaulting 13) and returns
  a rotated ceaserian cypher over the english alphabet by that
  rotation, all input outside a-zA-Z passes unchanged."
  ([] (caeser 13))
  ([N]
   (let [char-digit-range #(range (int %1) (inc (int %2)))
         lower (char-digit-range \a \z)
         upper (char-digit-range \A \Z)
         letters #(mapcat (partial map (comp str char)) %&)
         shift #(concat (drop %1 %2) (take %1 %2))
         original (letters lower upper)
         conversion (letters (shift N lower) (shift N upper))
         enc (zipmap original conversion)]
     (fn caeser [s]
       (string/replace s #"." #(enc % %))))))
#'user/caeser
(ins)user=> (def c (caeser))
#'user/c
(ins)user=> (c "Hello, World!")
"Uryyb, Jbeyq!"
(ins)user=> (c *1)
"Hello, World!"

I was attempting maximum clojure-idiom-density :D

should have written spec.alpha spec to round it out

a small thing in what you have there: (map f (seq c)) is identical to (map f c) in all cases - map uses seq internally

and I think aside from going all in with clojurisms, the important differences between your version and mine are: • I attempt to do as much work as possible one time before the function is called by abstracting repeated operations into a lookup table • I don't trust sequential operations for string->string so go out of my way to construct my code so it's a string function with a helper, with no string->seq seq->string conversions

Ok, awesome. That is definitely a different approach, I am going to marinate on that. Looks good

I think #(mapcat (partial map (comp str char)) %&) deserves a comment

OK - agreed on that one yeah

I'll probably be on here over the next couple weeks asking for opinions on my other solutions (for Harvard's CS50 course) on here too if anyone is willing to take a gander