Coder, writer, nonconformist 🤘
Ex-SWE @ Facebook 🤩
Sharing experience and notes on building a career, sharpening coding skills and creating great things 🏆
I'm not sure about "do-anything" pieces of code because in my experience their existence usually means I was (or am) too lazy to encapsulate them into something solid like a plugin, component or library. So what's actually so wrong with this piece of code?
You have an implicit dependency here, jQuery. It's not so bad per se but the fact you don't specify it explicitly may hurt. An inexperienced developer or just anyone who faces, say, 10 pieces like that in an unfamiliar to them codebase might get confused about why something doesn't work or work in a way they don't expect and would have to deal with debugging something they don't have an idea about.
Your dependency version is unspecified. If someone updates jQuery (or even disables) on your website you might end up debugging a whole bunch of spread out pieces of code - each of which could break in a different way because they could've been written by different people in different manners and depend on different versions of jQuery.
You spread your proto-component intestines all over markup, styles and javascript. It's almost pure evil because once it becomes a little bit more complex you're screwed. You never really understand what is this particular class name in HTML stands for as well as why this style is what it is. I'm exaggerating a little but only a little. Again, imagine a junior developer dealing with a dozen pieces like that within an unfamiliar to them codebase.
What if you accidentally execute this code twice? I'm not pretty sure everything will still work fine. This mistake might easily happen if you start refactoring or renovating your code. And there's no way now for you to prevent this from happening because you deal with your dependencies in a reckless manner.
Why would you need jQuery for such a simple task? You can use all modern CSS-selectors with document.querySelector and querySelectorAll. I know, dealing with class names is a more tricky task but I'm sure you can handle it in a better way than downloading a huge script just for a couple methods :-)
Global CSS-classes are unsafe and should be avoided. No one promises that you'll never forget about even your own naming convention and won't try to hook another functionality or appearance on the same c-reveal or c-active class.
There's no documentation or, even better, a concise interface which produces no confusion and mistakes. Messing up with a bunch of classes and a link (why is it even a link if it doesn't work as a link? Make it a span instead - and you won't need a preventDefault invocation) that depend on some implicit behavior of a function you won't even necessarily notice... Well, man, it's bad.
Again, I'm exaggerating and I'm sorry for criticizing; this particular piece of code must be almost harmless. Just trying to help and express an and up-to-date point of view on how to make your codebase neater.
How to make this code better
Isolate it into a component/plugin with proper versioning and include it as a commonly shaped module whether it is a CommonJS module and Babel/Browserify, ES6 imported module, UMD or whatever else.
Thanks for the response man. I don't mind the criticism at all, this is after all dev explicit space and I for one am on here to learn. I didn't expect the comment at all though, and now I kind of wish I used a different, more safe example to my question!
That snippet of code is at least 4 years old now, and was written when I was new to programming as a whole, nevermind javascript (I come from a design background). It has simply always worked for me since then, and I saw little need to change it since then.
That's not justification at all, I simply mention it because if I were to read your comment a year ago I would not have understood much of what you just said, probably deleted my account and hid under a couch for a week!
If you don't mind I would like to respond to your points, explain the thinking process and decisions I made, and I would hate for you to see them as arrogant or confrontational. I am genuinely interested in your opinions.
I think my response is too long, so breaking it up in bits.
If I click element A I want to add a class to element B. That's basically all this script does.
I have three problems to solve with this script:
1. The script shouldn't run on all links on a page, only the ones I wanted to attach the feature to.
Way back when I originally wrote this the only way I knew how to solve the first problem was to add a class to the link I want to the script to listen on. If I recall correctly, this was fairly common practise?
I decided that class should be c-reveal because I named this script Reveal.
2. I have to figure out how to target element B without having to declare it in js.
I figured using the html anchor method would be quite cool. The user clicks on something and goes to an element in a way, and I was able to get that anchor into myscript and match it to element B.
3. I need to be able to have multiple features on a single page using this script.
I didn't want to have the script as a function that I would have to call for every feature. I wanted it to just sit there and wait for a click and then toggle out a class.
The only way I knew how to achieve this was to use a global class. Global css is awful yes, but it was a compromise that up to today I've managed to deal with. lol.
Thinking about this now, I guess I could do something like:
<buttondata-reveal="#hidden-div">show the hidden div</button><divclass="feature"data-reveal-class="feature--show"id="hidden-div">This div's default state is hidden through .feature.</div>
This removes any global classes, but still allows me to target it in a reusable way. The script would then look for the data-reveal rather than the class.
Does this make more sense to you?
You will end up writing a lot of similar css code per feature, but I guess you can mitigate a lot of that with a sass mixin or something.
To be honest, this is the first time in a long time I look at that code. It just always worked so why fix a working thing amirite?! Lol.
On that note
While this was so not the type of response I expected I do appreciate that you took the time to do it man.
At the very least it's given me a bit of a push to go relook at quite a few other snippets and rework them.
If you would be kind enough to respond to my thoughts above it would be awesome, even if it's just to continue the discussion!
I'm not going to change my original post, because that would make this thread irellevant, and I think it's important to keep it in tact.
That said: Your most versatile bit of code could also be your most versatile module, plugin or other similar form of $thing! Mind sharing yours?
Coder, writer, nonconformist 🤘
Ex-SWE @ Facebook 🤩
Sharing experience and notes on building a career, sharpening coding skills and creating great things 🏆
Now I feel obligated to explain myself. I got used to a criticizing manner of giving feedback because of my last team leader job - now I guess it might be not the best way to encourage people for changes because critics simply hurt. I didn't mean to do that to you at all; so I highly appreciate your thorough response. Without any doubts, you're one hell of a great developer, and I hope my reasoning only helps. Being able to perceive things from another standpoint is an incredibly rare skill, keep it and you'll benefit from it much more than the criticism might ever hurt you.
I'd like to make myself as clear as possible. The step-by-step answer is following below.
1. The script shouldn't run on all links on a page, only the ones I wanted to attach the feature to.
Way back when I originally wrote this the only way I knew how to solve the first problem was to add a class to the link I want to the script to listen on. If I recall correctly, this was fairly common practise?
I decided that class should be c-reveal because I named this script Reveal.
I get your idea and I was writing code like that for a long time. I might even write it now under certain circumstances like, for example, supporting a website that has been made long ago. And yes, I can confirm that it was a fairly common practice.
Your point number 1 is absolutely valid. However, the approach of hooking javascript logic to CSS classes has at least one downside. Even though you explicitly define the purpose of the elements in HTML by setting these classes, you still need an implicitly required javascript snippet. These are two points of failure: you can either forget about the class or get the script lost for some reason like changing the head/footer part of your template where all the dependencies are included via <script>. What I'd suggest is a reduction of this excessive construction to a single plugin/function invocation in JavaScript with all required hooks (classes, data attributes or whatever else) specified. Again, this particular example is too simple to realize how severely it might affect the future debugging process or feasibility of altering the functionality or appearance.
2. I have to figure out how to target element B without having to declare it in js.
I figured using the html anchor method would be quite cool. The user clicks on something and goes to an element in a way, and I was able to get that anchor into myscript and match it to element B.
Given that you've accepted the approach of reducing your plugin invocation to a one, supposedly javascript, point, you might just wrap both of your elements into another one and target your script to it. Like this:
<div class="revealer">
<span>Click me</span>
<div><!-- something being hidden and shown --></div>
</div>
In that case, you have a whole bunch of options of how to link elements to each other without using an anchor, which is in my opinion much better.
3. I need to be able to have multiple features on a single page using this script.
I didn't want to have the script as a function that I would have to call for every feature. I wanted it to just sit there and wait for a click and then toggle out a class.
The only way I knew how to achieve this was to use a global class. Global css is awful yes, but it was a compromise that up to today I've managed to deal with. lol.
Again, you still have to invoke your script by including it into a page and running unconditionally even in case you don't need it. If you could get rid of this step I'd accept the point, otherwise, you just increase the complexity of the solution (that will also affect the ability to comprehend what's going on) by making part of it covert.
While this was so not the type of response I expected I do appreciate that you took the time to do it man.
[...cut...]
That said: Your most versatile bit of code could also be your most versatile module, plugin or other similar form of $thing! Mind sharing yours?
I guess I was too precise in listing my points and failed to convey the main idea. The concept of having copy-and-pastable pieces of code without any boundaries (i.e. lacking isolation/incapsulation), which drift from project to project, looks absolutely wrong to me. As I wrote before, if I had a reusable piece of code I'd rather make it a module/plugin/component - with documentation, versioning, proper isolation and interface than just carry it around as a bunch of js, css and obscure conventions about class names or anything else.
An example might be a simple PubSub module I used in my previous project. It's not related to DOM manipulations at all but even if it was, I wouldn't change the way I shaped it. Another example might be a detect-media function from my last hobby project. If I need it somewhere else I will just make an npm-module (or a module of any other type), publish it somewhere and then reuse.
For sure, this approach requires more infrastructure that your project might not have at the moment. However, even with just plain scripts attached to a page the of carrying useful stuff around can be much neater and apparent.
For further actions, you may consider blocking this person and/or reporting abuse
We're a place where coders share, stay up-to-date and grow their careers.
I'm not sure about "do-anything" pieces of code because in my experience their existence usually means I was (or am) too lazy to encapsulate them into something solid like a plugin, component or library. So what's actually so wrong with this piece of code?
You have an implicit dependency here, jQuery. It's not so bad per se but the fact you don't specify it explicitly may hurt. An inexperienced developer or just anyone who faces, say, 10 pieces like that in an unfamiliar to them codebase might get confused about why something doesn't work or work in a way they don't expect and would have to deal with debugging something they don't have an idea about.
You spread your proto-component intestines all over markup, styles and javascript. It's almost pure evil because once it becomes a little bit more complex you're screwed. You never really understand what is this particular class name in HTML stands for as well as why this style is what it is. I'm exaggerating a little but only a little. Again, imagine a junior developer dealing with a dozen pieces like that within an unfamiliar to them codebase.
Why would you need jQuery for such a simple task? You can use all modern CSS-selectors with
document.querySelectorandquerySelectorAll. I know, dealing with class names is a more tricky task but I'm sure you can handle it in a better way than downloading a huge script just for a couple methods :-)Global CSS-classes are unsafe and should be avoided. No one promises that you'll never forget about even your own naming convention and won't try to hook another functionality or appearance on the same
c-revealorc-activeclass.There's no documentation or, even better, a concise interface which produces no confusion and mistakes. Messing up with a bunch of classes and a link (why is it even a link if it doesn't work as a link? Make it a
spaninstead - and you won't need apreventDefaultinvocation) that depend on some implicit behavior of a function you won't even necessarily notice... Well, man, it's bad.Again, I'm exaggerating and I'm sorry for criticizing; this particular piece of code must be almost harmless. Just trying to help and express an and up-to-date point of view on how to make your codebase neater.
How to make this code better
Isolate it into a component/plugin with proper versioning and include it as a commonly shaped module whether it is a CommonJS module and Babel/Browserify, ES6 imported module, UMD or whatever else.
Hey Oleg,
Thanks for the response man. I don't mind the criticism at all, this is after all dev explicit space and I for one am on here to learn. I didn't expect the comment at all though, and now I kind of wish I used a different, more safe example to my question!
That snippet of code is at least 4 years old now, and was written when I was new to programming as a whole, nevermind javascript (I come from a design background). It has simply always worked for me since then, and I saw little need to change it since then.
That's not justification at all, I simply mention it because if I were to read your comment a year ago I would not have understood much of what you just said, probably deleted my account and hid under a couch for a week!
If you don't mind I would like to respond to your points, explain the thinking process and decisions I made, and I would hate for you to see them as arrogant or confrontational. I am genuinely interested in your opinions.
I think my response is too long, so breaking it up in bits.
Coding methodology and directions
If I click
element AI want to add a class toelement B. That's basically all this script does.I have three problems to solve with this script:
1. The script shouldn't run on all links on a page, only the ones I wanted to attach the feature to.
Way back when I originally wrote this the only way I knew how to solve the first problem was to add a class to the link I want to the script to listen on. If I recall correctly, this was fairly common practise?
I decided that class should be
c-revealbecause I named this script Reveal.2. I have to figure out how to target
element Bwithout having to declare it in js.I figured using the html anchor method would be quite cool. The user clicks on something and goes to an element in a way, and I was able to get that anchor into myscript and match it to
element B.3. I need to be able to have multiple features on a single page using this script.
I didn't want to have the script as a function that I would have to call for every feature. I wanted it to just sit there and wait for a click and then toggle out a class.
The only way I knew how to achieve this was to use a global class. Global css is awful yes, but it was a compromise that up to today I've managed to deal with. lol.
Thinking about this now, I guess I could do something like:
This removes any global classes, but still allows me to target it in a reusable way. The script would then look for the data-reveal rather than the class.
Does this make more sense to you?
You will end up writing a lot of similar css code per feature, but I guess you can mitigate a lot of that with a sass mixin or something.
To be honest, this is the first time in a long time I look at that code. It just always worked so why fix a working thing amirite?! Lol.
On that note
While this was so not the type of response I expected I do appreciate that you took the time to do it man.
At the very least it's given me a bit of a push to go relook at quite a few other snippets and rework them.
If you would be kind enough to respond to my thoughts above it would be awesome, even if it's just to continue the discussion!
I'm not going to change my original post, because that would make this thread irellevant, and I think it's important to keep it in tact.
That said: Your most versatile bit of code could also be your most versatile module, plugin or other similar form of
$thing! Mind sharing yours?Adrian,
Now I feel obligated to explain myself. I got used to a criticizing manner of giving feedback because of my last team leader job - now I guess it might be not the best way to encourage people for changes because critics simply hurt. I didn't mean to do that to you at all; so I highly appreciate your thorough response. Without any doubts, you're one hell of a great developer, and I hope my reasoning only helps. Being able to perceive things from another standpoint is an incredibly rare skill, keep it and you'll benefit from it much more than the criticism might ever hurt you.
I'd like to make myself as clear as possible. The step-by-step answer is following below.
I get your idea and I was writing code like that for a long time. I might even write it now under certain circumstances like, for example, supporting a website that has been made long ago. And yes, I can confirm that it was a fairly common practice.
Your point number 1 is absolutely valid. However, the approach of hooking javascript logic to CSS classes has at least one downside. Even though you explicitly define the purpose of the elements in HTML by setting these classes, you still need an implicitly required javascript snippet. These are two points of failure: you can either forget about the class or get the script lost for some reason like changing the head/footer part of your template where all the dependencies are included via
<script>. What I'd suggest is a reduction of this excessive construction to a single plugin/function invocation in JavaScript with all required hooks (classes, data attributes or whatever else) specified. Again, this particular example is too simple to realize how severely it might affect the future debugging process or feasibility of altering the functionality or appearance.Given that you've accepted the approach of reducing your plugin invocation to a one, supposedly javascript, point, you might just wrap both of your elements into another one and target your script to it. Like this:
In that case, you have a whole bunch of options of how to link elements to each other without using an anchor, which is in my opinion much better.
Again, you still have to invoke your script by including it into a page and running unconditionally even in case you don't need it. If you could get rid of this step I'd accept the point, otherwise, you just increase the complexity of the solution (that will also affect the ability to comprehend what's going on) by making part of it covert.
I guess I was too precise in listing my points and failed to convey the main idea. The concept of having copy-and-pastable pieces of code without any boundaries (i.e. lacking isolation/incapsulation), which drift from project to project, looks absolutely wrong to me. As I wrote before, if I had a reusable piece of code I'd rather make it a module/plugin/component - with documentation, versioning, proper isolation and interface than just carry it around as a bunch of js, css and obscure conventions about class names or anything else.
An example might be a simple PubSub module I used in my previous project. It's not related to DOM manipulations at all but even if it was, I wouldn't change the way I shaped it. Another example might be a detect-media function from my last hobby project. If I need it somewhere else I will just make an npm-module (or a module of any other type), publish it somewhere and then reuse.
For sure, this approach requires more infrastructure that your project might not have at the moment. However, even with just plain scripts attached to a page the of carrying useful stuff around can be much neater and apparent.