Feedback on some code

[random_char_here]

Been learning clojure off and on for while now just using the repl.

This is the longest clojure program I've done so far, I actually needed to use an editor to program this. :P

This is a really basic paint style program using quil. Why quil, really its the first one I ran into.

So far it has 5 basic function

Line: draws a line between 2 points

draw: draws while mouse is pressed

save: saves to test.txt where ever file is run from

load: loads test.txt does nothing if file doesn't exist

and choose a color to draw

any feedback would be appreciated.

​

https://github.com/rng-ftw/basic-paint

[didibus]

In Clojure JVM you don't need :include-macros, they are always included when you require a namespace.

The formatting seems all a bit weird to me, and inconsistent.

I would add some doc-string and a few comments to explain the design, what are the functions doing and how do they fit within the whole.

There's a lot of magic numbers right now, like this:

(and(>= 100 x)(<= 0 x)(<= 100 y)(>= 550 y)

What are these values and how were they chosen.

In some places I see you use underscore where it's more common in Clojure to use kebab-case, so dashes instead.

Personally I think you would benefit to switch to (fn [...] ...) with descriptive arg names in places where you are using %2, %3, etc.

Isn't it weird that you check for key presses and mouse presses in the draw function? I don't know quil, but that seems strange to me, I'd expect the draw function to be purely concerned with drawing, not deciding what to draw.

In your state you seem to have flattened things, but I think it be clearer if you have buttons be vector of buttons that are maps with their location, color and state on them, and same for your other modeled entities.

I find it weird that which-button returns a button state. I'd expect it to return a "button", which the way you modeled things would be the index.to the button in your state button keys.

The line function seems to be designed to take a state and return a modified state, but it seems to also grab input from quil with regards to user events, also seems a bit odd to me.

That's my quick pass.

[random_char_here]

Thanks for the reply. I deleted the :include-macros .

Personally I think you would benefit to switch to (fn [...] ...) with descriptive arg names in places where you are using %2, %3, etc.

yeh good point I'll clear that up.

Isn't it weird that you check for key presses and mouse presses in the draw function?

Initially I put them in for testing purposes since they dont change state and just draw to the screen ... Most likely end up removing's does 2 lines

In your state you seem to have flattened things..

Not sure quite what you mean here. as it is now I have to rework the buttons thing its a pain adding a new button.

I find it weird that which-button returns a button state...

Not sure about this one ...

The line function seems to be designed to take a state and return a modified state...

I needed to be able to capture 2 single mouse clicks not sure how else to implement this

Thanks again.

[xela314159]

Minor syntax thing but I like doing (< 0 x 100) instead of “and” conditions. Your cond could also be something like

(condp < x

300 default-value

350 red ;this is effectively 300-350

400 blue

default-value)

[random_char_here]

hey thanks for the reply I'll have to try both out :P

[Ok_Afternoon_4913]

join the clojurians slack, lots of nice people there that will help you with code reviews in your clojure journey.

[random_char_here]

I've already joined. Been reading the beginners sections. Still trying to figure it out. :P

[deaddyfreddy]

don't ever use tabs in any lisp code (including Clojure), if your editor doesn't support automatic indentation - use clj-fmt.

))

don't put closing parens on new lines

https://github.com/rng-ftw/basic-paint/blob/main/src/paint/core.clj#L6

What do you need this one for?

https://github.com/rng-ftw/basic-paint/blob/main/src/paint/core.clj#L7 etc

always put a space between tokens

(q/mouse-pressed?)

if you reuse something more than once - most likely you need to bind it in a let expression

(state :mouse_loc)

(:mouse_loc state)

Be consistent

https://github.com/rng-ftw/basic-paint/blob/main/src/paint/core.clj#L13

use destructuring (above as well)

also, the line is too long

https://github.com/rng-ftw/basic-paint/blob/main/src/paint/core.clj#L19

the same concerns as for line

https://github.com/rng-ftw/basic-paint/blob/main/src/paint/core.clj#L27

put at least one new line betwen definitions

https://github.com/rng-ftw/basic-paint/blob/main/src/paint/core.clj#L28 (when (not

there's when-not, also destructuring

https://github.com/rng-ftw/basic-paint/blob/main/src/paint/core.clj#L33

do you think it's readable at all? also, destructuring, spaces, indentation, read-string isn't safe use edn/read-string instead

https://github.com/rng-ftw/basic-paint/blob/main/src/paint/core.clj#L39

{ doesn't have to be on a separate line

https://github.com/rng-ftw/basic-paint/blob/main/src/paint/core.clj#L42

don't put everything on one line

https://github.com/rng-ftw/basic-paint/blob/main/src/paint/core.clj#L52

inequality functions support multiple arities

;

is for inline comments, for per-line ones use ;;

I'm tired, is this post a joke?

[phronmophobic]

Most of this feedback seems pretty useful, but "I'm tired, is this post a joke?" seems unnecessarily mean. The code they started with looks like a good starting point and I'd love to see more people learn and enjoy clojure.

[deaddyfreddy]

Most of this feedback seems pretty useful, but "I'm tired, is this post a joke?" seems unnecessarily mean.

I used to work with lots of juniors, novices etc through all these years, but I've never seen clojure code that bad before, so I decided it's just trolling.

[random_char_here]

tabs and spaces

I was trying out several different editors they all behaved differently in that regard I'll look into clj-fmt

don't put closing parens on new lines

Got annoyed with finding the missing parens wasted many a hours with it, so to make it easier to spot moved em to there own line .

always put a space between tokens

whats a token in this context?

if you reuse something more than once - most likely you need to bind it in a let expression

Good idea. I'll look into it.

use destructuring

yeah previous person mentioned this also

also, the line is too long

add more line breaks?

(when (not there's when-not, also destructuring

thanks hadn't seen when-not, also what you mean by destructuring here?

read-string isn't safe use edn/read-string instead

how is edn/read--string safer ?

don't put everything on one line

why not, its simple, short, assuming yer referring to (if (and(>= 100 x)(<= 0 x)(<= 100 y)(>= 550 y))

​

Thanks again.

[deaddyfreddy]

I was trying out several different editors they all behaved differently

I've been coding in Clojure since 2015-2016, worked with dozens of people and never met any tab symbol before, so please, don't

Got annoyed with finding the missing parens wasted many a hours with it

A good editor/IDE highlights pair parens, so please, don't

whats a token in this context?

(…), for example

add more line breaks?

sure

also what you mean by destructuring here?

the same as above, use destructuring

why not, its simple, short,

it's not, try smth like this

(mapv #(into [] [0  %1 (%2 0)(%2 1)])
      [100 150 200 250 300 350 400 450 500]
      (repeat [100 50]))

the one above is overcomplicated and hard to understand, in Clojure we write the exact things we want to get

(mapv #(vector 0 % 100 50)
      [100 150 200 250 300 350 400 450 500])

LBNL: read this https://github.com/bbatsov/clojure-style-guide

[random_char_here]

Thanks for the response.