Fork me on GitHub
#code-reviews
<
2020-07-30
>
Chase17:07:55

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

Chase18:07:45

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

phronmophobic18:07:45

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

Chase18:07:05

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

phronmophobic18:07:47

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

Chase18:07:08

Ahh! I always do that

😁 3
phronmophobic18:07:04

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

phronmophobic18:07:01

seems fine either way though

Chase18:07:52

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

phronmophobic18:07:43

lower and upper aren't strings

phronmophobic18:07:53

> (type lower)
clojure.lang.Cycle

phronmophobic18:07:06

which apparently has an indexOf method

Chase18:07:30

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

phronmophobic18:07:52

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

Chase18:07:21

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

Chase18:07:54

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

Chase18:07:08

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

phronmophobic18:07:12

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

Chase18:07:02

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)

Chase18:07:32

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

phronmophobic18:07:51

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

phronmophobic18:07:11

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

Chase18:07:25

oh! Of course. eesh

phronmophobic18:07:49

i did the same thing 😄

Chase18:07:25

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

phronmophobic18:07:55

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

phronmophobic18:07:40

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

phronmophobic18:07:12

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

Chase18:07:37

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

Chase18:07:50

and then using .indexOf

noisesmith18:07:44

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

noisesmith18:07:54

it doesn't borrow from any of the existing code

Chase18:07:00

I would love to see whatever you've got

noisesmith18:07:34

(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!"

noisesmith18:07:24

I was attempting maximum clojure-idiom-density :D

noisesmith18:07:35

should have written spec.alpha spec to round it out

noisesmith18:07:47

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

noisesmith19:07:52

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

Chase19:07:15

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

phronmophobic20:07:36

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

noisesmith01:07:02

OK - agreed on that one yeah

Chase17:07:48

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