Skip to content

Commit 4b4e300

Browse files
csswizardrycsswizardry
committed
Add new Code Smells article
1 parent 1c284d6 commit 4b4e300

3 files changed

Lines changed: 370 additions & 3 deletions

File tree

‎_config.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ description: "Write an awesome description for your new site here. You can edit
55
baseurl: ""
66
url: "https://csswizardry.com"
77
future: true
8-
sw: "0008"
8+
sw: "0009"
99

1010
# Build settings
1111
markdown: kramdown

‎_posts/2017-02-08-code-smells-in-css-revisited.md‎

Lines changed: 367 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,367 @@
1+
---
2+
layout: post
3+
title: "Code Smells in CSS Revisited"
4+
date: 2017-02-08 12:14:18
5+
categories: Web Development
6+
meta: "An update to my 2012 article, Code Smells in CSS"
7+
---
8+
9+
Way back in 2012, I wrote a post about potential CSS anti-patterns called
10+
[<cite>Code Smells in CSS</cite>](/2012/11/code-smells-in-css/). Looking back on
11+
that piece, I still agree with all of it even four years later, but I do have
12+
some new things to add to the list. Again, these aren’t necessarily always bad
13+
things, hence referring to them as code smells: they might be perfectly
14+
acceptable in your use case, but they still smell kinda funny.
15+
16+
Before we start, then, let’s remind ourselves what a Code Smell actually is.
17+
From [Wikipedia](https://en.m.wikipedia.org/wiki/Code_smell) (emphasis mine):
18+
19+
> Code smell, also known as bad smell, in computer programming code, refers to
20+
> any symptom in the source code of a program that **possibly indicates a deeper
21+
> problem**. According to Martin Fowler, ‘a code smell is a surface indication
22+
> that usually corresponds to a deeper problem in the system’. Another way to
23+
> look at smells is with respect to principles and quality: ‘smells are certain
24+
> structures in the code that indicate **violation of fundamental design
25+
> principles** and negatively impact design quality’. Code smells are usually
26+
> not bugs—**they are not technically incorrect** and do not currently prevent
27+
> the program from functioning. Instead, **they indicate weaknesses in design
28+
> that may be slowing down development** or increasing the risk of bugs or
29+
> failures in the future. Bad code smells can be an indicator of factors that
30+
> contribute to technical debt. Robert C. Martin calls a list of code smells a
31+
> ‘value system’ for software craftsmanship.
32+
33+
So they’re not technically, always wrong, they’re just a good litmus test.
34+
35+
## `@extend`
36+
37+
Hopefully I can keep this first one nice and brief: I have long been vocal about
38+
the side effects and pitfalls of `@extend`, and now I would actively consider it
39+
a code smell. It’s not absolutely, always, definitely bad, but it usually is.
40+
Treat it with suspicion.
41+
42+
The problems with `@extend` are manifold, but to summarise:
43+
44+
* **It’s actually worse for performance than mixins are.** Gzip favours
45+
repetition, so CSS files with greater repetition (i.e. mixins) achieve a
46+
greater compression delta.
47+
* **It’s greedy.** Sass’ `@extend` will `@extend` every instance of a class that
48+
it finds, giving us crazy-long selector chains [that look like
49+
this](https://twitter.com/gaelmetais/status/564109775995437057).
50+
* **It moves things around your codebase.** Source order is vital in CSS, so
51+
moving selectors around your project should always be avoided.
52+
* **It obscures the paper-trail.** `@extend` hides a lot of complexity in your
53+
Sass that you need to unpick more gradually, whereas the multiple class
54+
approach puts all of the information front-and-center in your markup.
55+
56+
For further reading:
57+
58+
* [<cite>Mixins Better for
59+
Performance</cite>](/2016/02/mixins-better-for-performance/)
60+
* [<cite>When to Use `@extend`; When to Use a
61+
Mixin</cite>](/2014/11/when-to-use-extend-when-to-use-a-mixin/)
62+
* [<cite>Extending Silent Classes in
63+
Sass</cite>](/2014/01/extending-silent-classes-in-sass/)
64+
65+
## String Concatenation for Classes
66+
67+
Another Sass-based gripe is the use of the `&` to concatenate strings in your
68+
classes, e.g.:
69+
70+
```
71+
.foo {
72+
color: red;
73+
74+
&-bar {
75+
font-weight: bold;
76+
}
77+
78+
}
79+
```
80+
81+
Which yields:
82+
83+
```
84+
.foo {
85+
color: red;
86+
}
87+
88+
.foo-bar {
89+
font-weight: bold;
90+
}
91+
```
92+
93+
The obvious benefit of this is its terseness: the fact that we have to write our
94+
`foo` namespace only once is certainly very DRY.
95+
96+
One less obvious downside, however, is the fact that the string `foo-bar` now no
97+
longer exists in my source code. Searching my codebase for `foo-bar` will
98+
return only results in HTML (or compiled CSS if we’ve checked that into our
99+
project). It suddenly became a lot more difficult to locate the source of
100+
`.foo-bar`’s styles.
101+
102+
I am much more a fan of writing my CSS longhand: on balance, I am far more
103+
likely to look for a class than I am to rename it, so findability wins for me.
104+
If I join a project making heavy use of Sass’ string concatenation, I’m usually
105+
expecting to have a hard time tracking things down.
106+
107+
Of course you could argue that sourcemaps will help us out, or that if I’m
108+
looking for a class of `.nav__item` then I simply need to open the `nav.scss`
109+
file, but unfortunately that’s not always going to help. For a little more
110+
detail, I made [a screencast](https://www.youtube.com/watch?v=MGzoRM3Al40) about
111+
it.
112+
113+
## Background Shorthand
114+
115+
Something else I discussed only recently is the use of `background` shorthand
116+
syntax. For full details, please refer to [the relevant
117+
article](/2016/12/css-shorthand-syntax-considered-an-anti-pattern/), but the
118+
summary here is that using something like:
119+
120+
```
121+
.btn {
122+
background: #f43059;
123+
}
124+
```
125+
126+
…when you probably meant:
127+
128+
```
129+
.btn {
130+
background-color: #f43059;
131+
}
132+
```
133+
134+
…is another practice I would consider a code smell. When I see the former being
135+
used, it is seldom what the developer actually intended: nearly every time they
136+
really meant the latter. Where the latter _only_ sets or modifies a background
137+
colour, the former will also reset/unset background images, positions,
138+
attachments, etc.
139+
140+
Seeing this in CSS projects immediately warns me that we could end up having
141+
problems with it.
142+
143+
## Key Selector Appearing More Than Once
144+
145+
The key selector is the selector that actually gets targeted/styled. It is
146+
often, though not always, the selector just before your opening curly brace
147+
(`{`). In the following CSS:
148+
149+
```
150+
.foo {}
151+
152+
nav li .bar {}
153+
154+
.promo a,
155+
.promo .btn {}
156+
```
157+
158+
…the key selectors are:
159+
160+
* `.foo`,
161+
* `.bar`,
162+
* `a`, and
163+
* `.btn` respectively.
164+
165+
If I were to take a codebase and [ack for
166+
`.btn`](/2017/01/ack-for-css-developers/), I might see some output like this:
167+
168+
```
169+
.btn {}
170+
171+
.header .btn,
172+
.header .btn:hover {}
173+
174+
.sidebar .btn {}
175+
176+
.modal .btn {}
177+
178+
.page aside .btn {}
179+
180+
nav .btn {}
181+
```
182+
183+
Aside from the fact that a lot of that is just generally pretty poor CSS, the
184+
problem I’m spotting here is that `.btn` is defined many times. This tells me
185+
that:
186+
187+
1. **there is no Single Source of Truth** telling me what buttons look like;
188+
2. **there has been a lot of mutation** meaning that the class `.btn` has many
189+
different potential outcomes, all via mutable CSS.
190+
191+
As soon as I see CSS like this, I’m aware of the fact that doing any work on
192+
buttons will have a large surface area, tracking down exactly where buttons’
193+
styles come from will be a lot more difficult, and that changing anything will
194+
likely have huge knock-on effects elsewhere. This is one of the key problems
195+
with mutable CSS.
196+
197+
Make use of something like BEM in order to create completely brand new classes
198+
that carry those changes, e.g.:
199+
200+
```
201+
.btn {}
202+
203+
.btn--large {}
204+
205+
.btn--primary {}
206+
207+
.btn--ghost {}
208+
```
209+
210+
Just one key selector each.
211+
212+
## A Class Appearing in Another Component’s File
213+
214+
On a similar but subtly different theme as above, the appearance of classes in
215+
other components’ files is indicative of a code smell.
216+
217+
Where the previous code smell deals with the question of there being more than
218+
one instance of the same key selector, this code smell deals with where those
219+
selectors might live. Take this question from [Dave
220+
Rupert](https://twitter.com/davatron5000):
221+
222+
<blockquote class="twitter-tweet" data-lang="en">
223+
<p lang="en" dir="ltr">
224+
<code>.some-context .thing { /* special rules and overrides */ }</code>
225+
<br />Does that go in thing.css or some-context.css?
226+
</p>
227+
— Dave Rupert (@davatron5000) <a href="https://twitter.com/davatron5000/status/829091851651149824">7 February, 2017</a>
228+
</blockquote>
229+
230+
If we need to style something differently because of its context, where should
231+
we put that additional CSS?
232+
233+
1. In the file that styles the thing?
234+
2. In the file that controls that context?
235+
236+
Let’s say we have the following CSS:
237+
238+
```
239+
.btn {
240+
[styles]
241+
}
242+
243+
.modal .btn {
244+
font-size: 0.75em;
245+
}
246+
```
247+
248+
Where should `.modal .btn {}` live?
249+
250+
It should live **in the `.btn` file.**
251+
252+
We should do our best to group our styles based on the subject (i.e. the key
253+
selector). In this example, the subject is `.btn`: that’s the thing we actually
254+
care about. `.modal` is purely a context for `.btn`, so we aren’t styling it at
255+
all. To this end, we shouldn’t move `.btn` styling out into another file.
256+
257+
The reason we shouldn’t do this is simply down to collocation: it’s much more
258+
convenient to have the context of all of our buttons in one place. If I want to
259+
get a good overview of all of the button styles in my project, I should expect
260+
to only have to open `_components.buttons.scss`, and not a dozen other files.
261+
262+
This makes it much easier to move all of the button styles onto a new project,
263+
but more importantly it eases cognitive overhead. I’m sure you’re all familiar
264+
with the feeling of having ten files open in your text editor whilst just trying
265+
to change one small piece of styling. This is something we can avoid.
266+
267+
Group your styles into files based on the subject: if it styles a button, no
268+
matter how it goes about it, we should see it in `_components.buttons.scss`.
269+
270+
As a simple rule of thumb, ask yourself the question <q>am I styling
271+
<var>x</var> or am I styling <var>y</var>?</q> If the answer is <var>x</var>,
272+
then your CSS should live in `x.css`; if the answer is <var>y</var>, it should
273+
live in `y.css`.
274+
275+
### BEM Mixes
276+
277+
Actually, interestingly, I wouldn’t write this CSS at all—I’d use a BEM mix—but
278+
that’s an answer to a different question. Instead of this:
279+
280+
```
281+
// _components.buttons.scss
282+
283+
.btn {
284+
[styles]
285+
}
286+
287+
.modal .btn {
288+
[styles]
289+
}
290+
291+
292+
// _components.modal.scss
293+
294+
.modal {
295+
[styles]
296+
}
297+
```
298+
299+
We’d have this:
300+
301+
```
302+
// _components.buttons.scss
303+
304+
.btn {
305+
[styles]
306+
}
307+
308+
309+
// _components.modal.scss
310+
311+
.modal {
312+
[styles]
313+
}
314+
315+
.modal__btn {
316+
[styles]
317+
}
318+
```
319+
320+
This third, brand new class would get applied to the HTML like this:
321+
322+
```
323+
<div class="modal">
324+
<button class="btn modal__btn">Dismiss</button>
325+
</div>
326+
```
327+
328+
This is called a BEM mix, in which we introduce a third brand new class to refer
329+
to a button belonging to a modal. This avoids the question of where things live,
330+
it reduces the specificity by avoiding nesting, and also prevents mutation by
331+
avoiding repeating the `.btn` class again. Magical.
332+
333+
## CSS `@import`
334+
335+
I would go as far as saying that CSS `@import` is not just a code smell, it’s an
336+
active bad practice. It poses a huge performance penalty in that it delays the
337+
downloading of CSS (which is a critical asset) until later than necessary. The
338+
(simplified) workflow involved in downloading `@import`ed CSS looks a little
339+
like:
340+
341+
1. Get the HTML file, which asks for a CSS file;
342+
2. Get the CSS file, which asks for another CSS file;
343+
3. Get the last CSS file;
344+
4. Begin rendering the page.
345+
346+
If we’d got that `@import` flattened into one single file, the workflow would
347+
look more like this:
348+
349+
1. Get the HTML file, which asks for a CSS file;
350+
2. Get the CSS file;
351+
3. Begin rendering the page.
352+
353+
If we can’t manage to smush all of our CSS into one file (e.g. we’re linking to
354+
Google Fonts), then we should use two `<link />` elements in our HTML instead of
355+
`@import`. While this might feel a little less encapsulated (it would be nicer
356+
to handle all of these dependencies in our CSS files), it’s still much better
357+
for performance:
358+
359+
1. Get the HTML file, which asks for two CSS files;
360+
2. Get both CSS files;
361+
3. Begin rendering the page.
362+
363+
- - -
364+
365+
So there we have a few additions to my previous piece on Code Smells. These are
366+
usually all things I have seen and suffered in the wild: hopefully now you can
367+
avoid them as well.

‎sw.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
var cacheName = 'csswizardry:0008';
1+
var cacheName = 'csswizardry:0009';
22
var cacheFiles = [
33
'/',
44
'/about/',
@@ -66,7 +66,7 @@ self.addEventListener('fetch', function(event) {
6666
// Empty out any caches that don’t match the ones listed.
6767
self.addEventListener('activate', function(event) {
6868

69-
var cacheWhitelist = ['csswizardry:0008'];
69+
var cacheWhitelist = ['csswizardry:0009'];
7070

7171
event.waitUntil(
7272
caches.keys().then(function(cacheNames) {

0 commit comments

Comments
 (0)