As I progress to more and more complex projects, I’ve begun to see a Clean Code problem arise. I can’t seem to find a good way to organize my event listeners. Once I get all the functionality up and running, they kinda look like crap. And they look like CRAP!!!
Especially the ones that have several different conditionals.
I try to keep things clean by putting a lot of the conditional logic into outside functions, but it still just seems like a mess to me.
Is this one of those things where you just have to accept that “clunky” is the way of the world?
Thanks for these snippets but it is not enough information to comment on the quality of your code. Taking two very short event handlers out of context doesn’t give us much to go on.
The only things that sort of raise a red flag for me are:
Putting the click handler on the document. This will capture a mouse click on any element on the page. Are you 100% sure that the target element is always what you are expecting it to be?
Why declare the container variable outside of the submit handler? Perhaps you are using it elsewhere? Again, I can’t know this because we don’t have enough context.
I might suggest using named handler functions instead of inline anonymous functions. A few inline anonymous handlers are fine for small bits of code but it can very quickly get out of hand for larger functions or when looping to attach the handlers.
Having named handlers also gives you better code organization. You do have to jump around in the code more to see what the functions do (but most editors let you Ctrl + left-click the function name to jump to the definition).
function handleFormSubmit(e) {
e.preventDefault();
createBookFromModal(container, formElement);
toggleModal();
}
formElement.addEventListener("submit", handleFormSubmit);
In very large projects you might need some abstractions/APIs but then it might also be time to think about using a framework instead.
Please be aware, this forum is a support for the freecodecamp curriculum, as Odin’s forum etc are for theirs.
That said, being familiar with Odin and beinga part of the Clean Code book reading group, i get where you’re starting from and trying to get to. As @lasjorg poipointed out, using a named function if both cleaner, and self documenting.
Further, this is a project growing in complexity. One of the clean code principles, and a good general guide, is to make small, simple, reusable functions, ones that can be applied in other places, and build your larger function by combining those smaller parts. I’m seeing a lot of that being applied, and it’s a good start.
One thing I would suggest is that you take advantage of JS modules and put the Book code into a separate file. Being a classically trained OOP guy I would also recommend you use the JS class syntax to define a Book class, as I think this will make your code even cleaner. For example
uniqueId(book)
becomes
book.id
Granted, you can do this without the class syntax but I think if you are going to use OOP principles you might as well use the syntax too
Also, I think the toggleRead function could use some work. Instead of using class names to determine the read state of a book why not just use the read property on the book? Then you wouldn’t have to pass in those last two parameters. A book should know if it has been read or not because it is storing that property, so it should be the final arbiter on its read state. Also, I would use a value for read that can be used as a boolean. That way you can do
if (book.read) {...}
Personally, I would be tempted to move a lot more into the Book class. For example, I think a Book might know how to render itself on a page. But of course there is always more than one way to do things.
As for your original issue with the event listeners, yes, now that I see everything I think I understand why you think they are clunky. You are using event delegation to handle the click event, which is perfectly fine, but you are deviating a little from how it usually works.
Usually in the event handler you would look at the event target and then call the necessary functions based on the target. Instead, you are always calling all of the functions in the handler and passing in a lot of extra information to those functions so that the functions can determine if they need to do something. All that extra information you have to pass into those functions does seem clunky. I would definitely change it around so the handler is making the decision what function to call based on the event target. It will make it much clearer what the handler is doing and will clean up your code.
And if you want to take a more OOP approach, you could create a Book container class to store the books in and you could put a click handler on that class to handle just the read and delete buttons on each book. Your still taking advantage of event delegation but now your event handler will be cleaner because you only need to handle two distinct actions. Again, I like to think in objects so my suggestions are biased toward OOP.
Not a problem for me, my time is invested in helping y’all here as much as i can. My point was that you may find a different response on the Odin forums and discord, as they’re versed in the particulars of Clean Code (am excellent book, and one i highly recommend for those getting deeper into complex coding).
But @bbsmooth gave some great guidance, and I’ll likely write about some of this in something article-related soon lol
