code-reviews

Chase 2020-07-30T17:58:55.191Z

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

Chase 2020-07-30T17:59:48.192200Z

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

Chase 2020-07-30T18:02:45.192400Z

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

phronmophobic 2020-07-30T18:02:45.192600Z

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

Chase 2020-07-30T18:04:05.192800Z

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

phronmophobic 2020-07-30T18:04:47.193Z

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

Chase 2020-07-30T18:06:08.193200Z

Ahh! I always do that

😁 1
phronmophobic 2020-07-30T18:08:04.193500Z

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

phronmophobic 2020-07-30T18:09:01.193700Z

seems fine either way though

Chase 2020-07-30T18:09:52.193900Z

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

phronmophobic 2020-07-30T18:11:43.194100Z

lower and upper aren't strings

phronmophobic 2020-07-30T18:11:53.194300Z

> (type lower)
clojure.lang.Cycle

phronmophobic 2020-07-30T18:12:06.194500Z

which apparently has an indexOf method

Chase 2020-07-30T18:12:30.194700Z

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

phronmophobic 2020-07-30T18:12:52.194900Z

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

Chase 2020-07-30T18:13:21.195300Z

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

Chase 2020-07-30T18:13:54.195500Z

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

Chase 2020-07-30T18:14:08.195700Z

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

phronmophobic 2020-07-30T18:15:12.195900Z

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

Chase 2020-07-30T18:16:02.196100Z

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)

phronmophobic 2020-07-30T18:17:32.196300Z

i meant that it's not documented anywhere afaik. looks like it comes from https://github.com/clojure/clojure/blob/e5fc803ff13f783661ef9491842719ab68a245ca/src/jvm/clojure/lang/ASeq.java#L228

Chase 2020-07-30T18:17:32.196500Z

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

phronmophobic 2020-07-30T18:17:51.196800Z

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

phronmophobic 2020-07-30T18:18:11.197Z

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

Chase 2020-07-30T18:18:25.197200Z

oh! Of course. eesh

phronmophobic 2020-07-30T18:18:49.197400Z

i did the same thing 😄

Chase 2020-07-30T18:19:25.197600Z

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

phronmophobic 2020-07-30T18:19:55.197800Z

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

phronmophobic 2020-07-30T18:20:40.198Z

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

phronmophobic 2020-07-30T18:21:12.198400Z

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

Chase 2020-07-30T18:21:37.198600Z

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

Chase 2020-07-30T18:21:50.198800Z

and then using .indexOf

2020-07-30T18:39:44.199Z

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

2020-07-30T18:39:54.199200Z

it doesn't borrow from any of the existing code

Chase 2020-07-30T18:45:00.199400Z

I would love to see whatever you've got

2020-07-30T18:48:34.199600Z

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

2020-07-30T18:49:24.199800Z

I was attempting maximum clojure-idiom-density :D

2020-07-30T18:49:35.200Z

should have written spec.alpha spec to round it out

2020-07-30T18:53:47.200200Z

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

2020-07-30T19:03:52.200400Z

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

Chase 2020-07-30T19:59:15.200600Z

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

phronmophobic 2020-07-30T20:00:36.200800Z

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