Event dispatch improvements #1
Conversation
|
Oh, this might not fit your style guidelines. I can fix it tomorrow. |
src/disclojure/core.clj
Outdated
| (:require | ||
| [disclojure.gateway :as gw] | ||
| [disclojure.cache :as cache])) | ||
| (:require [clojure.string :as str] |
There was a problem hiding this comment.
Generally speaking, I prefer using the existing Java methods over adding an extra import when it comes to string manipulation. Nothing particularly wrong with it, but it's a bit more concise and doesn't hurt readability IMO.
Being idiomatic is great, but if it's only being used in internal code (for example, string manipulation inside the dispatch method) as opposed to being something exposed to potential callers (for example, the dispatch method itself), I'm more concerned with conciseness and readability than with being idiomatic. If there's a distinct benefit beyond personal opinion (performance, a significant difference in LOC, etc.), then feel free to let me know - otherwise, I'd prefer to avoid these sorts of changes.
There was a problem hiding this comment.
Makes sense. I'm not aware of any other benefit, no, so...Java interop it is.
src/disclojure/core.clj
Outdated
| (= (% :event) :any) | ||
| (= (or (-> % :event event-aliases) (% :event)) event-name)) | ||
| (@client :listeners))] | ||
| (when (pos? (count listeners)) |
There was a problem hiding this comment.
Not really sure what this line is for... What does it add to the process that was missing before? Especially considering it forces the extra let statement on the next line.
There was a problem hiding this comment.
My reasoning was that if there are no listeners for this event, might as well avoid the extra work of accessing the cache, struct creation, etc.
There was a problem hiding this comment.
Ah, okay. Is there a way we can short-circuit that without splitting the let in two?
There was a problem hiding this comment.
Hm. Could do an if for every definition after that. That's all I can think of.
There was a problem hiding this comment.
Blech... I'll take a look at it when I have a few minutes, see if there's anything to be done there.
Backported from my event-rewrite branch.
|
I should mention, with this latest commit, one of the new functions I added I made public, since I feel like it could have use to users. Perhaps this would be more suitable to a new file and ns for utility functions; perhaps a |
Changed
dispatchto be more efficient, readable, and idiomatic. Also fixed a cache retrieval bug indispatch.Note: this hasn't been tested yet.