-
Notifications
You must be signed in to change notification settings - Fork 10
/
Copy pathrefactoring-tales.html
1345 lines (1298 loc) · 130 KB
/
refactoring-tales.html
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
467
468
469
470
471
472
473
474
475
476
477
478
479
480
481
482
483
484
485
486
487
488
489
490
491
492
493
494
495
496
497
498
499
500
501
502
503
504
505
506
507
508
509
510
511
512
513
514
515
516
517
518
519
520
521
522
523
524
525
526
527
528
529
530
531
532
533
534
535
536
537
538
539
540
541
542
543
544
545
546
547
548
549
550
551
552
553
554
555
556
557
558
559
560
561
562
563
564
565
566
567
568
569
570
571
572
573
574
575
576
577
578
579
580
581
582
583
584
585
586
587
588
589
590
591
592
593
594
595
596
597
598
599
600
601
602
603
604
605
606
607
608
609
610
611
612
613
614
615
616
617
618
619
620
621
622
623
624
625
626
627
628
629
630
631
632
633
634
635
636
637
638
639
640
641
642
643
644
645
646
647
648
649
650
651
652
653
654
655
656
657
658
659
660
661
662
663
664
665
666
667
668
669
670
671
672
673
674
675
676
677
678
679
680
681
682
683
684
685
686
687
688
689
690
691
692
693
694
695
696
697
698
699
700
701
702
703
704
705
706
707
708
709
710
711
712
713
714
715
716
717
718
719
720
721
722
723
724
725
726
727
728
729
730
731
732
733
734
735
736
737
738
739
740
741
742
743
744
745
746
747
748
749
750
751
752
753
754
755
756
757
758
759
760
761
762
763
764
765
766
767
768
769
770
771
772
773
774
775
776
777
778
779
780
781
782
783
784
785
786
787
788
789
790
791
792
793
794
795
796
797
798
799
800
801
802
803
804
805
806
807
808
809
810
811
812
813
814
815
816
817
818
819
820
821
822
823
824
825
826
827
828
829
830
831
832
833
834
835
836
837
838
839
840
841
842
843
844
845
846
847
848
849
850
851
852
853
854
855
856
857
858
859
860
861
862
863
864
865
866
867
868
869
870
871
872
873
874
875
876
877
878
879
880
881
882
883
884
885
886
887
888
889
890
891
892
893
894
895
896
897
898
899
900
901
902
903
904
905
906
907
908
909
910
911
912
913
914
915
916
917
918
919
920
921
922
923
924
925
926
927
928
929
930
931
932
933
934
935
936
937
938
939
940
941
942
943
944
945
946
947
948
949
950
951
952
953
954
955
956
957
958
959
960
961
962
963
964
965
966
967
968
969
970
971
972
973
974
975
976
977
978
979
980
981
982
983
984
985
986
987
988
989
990
991
992
993
994
995
996
997
998
999
1000
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<meta name="generator" content="pandoc">
<meta name="viewport" content="width=device-width, initial-scale=1.0, user-scalable=yes">
<title>The Refactoring Tales - </title>
<style type="text/css">code{white-space: pre;}</style>
<!--[if lt IE 9]>
<script src="http://html5shim.googlecode.com/svn/trunk/html5.js"></script>
<![endif]-->
<style type="text/css">
table.sourceCode, tr.sourceCode, td.lineNumbers, td.sourceCode {
margin: 0; padding: 0; vertical-align: baseline; border: none; }
table.sourceCode { width: 100%; line-height: 100%; }
td.lineNumbers { text-align: right; padding-right: 4px; padding-left: 4px; color: #aaaaaa; border-right: 1px solid #aaaaaa; }
td.sourceCode { padding-left: 5px; }
code > span.kw { color: #007020; font-weight: bold; }
code > span.dt { color: #902000; }
code > span.dv { color: #40a070; }
code > span.bn { color: #40a070; }
code > span.fl { color: #40a070; }
code > span.ch { color: #4070a0; }
code > span.st { color: #4070a0; }
code > span.co { color: #60a0b0; font-style: italic; }
code > span.ot { color: #007020; }
code > span.al { color: #ff0000; font-weight: bold; }
code > span.fu { color: #06287e; }
code > span.er { color: #ff0000; font-weight: bold; }
</style>
<link rel="stylesheet" href="style.css">
</head>
<body>
<p>
<h1>The Refactoring Tales</h1>
<h3>By Jack Franklin (<a href="http://twitter.com/jack_franklin">@jack_franklin</a>)</h3>
<p>This book is freely available to read online, but you can purchase PDF, MOBI and EPUB versions for a small fee to help support The JavaScript Playground. <a href="http://javascriptplayground.com/the-refactoring-tales/">Find out more and purchase digital download editions here.</a></p>
<p>The book is also not yet fully completed, and more content is to come very soon. <a href="http://twitter.com/jsplayground_">Follow The JavaScript Playground on Twitter</a> to keep up to date with changes.</p>
</p>
<nav id="TOC">
<ul>
<li><a href="#introduction">Introduction</a><ul>
<li><a href="#thanks">Thanks</a></li>
<li><a href="#changelog">Changelog</a></li>
<li><a href="#refactoring">Refactoring</a></li>
<li><a href="#tests">Tests</a></li>
</ul></li>
<li><a href="#tale-1-terrible-tabs">Tale 1: Terrible Tabs</a><ul>
<li><a href="#reuse-of-selectors">Reuse of Selectors</a></li>
<li><a href="#more-duplication">More Duplication</a></li>
<li><a href="#step-back">Step Back</a></li>
<li><a href="#higher-level-duplication">Higher level duplication</a></li>
<li><a href="#merging-the-branches">Merging the branches</a></li>
<li><a href="#one-method-to-rule-them-all">One method to rule them all</a></li>
<li><a href="#reflection">Reflection</a></li>
</ul></li>
<li><a href="#tale-2-cringey-carousels">Tale 2: Cringey Carousels</a><ul>
<li><a href="#the-carousel">The Carousel</a></li>
<li><a href="#return-false-the-anti-pattern">return false; the anti-pattern</a></li>
<li><a href="#on-and-off">on() and off()</a></li>
<li><a href="#repeated-numbers">Repeated Numbers</a></li>
<li><a href="#caching-selectors">Caching selectors</a></li>
<li><a href="#functions">functions</a></li>
<li><a href="#summary">Summary</a></li>
</ul></li>
<li><a href="#tale-3-async-abominations">Tale 3: Async Abominations</a><ul>
<li><a href="#the-code">The code</a></li>
<li><a href="#back-to-the-beginning">Back to the beginning</a></li>
<li><a href="#abstracting-functions">Abstracting functions</a></li>
</ul></li>
<li><a href="#tale-4-parsing-problems">Tale 4: Parsing Problems</a><ul>
<li><a href="#email-sending"> Email Sending</a></li>
<li><a href="#single-responsibility-principle">Single Responsibility Principle</a></li>
<li><a href="#an-improvement">An improvement</a></li>
<li><a href="#coupling">Coupling</a></li>
<li><a href="#publish-and-subscribe">Publish and Subscribe</a></li>
<li><a href="#conclusion">Conclusion</a></li>
</ul></li>
</ul>
</nav>
<h1 id="introduction">Introduction</h1>
<p>Welcome to The Refactoring Tales, a book that documents some of the refactorings and changes I’ve made in recent (and mostly real-life) projects. This book isn’t going to teach you about language constructs, conditionals, functions, or so on, but hopefully offer insight into how to take steps to make your code more readable and more importantly, maintainable.</p>
<p>Think of how much time you spend maintaining code, rather than being able to write code from scratch. Day to day, I’m not typically creating new projects, but I am maintaining, editing or refactoring existing projects. This book is just like that. Each chapter will start by looking at some existing code, and over the course of a few pages we will examine, dissect and then refactor the code into an improved alternative. Of course, the idea of code being “better” is largely subjective, but even if you don’t quite agree with every step I take, you should be able to see the overall benefits.</p>
<p>The GitHub repository for this book is here: <a href="https://github.com/jackfranklin/the-refactoring-tales">https://github.com/jackfranklin/the-refactoring-tales</a>, both the raw book files and the code samples are all there for you to take. If you spot any issues as you read this book, a new issue (or even better, a pull request!) is greatly appreciated.</p>
<h2 id="thanks">Thanks</h2>
<p>There are so many people who have thanked me along the way that it would be wrong to not include some form of list of names in this book. Thanks in particular go to Addy Osmani, Drew Neil, Guy Routledge, Katja Durrani, Adam Yeats, Stu Robson and Todd Motto along with many more who have shaped this book since the idea formed.</p>
<h2 id="changelog">Changelog</h2>
<p>If you’ve been reading this book since the beta, to help you know what’s changed, a list of changes and dates of update will be kept below.</p>
<h5 id="section">21/07/14</h5>
<ul>
<li>initial release</li>
</ul>
<h2 id="refactoring">Refactoring</h2>
<p>Before we continue I think it’s important to define just what exactly I mean when I say “refactoring”. Refactoring has lots of definitions, but I think my favourite and the one I stick to is that of Martin Fowler, in his <a href="http://martinfowler.com/books/refactoring.html">book on refactoring</a>:</p>
<blockquote>
<p>Refactoring is a controlled technique for improving the design of an existing code base. Its essence is applying a series of small behaviour-preserving transformations, each of which “too small to be worth doing”.</p>
</blockquote>
<p>We apply small changes to the code, and do so several times, until we’re left with a section of code whose design is greatly improved. Notice how they are <em>behaviour-preserving</em> transformations: a refactor should never change behaviour, merely improve the code. The idea of “better” code may be subjective, but to me when I think of good qualities for code to have, I think of it being:</p>
<ul>
<li>self documenting: methods and variables are well named and follow a consistent naming pattern</li>
<li>DRY: there is no duplication or knowledge sharing; every piece of information that could change is declared only once</li>
<li>clear in its intention; it should read somewhat like a story, and at a quick skim the overall function of that block of code should be apparent</li>
</ul>
<p>In his book <a href="http://www.wrox.com/WileyCDA/WroxTitle/productCd-0764579088.html">Professional JavaScript for Web Developers</a>, Nicholas Zakas has a brilliant definition of maintainability which cites the following characteristics: Understandable, intuitive, adaptable, extendable and debuggable. It’s these traits that I hope this book will help you adhere to and aim for.</p>
<h2 id="tests">Tests</h2>
<p>When refactoring, you need to have confidence in the fact that you’ve not broken anything. Similarly, if you do break something, you need to know immediately. This is where the benefit of having tests plays a key role. The ability to run a set of tests and get immediate feedback is fantastic, and that’s precisely what tests give you. It’s very easy these days to get started with tests, whether you’re writing Ruby, or JavaScript on the server with NodeJS, or in the browser, there are a myriad of tools available to you. This isn’t a book on testing, and I could write in huge depth on the subject, so for each example in this book, you should assume that I have tested it as I’ve gone (in actual fact for every example there is, I did have tests every time). If you want to refactor but don’t have tests, write tests. Do not ever attempt to refactor without writing any tests. Ever.</p>
<p>You should also make sure you can run your tests easily. I have mine hooked up through my editor, Vim, so I can hit a 3 key shortcut and have my tests run. Configure it however you want, and make sure it’s easy to do.</p>
<h1 id="tale-1-terrible-tabs">Tale 1: Terrible Tabs</h1>
<p>One of the most common things any new jQuery user will try to do is build some basic HTML tabs. It’s like the initiation into the jQuery world (not quite, that’s carousels, which are next up) and it serves as a good starting point to show you the first refactoring.</p>
<p>Here’s a JavaScript function called <code>tabularize</code> which, as you might expect, is a small function for creating a tabbed interface.</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> tabularize = <span class="kw">function</span>() {
<span class="kw">var</span> active = <span class="ot">location</span>.<span class="fu">hash</span>;
<span class="kw">if</span>(active) {
<span class="fu">$</span>(<span class="st">".tabs"</span>).<span class="fu">children</span>(<span class="st">"div"</span>).<span class="fu">hide</span>();
<span class="fu">$</span>(active).<span class="fu">show</span>();
<span class="fu">$</span>(<span class="st">".active"</span>).<span class="fu">removeClass</span>(<span class="st">"active"</span>);
<span class="fu">$</span>(<span class="st">".tab-link"</span>).<span class="fu">each</span>(<span class="kw">function</span>() {
<span class="kw">if</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>) === active) {
<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>().<span class="fu">addClass</span>(<span class="st">"active"</span>);
}
});
}
<span class="fu">$</span>(<span class="st">".tabs"</span>).<span class="fu">find</span>(<span class="st">".tab-link"</span>).<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="fu">$</span>(<span class="st">".tabs"</span>).<span class="fu">children</span>(<span class="st">"div"</span>).<span class="fu">hide</span>();
<span class="fu">$</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>)).<span class="fu">show</span>();
<span class="fu">$</span>(<span class="st">".active"</span>).<span class="fu">removeClass</span>(<span class="st">"active"</span>);
<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>().<span class="fu">addClass</span>(<span class="st">"active"</span>);
<span class="kw">return</span> <span class="kw">false</span>;
});
};</code></pre>
<p>The first part of the function deals with the case where there is a fragment identifier in the URL. For example, if someone visits <code>mysite.com/#tab2</code>, they should have tab 2 active when they load the page. The second part deals with a user clicking on a tab, and updating the content accordingly.</p>
<p>To help you put this together, here is the HTML that the code is applied to.</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><div <span class="kw">class</span>=<span class="st">"tabs"</span>>
<ul>
<li <span class="kw">class</span>=<span class="st">"active"</span>><a href=<span class="st">"#tab1"</span> <span class="kw">class</span>=<span class="st">"tab-link"</span>>Tab <span class="dv">1</span><<span class="ot">/a></li</span>>
<li><a href=<span class="st">"#tab2"</span> <span class="kw">class</span>=<span class="st">"tab-link"</span>>Tab <span class="dv">2</span><<span class="ot">/a></li</span>>
<li><a href=<span class="st">"#tab3"</span> <span class="kw">class</span>=<span class="st">"tab-link"</span>>Tab <span class="dv">3</span><<span class="ot">/a></li</span>>
<<span class="ot">/ul></span>
<span class="ot"> <div id="tab1"></span>
<span class="ot"> <h3>Tab 1</h3</span>>
<p>Lorem ipsum dolor sit amet<<span class="ot">/p></span>
<span class="ot"> </div</span>>
<div id=<span class="st">"tab2"</span>>
<h3>Tab <span class="dv">2</span><<span class="ot">/h3></span>
<span class="ot"> <p>Lorem ipsum dolor sit amet</p</span>>
<<span class="ot">/div></span>
<span class="ot"> <div id="tab3"></span>
<span class="ot"> <h3>Tab 3</h3</span>>
<p>Lorem ipsum dolor sit amet<<span class="ot">/p></span>
<span class="ot"> </div</span>>
<<span class="ot">/div></span>
<span class="ot"><script></span>
<span class="ot"> $</span><span class="fl">(</span><span class="ot">tabularize</span><span class="fl">)</span><span class="ot">;</span>
<span class="ot"></script</span>></code></pre>
<p>There’s a fair bit wrong with the JavaScript above, but it’s not necessarily bad code. It performs the tasks that are required of it. There are a couple of bugs, but as refactorers, we are not here to change the behaviour of the code. It passes the tests (in the introduction we discussed how every refactoring must be backed by tests) and our aim is to change the design, not the behaviour, and pass all the tests. I won’t show the tests, as they distract from the main purpose, but rest assured I did have them when making the changes I’m about to talk through and I was careful to keep them passing.</p>
<h2 id="reuse-of-selectors">Reuse of Selectors</h2>
<p>The key to refactoring is to make the smallest steps you possibly can. The first problem I’d like to tackle is the reuse of selectors. Let’s take a look once more at that JavaScript:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> active = <span class="ot">location</span>.<span class="fu">hash</span>;
<span class="kw">if</span>(active) {
<span class="fu">$</span>(<span class="st">".tabs"</span>).<span class="fu">children</span>(<span class="st">"div"</span>).<span class="fu">hide</span>();
<span class="fu">$</span>(active).<span class="fu">show</span>();
<span class="fu">$</span>(<span class="st">".active"</span>).<span class="fu">removeClass</span>(<span class="st">"active"</span>);
<span class="fu">$</span>(<span class="st">".tab-link"</span>).<span class="fu">each</span>(<span class="kw">function</span>() {
<span class="kw">if</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>) === active) {
<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>().<span class="fu">addClass</span>(<span class="st">"active"</span>);
}
});
}
<span class="fu">$</span>(<span class="st">".tabs"</span>).<span class="fu">find</span>(<span class="st">".tab-link"</span>).<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="fu">$</span>(<span class="st">".tabs"</span>).<span class="fu">children</span>(<span class="st">"div"</span>).<span class="fu">hide</span>();
<span class="fu">$</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>)).<span class="fu">show</span>();
<span class="fu">$</span>(<span class="st">".active"</span>).<span class="fu">removeClass</span>(<span class="st">"active"</span>);
<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>().<span class="fu">addClass</span>(<span class="st">"active"</span>);
<span class="kw">return</span> <span class="kw">false</span>;
});</code></pre>
<p>Looking over the code again, you can see a few selectors that crop up again and again:</p>
<ul>
<li><code>$(".tabs")</code></li>
<li><code>$(".tabs").children("div")</code></li>
<li><code>$(".tab-link")</code></li>
</ul>
<p>So let’s make the change. We’ll replace them, but do it <em>one at a time</em>, and run the tests <em>after every change</em>. This might seem excessive, but it really is key to this process.</p>
<p>Firstly, I’ll store a reference to <code>$(".tabs")</code>:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> tabsWrapper = <span class="fu">$</span>(<span class="st">".tabs"</span>);</code></pre>
<p>And then I can replace every occurrence of <code>$(".tabs")</code> with <code>tabsWrapper</code>:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> tabsWrapper = <span class="fu">$</span>(<span class="st">".tabs"</span>);
<span class="kw">var</span> active = <span class="ot">location</span>.<span class="fu">hash</span>;
<span class="kw">if</span>(active) {
<span class="ot">tabsWrapper</span>.<span class="fu">children</span>(<span class="st">"div"</span>).<span class="fu">hide</span>();
<span class="fu">$</span>(active).<span class="fu">show</span>();
<span class="fu">$</span>(<span class="st">".active"</span>).<span class="fu">removeClass</span>(<span class="st">"active"</span>);
<span class="fu">$</span>(<span class="st">".tab-link"</span>).<span class="fu">each</span>(<span class="kw">function</span>() {
<span class="kw">if</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>) === active) {
<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>().<span class="fu">addClass</span>(<span class="st">"active"</span>);
}
});
}
<span class="ot">tabsWrapper</span>.<span class="fu">find</span>(<span class="st">".tab-link"</span>).<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="ot">tabsWrapper</span>.<span class="fu">children</span>(<span class="st">"div"</span>).<span class="fu">hide</span>();
<span class="fu">$</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>)).<span class="fu">show</span>();
<span class="fu">$</span>(<span class="st">".active"</span>).<span class="fu">removeClass</span>(<span class="st">"active"</span>);
<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>().<span class="fu">addClass</span>(<span class="st">"active"</span>);
<span class="kw">return</span> <span class="kw">false</span>;
});</code></pre>
<p>Now that’s done and everything is passing, I can repeat the step with <code>$(".tabs").children("div")</code>:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> tabsWrapper = <span class="fu">$</span>(<span class="st">".tabs"</span>);
<span class="kw">var</span> tabs = <span class="ot">tabsWrapper</span>.<span class="fu">children</span>(<span class="st">"div"</span>);
<span class="kw">var</span> active = <span class="ot">location</span>.<span class="fu">hash</span>;
<span class="kw">if</span>(active) {
<span class="ot">tabs</span>.<span class="fu">hide</span>();
<span class="fu">$</span>(active).<span class="fu">show</span>();
<span class="fu">$</span>(<span class="st">".active"</span>).<span class="fu">removeClass</span>(<span class="st">"active"</span>);
<span class="fu">$</span>(<span class="st">".tab-link"</span>).<span class="fu">each</span>(<span class="kw">function</span>() {
<span class="kw">if</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>) === active) {
<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>().<span class="fu">addClass</span>(<span class="st">"active"</span>);
}
});
}
<span class="ot">tabsWrapper</span>.<span class="fu">find</span>(<span class="st">".tab-link"</span>).<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="ot">tabs</span>.<span class="fu">hide</span>();
<span class="fu">$</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>)).<span class="fu">show</span>();
<span class="fu">$</span>(<span class="st">".active"</span>).<span class="fu">removeClass</span>(<span class="st">"active"</span>);
<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>().<span class="fu">addClass</span>(<span class="st">"active"</span>);
<span class="kw">return</span> <span class="kw">false</span>;
});</code></pre>
<p>And finally, deal with <code>$(".tab-link")</code>:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> tabsWrapper = <span class="fu">$</span>(<span class="st">".tabs"</span>);
<span class="kw">var</span> tabs = <span class="ot">tabsWrapper</span>.<span class="fu">children</span>(<span class="st">"div"</span>);
<span class="kw">var</span> tabLinks = <span class="ot">tabsWrapper</span>.<span class="fu">find</span>(<span class="st">".tab-link"</span>);
<span class="kw">var</span> active = <span class="ot">location</span>.<span class="fu">hash</span>;
<span class="kw">if</span>(active) {
<span class="ot">tabs</span>.<span class="fu">hide</span>();
<span class="fu">$</span>(active).<span class="fu">show</span>();
<span class="fu">$</span>(<span class="st">".active"</span>).<span class="fu">removeClass</span>(<span class="st">"active"</span>);
<span class="ot">tabLinks</span>.<span class="fu">each</span>(<span class="kw">function</span>() {
<span class="kw">if</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>) === active) {
<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>().<span class="fu">addClass</span>(<span class="st">"active"</span>);
}
});
}
<span class="ot">tabLinks</span>.<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="ot">tabs</span>.<span class="fu">hide</span>();
<span class="fu">$</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>)).<span class="fu">show</span>();
<span class="fu">$</span>(<span class="st">".active"</span>).<span class="fu">removeClass</span>(<span class="st">"active"</span>);
<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>().<span class="fu">addClass</span>(<span class="st">"active"</span>);
<span class="kw">return</span> <span class="kw">false</span>;
});</code></pre>
<p>And now, even with just that small change in place, our code is improved. We have removed duplication, which in my opinion has given us two big improvements.</p>
<ol type="1">
<li>If any of the selectors change, or our HTML changes, we only have to change those selectors in one place, not multiple.</li>
<li>The code is clearer now. If you need to skim and quickly gain an understanding of what the code does, having well named variables in place of complex selectors helps massively.</li>
</ol>
<h2 id="more-duplication">More Duplication</h2>
<p>There’s a bit more duplication going on though. If you look through the code, you’ll see the string <code>"active"</code> present far too many times. What if the class we gave the active tab had to change? Right now, we’re looking at <strong>five</strong> occurences of it. Wouldn’t it be better if that was just one?</p>
<p>Let’s introduce an <code>activeClass</code> variable to deal with this:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> tabsWrapper = <span class="fu">$</span>(<span class="st">".tabs"</span>);
<span class="kw">var</span> tabs = <span class="ot">tabsWrapper</span>.<span class="fu">children</span>(<span class="st">"div"</span>);
<span class="kw">var</span> tabLinks = <span class="ot">tabsWrapper</span>.<span class="fu">find</span>(<span class="st">".tab-link"</span>);
<span class="kw">var</span> activeClass = <span class="st">"active"</span>;
<span class="kw">var</span> active = <span class="ot">location</span>.<span class="fu">hash</span>;
<span class="kw">if</span>(active) {
<span class="ot">tabs</span>.<span class="fu">hide</span>();
<span class="fu">$</span>(active).<span class="fu">show</span>();
<span class="fu">$</span>(<span class="st">"."</span> + activeClass).<span class="fu">removeClass</span>(activeClass);
<span class="ot">tabLinks</span>.<span class="fu">each</span>(<span class="kw">function</span>() {
<span class="kw">if</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>) === active) {
<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>().<span class="fu">addClass</span>(activeClass);
}
});
}
<span class="ot">tabLinks</span>.<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="ot">tabs</span>.<span class="fu">hide</span>();
<span class="fu">$</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>)).<span class="fu">show</span>();
<span class="fu">$</span>(<span class="st">"."</span> + activeClass).<span class="fu">removeClass</span>(activeClass);
<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>().<span class="fu">addClass</span>(activeClass);
<span class="kw">return</span> <span class="kw">false</span>;
});</code></pre>
<p>That’s a good step to take, but in the midst of doing this you might have spotted some more duplication. Take a look at these two code blocks. They look pretty similar to me:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="fu">$</span>(<span class="st">"."</span> + activeClass).<span class="fu">removeClass</span>(activeClass);
<span class="ot">tabLinks</span>.<span class="fu">each</span>(<span class="kw">function</span>() {
<span class="kw">if</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>) === active) {
<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>().<span class="fu">addClass</span>(activeClass);
}
});</code></pre>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="fu">$</span>(<span class="st">"."</span> + activeClass).<span class="fu">removeClass</span>(activeClass);
<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>().<span class="fu">addClass</span>(activeClass);</code></pre>
<p>The first is slightly different, because it has to loop over the links to find the right element to work with, but both of these blocks are performing the same piece of work:</p>
<ol type="1">
<li>Find the current element with the active class, and remove the active class.</li>
<li>Take the new active link’s parent, and add the active class.</li>
</ol>
<p>When we have more than one block of code doing the same thing we can abstract them out into a function. Let’s do that now:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> activateLink = <span class="kw">function</span>(elem) {
<span class="fu">$</span>(<span class="st">"."</span> + activeClass).<span class="fu">removeClass</span>(activeClass);
<span class="fu">$</span>(elem).<span class="fu">addClass</span>(activeClass);
};</code></pre>
<p>The <code>activateLink</code> function takes an element and adds the active class to it once it’s first removed the active class from any other element that might have it. Now we have this function, we can use it in place of the code we looked at previously. We’ll do this change one at a time. Firstly, we can edit the code within the <code>tabLinks.click</code> handler:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> tabsWrapper = <span class="fu">$</span>(<span class="st">".tabs"</span>);
<span class="kw">var</span> tabs = <span class="ot">tabsWrapper</span>.<span class="fu">children</span>(<span class="st">"div"</span>);
<span class="kw">var</span> tabLinks = <span class="ot">tabsWrapper</span>.<span class="fu">find</span>(<span class="st">".tab-link"</span>);
<span class="kw">var</span> activeClass = <span class="st">"active"</span>;
<span class="kw">var</span> activateLink = <span class="kw">function</span>(elem) {
<span class="fu">$</span>(<span class="st">"."</span> + activeClass).<span class="fu">removeClass</span>(activeClass);
<span class="fu">$</span>(elem).<span class="fu">addClass</span>(activeClass);
};
<span class="kw">var</span> active = <span class="ot">location</span>.<span class="fu">hash</span>;
<span class="kw">if</span>(active) {
<span class="ot">tabs</span>.<span class="fu">hide</span>();
<span class="fu">$</span>(active).<span class="fu">show</span>();
<span class="fu">$</span>(<span class="st">"."</span> + activeClass).<span class="fu">removeClass</span>(activeClass);
<span class="ot">tabLinks</span>.<span class="fu">each</span>(<span class="kw">function</span>() {
<span class="kw">if</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>) === active) {
<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>().<span class="fu">addClass</span>(activeClass);
}
});
}
<span class="ot">tabLinks</span>.<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="ot">tabs</span>.<span class="fu">hide</span>();
<span class="fu">$</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>)).<span class="fu">show</span>();
<span class="fu">activateLink</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>());
<span class="kw">return</span> <span class="kw">false</span>;
});</code></pre>
<p>Now all we have to do is pass <code>$(this).parent()</code>, which is the element we want to gain the active class, into our <code>activateLink</code> function, and it does the rest. We can now swap our function in in place of the code in the <code>if(active) {}</code> block:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> tabsWrapper = <span class="fu">$</span>(<span class="st">".tabs"</span>);
<span class="kw">var</span> tabs = <span class="ot">tabsWrapper</span>.<span class="fu">children</span>(<span class="st">"div"</span>);
<span class="kw">var</span> tabLinks = <span class="ot">tabsWrapper</span>.<span class="fu">find</span>(<span class="st">".tab-link"</span>);
<span class="kw">var</span> activeClass = <span class="st">"active"</span>;
<span class="kw">var</span> activateLink = <span class="kw">function</span>(elem) {
<span class="fu">$</span>(<span class="st">"."</span> + activeClass).<span class="fu">removeClass</span>(activeClass);
<span class="fu">$</span>(elem).<span class="fu">addClass</span>(activeClass);
};
<span class="kw">var</span> active = <span class="ot">location</span>.<span class="fu">hash</span>;
<span class="kw">if</span>(active) {
<span class="ot">tabs</span>.<span class="fu">hide</span>();
<span class="fu">$</span>(active).<span class="fu">show</span>();
<span class="ot">tabLinks</span>.<span class="fu">each</span>(<span class="kw">function</span>() {
<span class="kw">if</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>) === active) {
<span class="fu">activateLink</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>());
}
});
}
<span class="ot">tabLinks</span>.<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="ot">tabs</span>.<span class="fu">hide</span>();
<span class="fu">$</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>)).<span class="fu">show</span>();
<span class="fu">activateLink</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>());
<span class="kw">return</span> <span class="kw">false</span>;
});</code></pre>
<p>Abstracting code out into functions is one of the easiest and most effective ways to make a block of code more maintainable. Functions are inherently self documenting, a well named function can tell you in one quick skim exactly what its function is, and what it does. As a new developer coming to the above block of code, I can understand the <code>activateLink</code> function’s effect <em>without looking at the code within it</em>. Being able to skim a block of code and gain an understanding without having to look at detailed implementation is a fantastic thing as a developer.</p>
<h2 id="step-back">Step Back</h2>
<p>We are far from done with these tabs, but I want you to notice how, even after just a couple of small changes, the code is now already in an improved position than when we picked it up. We have removed duplication of selectors and code, and made the code more self documenting and readable along the way. When refactoring, you should be in a position to stop the refactoring at any point, and move onto something else. If this was a real project, and suddenly I was called to an urgent bug in another project, I could commit this code now and still have improved it. You should never find yourself in such a mess that you can’t put the code down and come back later. This is vital to successful refactorings: keep them small and contained.</p>
<h2 id="higher-level-duplication">Higher level duplication</h2>
<p>The code we’ve been working with has two blocks:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">if</span>(active) {
<span class="co">// do tab things</span>
};
<span class="ot">tabLinks</span>.<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="co">// do tab things</span>
};</code></pre>
<p>Although it doesn’t look like it at a glance, there’s a lot of duplication going on - both those blocks of code perform basically the same task. This can be sometimes hard to spot, as it can be hidden behind code that doesn’t immediately look the same.</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> tabsWrapper = <span class="fu">$</span>(<span class="st">".tabs"</span>);
<span class="kw">var</span> tabs = <span class="ot">tabsWrapper</span>.<span class="fu">children</span>(<span class="st">"div"</span>);
<span class="kw">var</span> tabLinks = <span class="ot">tabsWrapper</span>.<span class="fu">find</span>(<span class="st">".tab-link"</span>);
<span class="kw">var</span> activeClass = <span class="st">"active"</span>;
<span class="kw">var</span> activateLink = <span class="kw">function</span>(elem) {
<span class="fu">$</span>(<span class="st">"."</span> + activeClass).<span class="fu">removeClass</span>(activeClass);
<span class="fu">$</span>(elem).<span class="fu">addClass</span>(activeClass);
};
<span class="kw">var</span> active = <span class="ot">location</span>.<span class="fu">hash</span>;
<span class="kw">if</span>(active) {
<span class="ot">tabs</span>.<span class="fu">hide</span>();
<span class="fu">$</span>(active).<span class="fu">show</span>();
<span class="ot">tabLinks</span>.<span class="fu">each</span>(<span class="kw">function</span>() {
<span class="kw">if</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>) === active) {
<span class="fu">activateLink</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>());
}
});
}
<span class="ot">tabLinks</span>.<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="ot">tabs</span>.<span class="fu">hide</span>();
<span class="fu">$</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>)).<span class="fu">show</span>();
<span class="fu">activateLink</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>());
<span class="kw">return</span> <span class="kw">false</span>;
});</code></pre>
<p>Now I’ve given you the primer, take another look over the code above. Notice how both blocks do the same thing:</p>
<ul>
<li>hide all the tabs</li>
<li>find and show a specific tab</li>
<li>update the link for that tab with a new class</li>
</ul>
<p>The duplication is obscured somewhat because of the need for the <code>tabLinks.each</code> in the first block, but this doesn’t mean that we can’t abstract that duplication into a function.</p>
<p>Sticking with our mantra of making small steps, let’s first make a function that shows a specific tab. Sticking with the naming conventions, we’ll call it <code>activateTab</code>:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> activateTab = <span class="kw">function</span>(tabSelector) {
<span class="ot">tabs</span>.<span class="fu">hide</span>();
<span class="fu">$</span>(tabSelector).<span class="fu">show</span>();
};</code></pre>
<p>This function takes a selector and shows it, after hiding all of the tabs first. We can now use this in both the <code>if(active)</code> block and in the event handler:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> tabsWrapper = <span class="fu">$</span>(<span class="st">".tabs"</span>);
<span class="kw">var</span> tabs = <span class="ot">tabsWrapper</span>.<span class="fu">children</span>(<span class="st">"div"</span>);
<span class="kw">var</span> tabLinks = <span class="ot">tabsWrapper</span>.<span class="fu">find</span>(<span class="st">".tab-link"</span>);
<span class="kw">var</span> activeClass = <span class="st">"active"</span>;
<span class="kw">var</span> activateLink = <span class="kw">function</span>(elem) {
<span class="fu">$</span>(<span class="st">"."</span> + activeClass).<span class="fu">removeClass</span>(activeClass);
<span class="fu">$</span>(elem).<span class="fu">addClass</span>(activeClass);
};
<span class="kw">var</span> activateTab = <span class="kw">function</span>(tabSelector) {
<span class="ot">tabs</span>.<span class="fu">hide</span>();
<span class="fu">$</span>(tabSelector).<span class="fu">show</span>();
};
<span class="kw">var</span> active = <span class="ot">location</span>.<span class="fu">hash</span>;
<span class="kw">if</span>(active) {
<span class="fu">activateTab</span>(active);
<span class="ot">tabLinks</span>.<span class="fu">each</span>(<span class="kw">function</span>() {
<span class="kw">if</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>) === active) {
<span class="fu">activateLink</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>());
}
});
}
<span class="ot">tabLinks</span>.<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="fu">activateTab</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>));
<span class="fu">activateLink</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>());
<span class="kw">return</span> <span class="kw">false</span>;
});</code></pre>
<p>We’re really motoring now. Notice how the tabLinks event handler simply calls two other functions, and has very little in it. This is a good sign that we’re on the right tracks. <a href="http://codeulate.com/">Ben Orenstein</a>, a developer who speaks a huge amount on refactoring, says that a function with one line inside is superior to a function with two lines, which in turn is superior to a function with three lines, and so on. <a href="http://www.sandimetz.com/">Sandi Metz</a>, a well known Ruby developer, defined a <a href="http://robots.thoughtbot.com/sandi-metz-rules-for-developers">set of rules</a> that help when building large projects, one of which is: “Methods can be no longer than five lines of code.”. Regardless of if you think five is a good or bad number for that rule, the point stands: short, small functions are nearly always preferable to large ones. Keep functions small and compose larger functions out of calling lots of little ones.</p>
<p>Before we continue, notice again how if we wanted to stop now, we could. It’s so important to not let yourself get down a huge rabbit hole of refactoring.</p>
<h2 id="merging-the-branches">Merging the branches</h2>
<p>Right now we have two branches in our code, the <code>if(active)</code> part and the event handler. I’d really like to try and get these into one, or at least make the branches as small as possible. Right now they still have duplication, they both call <code>activateTab</code> and <code>activateLink</code>. I’d really like to abstract that out into another function, but right now the obvious step isn’t that obvious. Sometimes you’ll reach a point like this when you’re coding, where you know what you need to do or want to do, but the step isn’t obvious. Often you’ll have to make another change, to make the new change easier. In their book <a href="http://refactoring.com/">Refactoring</a>, Martin Fowler and Kent Beck put this nicely:</p>
<blockquote>
<p>When you find you have to add a feature to a program, and the program’s code is not structured in a convenient way to add the feature, first refactor the program to make it easy to add the feature, then add the feature.</p>
</blockquote>
<p>Although this quote talks about new features, what it effectively says is that if you need to make a change, but that change is proving tough to make, make other changes such that your original change is easy.</p>
<p>I realised after some thinking that the bit of code making this change difficult is this bit:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="ot">tabLinks</span>.<span class="fu">each</span>(<span class="kw">function</span>() {
<span class="kw">if</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>) === active) {
<span class="fu">activateLink</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>());
}
});</code></pre>
<p>The fact that we have to loop over means we can’t just abstract out as easily. Instead of the <code>each</code>, we can instead use jQuery’s <code>filter</code> method:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> link = <span class="ot">tabLinks</span>.<span class="fu">filter</span>(<span class="kw">function</span>() {
<span class="kw">return</span> <span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>) === active;
}.<span class="fu">parent</span>());
<span class="fu">activateLink</span>(link);</code></pre>
<p>We now filter over the tab links, looking for the one that matches the <code>active</code> hash, and get at the item that way instead. I can store the result of that to a variable, and then pass <code>activateLink</code> that element. Adding that change into our code gives us:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> tabsWrapper = <span class="fu">$</span>(<span class="st">".tabs"</span>);
<span class="kw">var</span> tabs = <span class="ot">tabsWrapper</span>.<span class="fu">children</span>(<span class="st">"div"</span>);
<span class="kw">var</span> tabLinks = <span class="ot">tabsWrapper</span>.<span class="fu">find</span>(<span class="st">".tab-link"</span>);
<span class="kw">var</span> activeClass = <span class="st">"active"</span>;
<span class="kw">var</span> activateLink = <span class="kw">function</span>(elem) {
<span class="fu">$</span>(<span class="st">"."</span> + activeClass).<span class="fu">removeClass</span>(activeClass);
<span class="fu">$</span>(elem).<span class="fu">addClass</span>(activeClass);
};
<span class="kw">var</span> activateTab = <span class="kw">function</span>(tabSelector) {
<span class="ot">tabs</span>.<span class="fu">hide</span>();
<span class="fu">$</span>(tabSelector).<span class="fu">show</span>();
};
<span class="kw">var</span> active = <span class="ot">location</span>.<span class="fu">hash</span>;
<span class="kw">if</span>(active) {
<span class="fu">activateTab</span>(active);
<span class="kw">var</span> link = <span class="ot">tabLinks</span>.<span class="fu">filter</span>(<span class="kw">function</span>() {
<span class="kw">return</span> <span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>) === active;
}.<span class="fu">parent</span>());
<span class="fu">activateLink</span>(link);
}
<span class="ot">tabLinks</span>.<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="fu">activateTab</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>));
<span class="fu">activateLink</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>());
<span class="kw">return</span> <span class="kw">false</span>;
});</code></pre>
<p>Now, to see where the next change will come from, we need to examine a bit more closely the <code>activateLink</code> and <code>activateTab</code> function. Ideally, I’d like to encapsulate these into another function, but to do that we need to see what each function needs as a parameter. <code>activateTab</code> just takes a selector and uses that to hide and show what’s required, but <code>activateLink</code> actually takes in an element. However, if you look closely, you’ll note that there is a relationship between the <code>activateTab</code> parameter and the <code>activateLink</code> parameter. The <code>activateLink</code> is the parent of the element whose selector we pass into <code>activateTab</code>. So why don’t we pass the selector into <code>activateLink</code>, and let it find the exact element it needs?</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> activateLink = <span class="kw">function</span>(selector) {
<span class="fu">$</span>(<span class="st">"."</span> + activeClass).<span class="fu">removeClass</span>(activeClass);
<span class="kw">var</span> elem = <span class="ot">tabLinks</span>.<span class="fu">filter</span>(<span class="kw">function</span>() {
<span class="kw">return</span> <span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>) === selector;
}.<span class="fu">parent</span>());
<span class="fu">$</span>(elem).<span class="fu">addClass</span>(activeClass);
};</code></pre>
<p>With that change, suddenly we can rewrite the two branches of our code to look very similar indeed:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">if</span>(active) {
<span class="fu">activateTab</span>(active);
<span class="fu">activateLink</span>(active);
}
<span class="ot">tabLinks</span>.<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="fu">activateTab</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>));
<span class="fu">activateLink</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>));
<span class="kw">return</span> <span class="kw">false</span>;
});</code></pre>
<p>Now we’ve achieved what we wanted; by performing some intermediate refactorings we now are in a position to deal with the duplication we have in the two branches.</p>
<h2 id="one-method-to-rule-them-all">One method to rule them all</h2>
<p>We’re going to extract a new method, called <code>transition</code>, which will take the selector of the active tab in as its argument, and perform the tasks required. The <code>transition</code> method is very simple, it just hands off to <code>activateTab</code> and <code>activateLink</code>:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> transition = <span class="kw">function</span>(selector) {
<span class="fu">activateTab</span>(selector);
<span class="fu">activateLink</span>(selector);
};</code></pre>
<p>And now we can use that method in our code. In reality I did make this change in two steps, inserting one usage at a time, but I think you get the picture, so I’ll show it here as one change. Our new code looks like so:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> tabsWrapper = <span class="fu">$</span>(<span class="st">".tabs"</span>);
<span class="kw">var</span> tabs = <span class="ot">tabsWrapper</span>.<span class="fu">children</span>(<span class="st">"div"</span>);
<span class="kw">var</span> tabLinks = <span class="ot">tabsWrapper</span>.<span class="fu">find</span>(<span class="st">".tab-link"</span>);
<span class="kw">var</span> activeClass = <span class="st">"active"</span>;
<span class="kw">var</span> activateLink = <span class="kw">function</span>(elem) {
<span class="fu">$</span>(<span class="st">"."</span> + activeClass).<span class="fu">removeClass</span>(activeClass);
<span class="fu">$</span>(elem).<span class="fu">addClass</span>(activeClass);
};
<span class="kw">var</span> activateTab = <span class="kw">function</span>(tabSelector) {
<span class="ot">tabs</span>.<span class="fu">hide</span>();
<span class="fu">$</span>(tabSelector).<span class="fu">show</span>();
};
<span class="kw">var</span> transition = <span class="kw">function</span>(selector) {
<span class="fu">activateTab</span>(selector);
<span class="fu">activateLink</span>(selector);
};
<span class="kw">var</span> active = <span class="ot">location</span>.<span class="fu">hash</span>;
<span class="kw">if</span>(active) {
<span class="fu">transition</span>(active);
}
<span class="ot">tabLinks</span>.<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="fu">transition</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href'));</span>
<span class="kw">return</span> <span class="kw">false</span>;
});</code></pre>
<h2 id="reflection">Reflection</h2>
<p>There’s certainly more you could do with this code, and it’s also far from being the best implementation of tabs around, but I hope you agree with me that the end result is now much nicer than the one we had at the beginning, which is printed below for you to compare.</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> active = <span class="ot">location</span>.<span class="fu">hash</span>;
<span class="kw">if</span>(active) {
<span class="fu">$</span>(<span class="st">".tabs"</span>).<span class="fu">children</span>(<span class="st">"div"</span>).<span class="fu">hide</span>();
<span class="fu">$</span>(active).<span class="fu">show</span>();
<span class="fu">$</span>(<span class="st">".active"</span>).<span class="fu">removeClass</span>(<span class="st">"active"</span>);
<span class="fu">$</span>(<span class="st">".tab-link"</span>).<span class="fu">each</span>(<span class="kw">function</span>() {
<span class="kw">if</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>) === active) {
<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>().<span class="fu">addClass</span>(<span class="st">"active"</span>);
}
});
}
<span class="fu">$</span>(<span class="st">".tabs"</span>).<span class="fu">find</span>(<span class="st">".tab-link"</span>).<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="fu">$</span>(<span class="st">".tabs"</span>).<span class="fu">children</span>(<span class="st">"div"</span>).<span class="fu">hide</span>();
<span class="fu">$</span>(<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">attr</span>(<span class="st">"href"</span>)).<span class="fu">show</span>();
<span class="fu">$</span>(<span class="st">".active"</span>).<span class="fu">removeClass</span>(<span class="st">"active"</span>);
<span class="fu">$</span>(<span class="kw">this</span>).<span class="fu">parent</span>().<span class="fu">addClass</span>(<span class="st">"active"</span>);
<span class="kw">return</span> <span class="kw">false</span>;
});</code></pre>
<p>By removing duplication and making things clearer, we’ve ended up with a solution that is more readable, self documenting and maintainable. Notice how at first glance the two different sections of the code looked very different, but after some initial refactorings we found them actually to be near identical. This is a common occurrence - often a refactor will open up new possibilities and ways of working, which is another reason to keep your refactorings small and your mind open - an improvement might not be immediately obvious at the beginning.</p>
<h1 id="tale-2-cringey-carousels">Tale 2: Cringey Carousels</h1>
<p>In this chapter I want to talk about the value of some “quick wins” - very simple refactorings that, when applied to a codebase, will hugely improve the readability and maintainability of the project, at very little time and effort to yourself. You may find yourself too busy to fully refactor a huge method, but with these small steps you can make large gains quickly.</p>
<h2 id="the-carousel">The Carousel</h2>
<p>I’ve put together a simple enough jQuery Carousel which boasts the following feature set:</p>
<ul>
<li>‘Left’ and ‘Right’ links to navigate the carousel</li>
<li>Moves automatically every 10 seconds</li>
<li>If you hit the page with a hash such as <code>#image2</code>, it will move the carousel to that image.</li>
</ul>
<p>Hardily a fully featured carousel but it covers the basic functionality of most carousels I’ve seen, and those features provide enough scope for me to produce bad code that we can tidy. Take a deep breath and we’ll dive in.</p>
<p>First, there’s the HTML. Pretty simple and standard really:</p>
<pre class="sourceCode html"><code class="sourceCode html"><span class="dt"><!DOCTYPE </span>html<span class="dt">></span>
<span class="kw"><html></span>
<span class="kw"><head></span>
<span class="kw"><title></title></span>
<span class="kw"><link</span><span class="ot"> rel=</span><span class="st">"stylesheet"</span><span class="ot"> href=</span><span class="st">"css/style.css"</span><span class="kw">></span>
<span class="kw"><script</span><span class="ot"> src=</span><span class="st">"js/jquery.min.js"</span><span class="kw">></script></span>
<span class="kw"><script</span><span class="ot"> src=</span><span class="st">"js/app.js"</span><span class="kw">></script></span>
<span class="kw"></head></span>
<span class="kw"><body></span>
<span class="kw"><div</span><span class="ot"> class=</span><span class="st">"wrapper"</span><span class="kw">></span>
<span class="kw"><ul></span>
<span class="kw"><li><img</span><span class="ot"> src=</span><span class="st">"http://placekitten.com/300/300"</span><span class="ot"> alt=</span><span class="st">"Kitten"</span> <span class="kw">/></li></span>
<span class="kw"><li><img</span><span class="ot"> src=</span><span class="st">"http://placekitten.com/300/300"</span><span class="ot"> alt=</span><span class="st">"Kitten"</span> <span class="kw">/></li></span>
<span class="kw"><li><img</span><span class="ot"> src=</span><span class="st">"http://placekitten.com/300/300"</span><span class="ot"> alt=</span><span class="st">"Kitten"</span> <span class="kw">/></li></span>
<span class="kw"><li><img</span><span class="ot"> src=</span><span class="st">"http://placekitten.com/300/300"</span><span class="ot"> alt=</span><span class="st">"Kitten"</span> <span class="kw">/></li></span>
<span class="kw"><li><img</span><span class="ot"> src=</span><span class="st">"http://placekitten.com/300/300"</span><span class="ot"> alt=</span><span class="st">"Kitten"</span> <span class="kw">/></li></span>
<span class="kw"></ul></span>
<span class="kw"><div</span><span class="ot"> class=</span><span class="st">"controls"</span><span class="kw">></span>
<span class="kw"><a</span><span class="ot"> href=</span><span class="st">"#"</span><span class="ot"> class=</span><span class="st">"left"</span><span class="kw">></span>Left<span class="kw"></a></span>
<span class="kw"><a</span><span class="ot"> href=</span><span class="st">"#"</span><span class="ot"> class=</span><span class="st">"right"</span><span class="kw">></span>Right<span class="kw"></a></span>
<span class="kw"><span></span></span>
<span class="kw"></div></span>
<span class="kw"></div></span>
<span class="kw"></body></span>
<span class="kw"></html></span></code></pre>
<p>There is also some CSS applied to make it look good, but we won’t be focusing on that.</p>
<p>Finally, the JavaScript:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="fu">$</span>(<span class="kw">function</span>() {
<span class="kw">if</span>(<span class="ot">location</span>.<span class="fu">hash</span> && <span class="ot">location</span>.<span class="ot">hash</span>.<span class="fu">indexOf</span>(<span class="st">"image"</span>) > -<span class="dv">1</span>) {
<span class="kw">var</span> number = <span class="fu">parseInt</span>(<span class="ot">location</span>.<span class="ot">hash</span>.<span class="fu">charAt</span>(<span class="ot">location</span>.<span class="ot">hash</span>.<span class="fu">length</span> -<span class="dv">1</span>));
<span class="fu">$</span>(<span class="st">"ul"</span>).<span class="fu">animate</span>({
<span class="st">"margin-left"</span>: number * -<span class="dv">300</span>
}, <span class="kw">function</span>() {
currentImage = number;
<span class="fu">$</span>(<span class="st">".controls span"</span>).<span class="fu">text</span>(<span class="st">"Current: "</span> + (currentImage + <span class="dv">1</span>));
});
}
<span class="kw">var</span> timeout = <span class="fu">setTimeout</span>(<span class="kw">function</span>() {
<span class="fu">$</span>(<span class="st">".left"</span>).<span class="fu">trigger</span>(<span class="st">"click"</span>);
}, <span class="dv">10000</span>);
<span class="kw">var</span> currentImage = <span class="dv">0</span>;
<span class="fu">$</span>(<span class="st">".left"</span>).<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="fu">clearTimeout</span>(timeout);
<span class="kw">if</span>(currentImage == <span class="fu">$</span>(<span class="st">"li"</span>).<span class="fu">length</span> - <span class="dv">1</span>) {
<span class="fu">$</span>(<span class="st">"ul"</span>).<span class="fu">animate</span>({
<span class="st">"margin-left"</span>: <span class="dv">0</span>
}, <span class="kw">function</span>() {
currentImage = <span class="dv">0</span>;
<span class="fu">$</span>(<span class="st">".controls span"</span>).<span class="fu">text</span>(<span class="st">"Current: "</span> + (currentImage + <span class="dv">1</span>));
});
} <span class="kw">else</span> {
<span class="fu">$</span>(<span class="st">"ul"</span>).<span class="fu">animate</span>({
<span class="st">"margin-left"</span>: <span class="st">"-=300px"</span>
}, <span class="kw">function</span>() {
currentImage+=<span class="dv">1</span>;
<span class="fu">$</span>(<span class="st">".controls span"</span>).<span class="fu">text</span>(<span class="st">"Current: "</span> + (currentImage + <span class="dv">1</span>));
});
}
timeout = <span class="fu">setTimeout</span>(<span class="kw">function</span>() {
<span class="fu">$</span>(<span class="st">".left"</span>).<span class="fu">trigger</span>(<span class="st">"click"</span>);
}, <span class="dv">10000</span>);
<span class="kw">return</span> <span class="kw">false</span>;
});
<span class="fu">$</span>(<span class="st">".right"</span>).<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="fu">clearTimeout</span>(timeout);
<span class="kw">if</span>(currentImage == <span class="dv">0</span>) {
<span class="fu">$</span>(<span class="st">"ul"</span>).<span class="fu">animate</span>({
<span class="st">"margin-left"</span>: (<span class="fu">$</span>(<span class="st">"li"</span>).<span class="fu">length</span> - <span class="dv">1</span>) * -<span class="dv">300</span>
}, <span class="kw">function</span>() {
currentImage = <span class="fu">$</span>(<span class="st">"li"</span>).<span class="fu">length</span> - <span class="dv">1</span>;
<span class="fu">$</span>(<span class="st">".controls span"</span>).<span class="fu">text</span>(<span class="st">"Current: "</span> + (currentImage + <span class="dv">1</span>));
});
} <span class="kw">else</span> {
<span class="fu">$</span>(<span class="st">"ul"</span>).<span class="fu">animate</span>({
<span class="st">"margin-left"</span>: <span class="st">"+=300px"</span>
}, <span class="kw">function</span>() {
currentImage-=<span class="dv">1</span>;
<span class="fu">$</span>(<span class="st">".controls span"</span>).<span class="fu">text</span>(<span class="st">"Current: "</span> + (currentImage + <span class="dv">1</span>));
});
}
timeout = <span class="fu">setTimeout</span>(<span class="kw">function</span>() {
<span class="fu">$</span>(<span class="st">".left"</span>).<span class="fu">trigger</span>(<span class="st">"click"</span>);
}, <span class="dv">10000</span>);
<span class="kw">return</span> <span class="kw">false</span>;
});
});</code></pre>
<p>I think JavaScript like this is JavaScript we’ve all written before. I know I have. Take a moment to study it and see if you can spot the problems with it. There’s some much larger problems that we won’t look at right now, but you should be able to spot a lot of “quick wins” that we can take care of right here and now. I highly recommend noting down on paper a list of all the problems you spot and comparing them to the one I came up with below, to see what you might miss and to see if you identify things I didn’t.</p>
<p>I’ve split my list of problems into two parts. Firstly, the big problems:</p>
<ul>
<li><strong>Duplication</strong>. the same block of code is used multiple times to animate the margin. Additionally, the event handlers for the click on <code>.left</code> and <code>.right</code> are almost identical too, along with numerous others.</li>
<li><strong>Bad Selectors</strong>. There’s very little in the way of contextual selectors. What I mean by this is the selectors are too general, <code>$("ul")</code> for example.</li>
<li><strong>Magic Values</strong>. A new developer reading this would have no idea why the number <code>300</code> crops up so regularly. Nor would it be immediately obvious why this was sometimes negative 300.</li>
<li><strong>Reusing selectors</strong>. By my count there are <strong>15</strong> invocations of <code>$(thing)</code>, often with the same thing passed in as on a line previous.</li>
<li><strong>document ready abuse</strong>. All the code is within one <code>$(function() {})</code> block.</li>
</ul>
<p>Remember, <code>$(function() {})</code> is shorthand for <code>$(document).ready(function() {})</code>. Whenever you pass a function into <code>$()</code>, jQuery will presume you want it to run only when the document is ready.</p>
<p>And some things we can clean up immediately:</p>
<ul>
<li>Use of <code>return false</code>. Generally passing the event into the handler function and calling <code>e.preventDefault()</code> is preferred (I’ll discuss why in more detail shortly).</li>
<li>Using <code>click()</code> instead of the newer <code>on()</code> API.</li>
<li>Referencing the number <code>10000</code> more than once. What if the client decides this number should be <code>5000</code>? We’d have to change it in three separate places.</li>
<li>We can easily cache some selectors into variables, such as <code>$(".controls span")</code>.</li>
<li>We can look at abstracting some of the duplication into functions. For example, lines 23, 30, 46 and 53 are identical.</li>
</ul>
<h2 id="return-false-the-anti-pattern">return false; the anti-pattern</h2>
<p>Currently both the event handlers end with <code>return false</code>:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="fu">$</span>(<span class="st">".left"</span>).<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="co">// things happen here</span>
<span class="kw">return</span> <span class="kw">false</span>;
});
<span class="fu">$</span>(<span class="st">".right"</span>).<span class="fu">click</span>(<span class="kw">function</span>() {
<span class="co">// things happen here</span>
<span class="kw">return</span> <span class="kw">false</span>;
});</code></pre>
<p>This topic was first talked about back in 2010 in Doug Neiner’s article <a href="http://fuelyourcoding.com/jquery-events-stop-misusing-return-false/">“Stop (mis)using Return False</a> and is still very much relevant today.</p>
<p>jQuery event handlers take one argument, the <em>event object</em>. This object contains information about the event that triggered the event handler to fire. This object not only contains properties, such as the co-ordinates of the mouse pointer when the event took place, but also methods, including <code>preventDefault()</code> and <code>stopPropagation()</code>.</p>
<p><em>Normally</em> when a developer writes <code>return false</code>, what they actually want is to pass the event object in and call <code>event.preventDefault()</code>, like so:</p>
<pre><code>$(".right").click(function(event) {
// things happen here
event.preventDefault();
});</code></pre>
<p>As I’m sure you’re aware, <code>preventDefault()</code> prevents the default action being taken. <code>return false</code> has the same effect, but it does something else too.</p>
<p>Let’s just head out on a quick tangent to fully discuss propagation. Take a look at the code sample below:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="fu">$</span>(<span class="kw">function</span>() {
<span class="fu">$</span>(<span class="st">"div"</span>).<span class="fu">on</span>(<span class="st">"click"</span>, <span class="kw">function</span>() {
<span class="ot">console</span>.<span class="fu">log</span>(<span class="st">"div got clicked"</span>);
});
<span class="fu">$</span>(<span class="st">"div p"</span>).<span class="fu">on</span>(<span class="st">"click"</span>, <span class="kw">function</span>(e) {
<span class="ot">console</span>.<span class="fu">log</span>(<span class="st">"p got clicked"</span>);
<span class="ot">e</span>.<span class="fu">stopPropagation</span>();
});
});</code></pre>
<p>If you were to load that up in a browser and click on the <code>p</code> element within the <code>div</code> element, what would you see in the console? You would only see the second log statement, “p got clicked”. <code>event.stopPropagation()</code> <em>stops the event from bubbling up the DOM tree</em>. There are occasions when this is useful but the majority of the time, you don’t want this. What you might not realise though, is that <strong>return false; has the same effect</strong>. <code>return false</code> has the same effect as calling <code>preventDefault()</code> and <code>stopPropagation()</code>. This can lead to nasty side effects or bugs later which can be incredibly difficult and frustrating to deal with - trust me, I’ve been there.</p>
<p>So although it might take longer, it’s much better to be a bit more verbose here. If you only want the default action to be prevented, call <code>preventDefault()</code>. If you want propagation to be prevented, call <code>stopPropagation()</code>. If you want them both, <strong>don’t type</strong> <code>return false;</code>. Be explicit and type them both out:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="ot">event</span>.<span class="fu">stopPropagation</span>();
<span class="ot">event</span>.<span class="fu">preventDefault</span>();</code></pre>
<p>The question on where it makes sense to place calls to <code>preventDefault()</code> and <code>stopPropagation()</code> is largely down to you and your preference, but I like to put them at the top as the very first thing that happens in the method. That way it’s easily spotted if you or another developer is reading through the code to see what it does.</p>
<p>So for our first quick win, we can swap out <code>return false</code> with calls to <code>event.preventDefault()</code>. I’ve also put <code>event.preventDefault()</code> at the top, above the rest of the event handler code (which I’ve left out here just to save room).</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="fu">$</span>(<span class="st">".left"</span>).<span class="fu">click</span>(<span class="kw">function</span>(event) {
<span class="ot">event</span>.<span class="fu">preventDefault</span>();
<span class="co">// things happen here</span>
});
<span class="fu">$</span>(<span class="st">".right"</span>).<span class="fu">click</span>(<span class="kw">function</span>(event) {
<span class="ot">event</span>.<span class="fu">preventDefault</span>();
<span class="co">// things happen here</span>
});</code></pre>
<h2 id="on-and-off">on() and off()</h2>
<p>In jQuery 1.7 there was a new API introduced for binding and unbinding event handlers in the form of <code>on()</code> and <code>off()</code> to supersede the old API which was (and still is) a myriad of methods like <code>click()</code>, <code>hover()</code>, <code>mouseout()</code> along with <code>live()</code>, <code>bind()</code> and so on. This point may be a bit contentious, but I think that <code>on()</code> and <code>off()</code> are absolutely vast improvements and the fact that the entire event binding API was able to be reduced to two methods is brilliant. Of course, the old methods are not going anywhere (imagine how much code would break!) but as a rule I now will never use <code>click()</code> or similar, and will always use <code>on("click", function() {})</code>. This isn’t going to bring you huge speed improvements or even gain any readability, but personally I do think it reads slightly nicer.</p>
<p>This one’s easy. Just swap out the calls to <code>click</code> with <code>on</code>:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="fu">$</span>(<span class="st">".left"</span>).<span class="fu">on</span>(<span class="st">"click"</span>, <span class="kw">function</span>(event) {
<span class="ot">event</span>.<span class="fu">preventDefault</span>();
<span class="co">// things happen here</span>
});
<span class="fu">$</span>(<span class="st">".right"</span>).<span class="fu">on</span>(<span class="st">"click"</span>, <span class="kw">function</span>(event) {
<span class="ot">event</span>.<span class="fu">preventDefault</span>();
<span class="co">// things happen here</span>
});</code></pre>
<h2 id="repeated-numbers">Repeated Numbers</h2>
<p>Our code has some numbers that crop up time and time again. The first is <code>300</code>, which I’d refer to as a “Magic Number” and we’ll tackle separately. The second is <code>10000</code>. This isn’t so much a magic number in my opinion as it’s not connected with the page as much. <code>300</code> refers to the width of an image, but it’s not immediately apparent looking at the code that that is the case. We should treat it differently, and we will. <code>10000</code> has no connections, it’s just simply the time we decided should be between each automatic progression of our carousel.</p>
<p>If I were in another language that has <em>constants</em>, I’d define this value as a constant at the top of the file. For example, if this was the language Ruby I could simply do:</p>
<pre class="sourceCode ruby"><code class="sourceCode ruby"><span class="dt">CAROUSEL_TRANSITION_TIME</span> = <span class="dv">10000</span></code></pre>
<p>That constant would then be set to 10,000 and nothing could possibly change it later on.</p>
<p>Although JavaScript doesn’t have constants, a convention has formed that any variable in capital letters should be treated as such. So I’d actually type exactly what I typed above in the Ruby example, and place it towards the top of the JavaScript file:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="fu">$</span>(<span class="kw">function</span>() {
CAROUSEL_TRANSITION_TIME = <span class="dv">10000</span>;
<span class="kw">if</span>(<span class="ot">location</span>.<span class="fu">hash</span> && <span class="ot">location</span>.<span class="ot">hash</span>.<span class="fu">indexOf</span>(<span class="st">"image"</span>) > -<span class="dv">1</span>) {
<span class="co">// more code</span></code></pre>
<p>And then I’d replace all occurrences of <code>10000</code> in the code with <code>CAROUSEL_TRANSITION_TIME</code>:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> timeout = <span class="fu">setTimeout</span>(<span class="kw">function</span>() {
<span class="fu">$</span>(<span class="st">".left"</span>).<span class="fu">trigger</span>(<span class="st">"click"</span>);
}, CAROUSEL_TRANSITION_TIME);</code></pre>
<p>And similarly in the other two places it occurs.</p>
<p>By doing this we’ve definitely made this code more maintainable. Say the client turns up and wants the value changed. Now we’ve just one value to change, instead of three. Try to get into the habit of defining things as constants early. You can always remove the constant if you end up using it once, but it’s a good practice to get into if you find yourself referring to the same value over and over again.</p>
<p>I> Remember that constants should never be altered - in languages with actual support for constants, you are unable to edit them, you should treat your makeshift JavaScript constants equally.</p>
<h2 id="caching-selectors">Caching selectors</h2>
<p>This is something that everybody <em>should</em> do but people don’t always do it (I know I’m guilty). jQuery makes it so easy to quickly query the DOM for something that it’s easy to just keep doing it and pay no attention to if you’ve done that previously or not. The common argument for this is largely performance. Whilst there is an obvious performance increase if you can avoid doing something multiple times, I would argue that today the main reason behind this should be the maintainability of your code. In the previous section we just swapped out occurrences of <code>10000</code> with a constant which took us from 3 changes down to 1 if that number should change.</p>
<p>Take a look at the carousel code and selectors that come up time and time again. There’s not too many unique selectors but they all occur multiple times:</p>
<ul>
<li><code>ul</code> (5 times)</li>
<li><code>.controls span</code> (5 times)</li>
<li><code>.left</code> (4 times)</li>
<li><code>.right</code> (once)</li>
</ul>
<p>The fix for this is simple, and I’m sure you already know what’s coming up. <strong>Cache those selectors!</strong>.</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> ul = <span class="fu">$</span>(<span class="st">"ul"</span>);
<span class="kw">var</span> controlText = <span class="fu">$</span>(<span class="st">".controls span"</span>);
<span class="kw">var</span> leftLink = <span class="fu">$</span>(<span class="st">".left"</span>);
<span class="kw">var</span> rightLink = <span class="fu">$</span>(<span class="st">".right"</span>);</code></pre>
<p>Now go through and replace all occurrences of each selector with the relevant variable. What we’ve achieved here is a much easier job, should any of these selectors change. If you take anything away from this section, make it be this:</p>
<p><strong>Anything that could change should only be referenced <em>once</em> in your code</strong>.</p>
<p>Make changes and alterations as frictionless as possible and everyone’s happy.</p>
<h2 id="functions">functions</h2>
<p>We’re going to look more in depth at functions in a later chapter, where we’ll discuss their usage and some technical details in depth. This section can serve as a precursor to that.</p>
<p>A lot of people I talk to seem wary of using functions, like they come with some huge cost that people can’t afford. In other communities like the Ruby one (which I’ll reference purely because it’s the one I’m most familiar with) it’s very common to see posts heavily advocating using methods in Ruby. Ben Orenstein talks heavily about how he’s incredibly aggressive at abstracting code into new methods and having methods of extremely short length. I agree with Ben’s approach entirely we can certainly take some of what he says and apply it to our code.</p>
<p>As I said, we’ll go much more into this later, but for now let’s look at one quick example. This very line crops up four times:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="ot">controlText</span>.<span class="fu">text</span>(<span class="st">"Current: "</span> + (currentImage + <span class="dv">1</span>));</code></pre>
<p>By abstracting this line into its own function we can gain a large amount:</p>
<ol type="1">
<li><em>Maintainability</em>. This code contains something that very much could change - the text that we put into the page to show the user what number they are on. Right now, that change would have to be made in four places.</li>
<li><em>Readability</em>. If you extract code into a function and name that function well, it’s then less code for a developer to have to read to understand the functionality. If, instead of the line above, we just had a call to <code>updateControlText()</code>, I can understand what that means <em>instantly</em>, and move on. I can understand what that means a lot quicker than I can understand what <code>controlText.text("Current: " + (currentImage + 1));</code> means.</li>
<li>Plus, it’s a first step towards properly structuring our code, something we’ll also look into in more detail later.</li>
</ol>
<p>Let’s make the change. At the top of the code, just below all your selector variables, add the function:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> updateControlText = <span class="kw">function</span>() {
<span class="ot">controlText</span>.<span class="fu">text</span>(<span class="st">"Current: "</span> + (currentImage + <span class="dv">1</span>));
};</code></pre>
<p>And then we can update the code to make use of it:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="ot">ul</span>.<span class="fu">animate</span>({
<span class="st">"margin-left"</span>: number * -<span class="dv">300</span>
}, <span class="kw">function</span>() {
currentImage = number;
<span class="fu">updateControlText</span>();
});</code></pre>
<p>(And in the other three locations too).</p>
<h2 id="summary">Summary</h2>
<p>With that change we’ve made <strong>five</strong> really quick and simple changes that have already improved our code hugely, with very little time spent making them. As you get used to spotting these problems and fixing them, you’ll find it becomes almost second nature to make them all the time, without thinking.</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="fu">$</span>(<span class="kw">function</span>() {
<span class="kw">var</span> CAROUSEL_TRANSITION_TIME = <span class="dv">10000</span>;
<span class="kw">var</span> ul = <span class="fu">$</span>(<span class="st">"ul"</span>);
<span class="kw">var</span> controlText = <span class="fu">$</span>(<span class="st">".controls span"</span>);
<span class="kw">var</span> leftLink = <span class="fu">$</span>(<span class="st">".left"</span>);
<span class="kw">var</span> rightLink = <span class="fu">$</span>(<span class="st">".right"</span>);
<span class="kw">var</span> updateControlText = <span class="kw">function</span>() {
<span class="ot">controlText</span>.<span class="fu">text</span>(<span class="st">"Current: "</span> + (currentImage + <span class="dv">1</span>));
};
<span class="kw">if</span>(<span class="ot">location</span>.<span class="fu">hash</span> && <span class="ot">location</span>.<span class="ot">hash</span>.<span class="fu">indexOf</span>(<span class="st">"image"</span>) > -<span class="dv">1</span>) {
<span class="kw">var</span> number = <span class="fu">parseInt</span>(<span class="ot">location</span>.<span class="ot">hash</span>.<span class="fu">charAt</span>(<span class="ot">location</span>.<span class="ot">hash</span>.<span class="fu">length</span> -<span class="dv">1</span>));
<span class="ot">ul</span>.<span class="fu">animate</span>({
<span class="st">"margin-left"</span>: number * -<span class="dv">300</span>
}, <span class="kw">function</span>() {
currentImage = number;
<span class="fu">updateControlText</span>();
});
}
<span class="kw">var</span> timeout = <span class="fu">setTimeout</span>(<span class="kw">function</span>() {
<span class="ot">leftLink</span>.<span class="fu">trigger</span>(<span class="st">"click"</span>);
}, CAROUSEL_TRANSITION_TIME);
<span class="kw">var</span> currentImage = <span class="dv">0</span>;
<span class="ot">leftLink</span>.<span class="fu">on</span>(<span class="st">"click"</span>, <span class="kw">function</span>(event) {
<span class="ot">event</span>.<span class="fu">preventDefault</span>();
<span class="fu">clearTimeout</span>(timeout);
<span class="kw">if</span>(currentImage == <span class="fu">$</span>(<span class="st">"li"</span>).<span class="fu">length</span> - <span class="dv">1</span>) {
<span class="ot">ul</span>.<span class="fu">animate</span>({
<span class="st">"margin-left"</span>: <span class="dv">0</span>
}, <span class="kw">function</span>() {
currentImage = <span class="dv">0</span>;
<span class="fu">updateControlText</span>();
});
} <span class="kw">else</span> {
<span class="ot">ul</span>.<span class="fu">animate</span>({
<span class="st">"margin-left"</span>: <span class="st">"-=300px"</span>
}, <span class="kw">function</span>() {
currentImage+=<span class="dv">1</span>;
<span class="fu">updateControlText</span>();
});
}
timeout = <span class="fu">setTimeout</span>(<span class="kw">function</span>() {
<span class="ot">leftLink</span>.<span class="fu">trigger</span>(<span class="st">"click"</span>);
}, CAROUSEL_TRANSITION_TIME);
});
<span class="ot">rightLink</span>.<span class="fu">on</span>(<span class="st">"click"</span>, <span class="kw">function</span>(event) {
<span class="ot">event</span>.<span class="fu">preventDefault</span>();
<span class="fu">clearTimeout</span>(timeout);
<span class="kw">if</span>(currentImage == <span class="dv">0</span>) {
<span class="ot">ul</span>.<span class="fu">animate</span>({
<span class="st">"margin-left"</span>: (<span class="fu">$</span>(<span class="st">"li"</span>).<span class="fu">length</span> - <span class="dv">1</span>) * -<span class="dv">300</span>
}, <span class="kw">function</span>() {
currentImage = <span class="fu">$</span>(<span class="st">"li"</span>).<span class="fu">length</span> - <span class="dv">1</span>;
<span class="fu">updateControlText</span>();
});
} <span class="kw">else</span> {
<span class="ot">ul</span>.<span class="fu">animate</span>({
<span class="st">"margin-left"</span>: <span class="st">"+=300px"</span>
}, <span class="kw">function</span>() {
currentImage-=<span class="dv">1</span>;
<span class="fu">updateControlText</span>();
});
}
timeout = <span class="fu">setTimeout</span>(<span class="kw">function</span>() {
<span class="ot">leftLink</span>.<span class="fu">trigger</span>(<span class="st">"click"</span>);
}, CAROUSEL_TRANSITION_TIME);
});
});</code></pre>
<p>In the next chapter we will look more in depth at a smaller block of code I wrote recently that’s absolutely terrible, and see if we can’t improve it.</p>
<h1 id="tale-3-async-abominations">Tale 3: Async Abominations</h1>
<p>The code for this example comes directly from a project I was working on recently. I was building a NodeJS powered API, and as part of that, needed a way of validating parameters that were passed as query strings in the API request. Every single request needs to be passed a <code>token</code> parameter, which is matched to the token of a user in the database, and some requests take a <code>userId</code> parameter, along with a token. In the case that both the token and userId are passed in, we validate that the token is the token for that specific user. In the case where we just take in a token, we validate that the token is valid, and exists in our database.</p>
<p>The API has many routes, so I wanted to abstract this into a function, which I called <code>validateParamsExist</code>. It is used like so:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="fu">validateParamsExist</span>([<span class="st">'userId'</span>, <span class="st">'token'</span>, <span class="st">'foo'</span>], req, res, <span class="kw">function</span>(requestIsValid) {
<span class="kw">if</span>(requestIsValid) {
<span class="co">// all parameters were passed</span>
} <span class="kw">else</span> {
<span class="co">// something went wrong</span>
}
});</code></pre>
<p>The function takes four arguments: - an array of parameters to ensure exist - the ExpressJS request object (don’t worry if you don’t know what it is, all you need to know is that it stores all the information about the request) - the ExpressJS response object (this is what we use to return data from the API) - a callback function, which is called once the validation is complete with a single argument, which is <code>true</code> if the validations passed, and <code>false</code> if it did not.</p>
<p>It’s also important to note that the ExpressJS request object (in the code, I refer to it as <code>req</code>), stores the parameters that we got with the URL in an object, which is stored in <code>req.query</code>. So if I made a request that looked like this: <code>http://somesite.com/foo?name=jack&id=123</code>, <code>req.query</code> would look like so:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="ot">req</span>.<span class="fu">query</span> = {
<span class="dt">name</span>: <span class="st">'jack'</span>,
<span class="dt">id</span>: <span class="st">'123'</span>
};</code></pre>
<p>The challenge here and why this refactor offers a different challenge to the others is because it’s asynchronous. It performs tasks asynchronously and hence it’s a bit more of an effort to pull pieces out. The basic idea for the implementation of this function is as follows:</p>
<ol type="1">
<li>Loop over every parameter the user is expecting, and make sure it exists</li>
<li>If the parameter is <code>token</code>, do some extra validation (as explained above). The exact extra validation to be done depends on if we also have a <code>userId</code> parameter or not.</li>
</ol>
<h2 id="the-code">The code</h2>
<p>Rather than show you the starting code, which is badly written and tough to understand, I’m going to show you the finished code first. This is what my refactoring lead me to:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> matchTokenToUser = <span class="kw">function</span>(token, userId, errors, done) {
<span class="co">// method for making sure a token matches a user</span>
}
<span class="kw">var</span> ensureTokenExists = <span class="kw">function</span>(token, errors, done) {
<span class="co">// method for ensuring a token exists</span>
};
<span class="kw">var</span> noParamsPassed = <span class="kw">function</span>(req, res) {
<span class="kw">if</span>(<span class="ot">req</span>.<span class="fu">query</span>) {
<span class="kw">return</span> <span class="kw">false</span>;
} <span class="kw">else</span> {
<span class="ot">res</span>.<span class="fu">json</span>({ <span class="dt">errors</span>: [<span class="st">'no parameters supplied'</span>] });
<span class="kw">return</span> <span class="kw">true</span>;
}
};
<span class="kw">var</span> checkTokenAndIds = <span class="kw">function</span>(req, errors, cb) {
<span class="kw">var</span> token = <span class="ot">req</span>.<span class="ot">query</span>.<span class="fu">token</span>;
<span class="kw">var</span> userId = <span class="ot">req</span>.<span class="ot">query</span>.<span class="fu">userId</span>;
<span class="kw">if</span>(token) {
<span class="kw">if</span>(userId) {
<span class="fu">matchTokenToUser</span>(token, userId, errors, cb);
} <span class="kw">else</span> {
<span class="fu">ensureTokenExists</span>(token, errors, cb);
}
} <span class="kw">else</span> {
<span class="fu">cb</span>();
}
};
<span class="kw">var</span> validateParamsExist = <span class="kw">function</span>(params, req, res, cb) {
<span class="kw">if</span>(<span class="fu">noParamsPassed</span>(req, res)) <span class="kw">return</span> <span class="fu">cb</span>(<span class="kw">false</span>);
<span class="kw">var</span> errors = [];
<span class="ot">params</span>.<span class="fu">forEach</span>(<span class="kw">function</span>(p) {
<span class="kw">if</span>(!<span class="ot">req</span>.<span class="fu">query</span>[p]) <span class="ot">errors</span>.<span class="fu">push</span>(<span class="st">'parameter '</span> + p + <span class="st">' is required'</span>);
});
<span class="fu">checkTokenAndIds</span>(req, errors, <span class="kw">function</span>() {
<span class="kw">if</span>(<span class="ot">errors</span>.<span class="fu">length</span> > <span class="dv">0</span>) {
<span class="ot">res</span>.<span class="fu">json</span>({ <span class="dt">errors</span>: errors });
<span class="kw">return</span> <span class="fu">cb</span>(<span class="kw">false</span>);
} <span class="kw">else</span> {
<span class="kw">return</span> <span class="fu">cb</span>(<span class="kw">true</span>);
}
});
};</code></pre>
<p>Have a read through the code, starting with the <code>validateParamsExist</code> method, and see if it makes sense. I have left some implementation details of functions out, because it plays no part in the refactoring (the method we’ll start with shortly also has <code>ensureTokenExists</code> and <code>matchTokenToUser</code>, too).</p>
<p>Stepping through the <code>validateParamsExist</code> method, here’s what it does:</p>
<ol type="1">
<li>If no parameters at all were passed, call the callback and pass in <code>false</code>, because the validation failed.</li>
<li>Go through each parameter we are expecting, and make sure that it actually exists. If it doesn’t, store an error in the <code>errors</code> array.</li>
<li>Check to see if the token and id parameters were passed, and ensure that they are as expected.</li>
<li>Once the checking of tokens and id are complete, check to see if the <code>errors</code> array has any items. If it does, use <code>res.json</code> to return those errors from the API, and call the callback with <code>false</code>, because the validation failed.</li>
<li>Else, if we have no errors, call the callback with <code>true</code>, because the validation must have passed.</li>
</ol>
<h2 id="back-to-the-beginning">Back to the beginning</h2>
<p>I showed you the finished version first because it’s readable and easy to digest what is going on, everything the first implementation isn’t. What we’ll do now is look at the pre-refactoring code and then step through the refactorings I made to get to the above code. Brace yourself, because here is the previous version:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> matchTokenToUser = <span class="kw">function</span>(token, userId, errors, done) {
<span class="co">// implementation irrelevant</span>
}
<span class="kw">var</span> ensureTokenExists = <span class="kw">function</span>(token, errors, done) {
<span class="co">// implementation irrelevant</span>
};
<span class="kw">var</span> validateParamsExist = <span class="kw">function</span>(params, req, res, cb) {
<span class="kw">if</span>(!<span class="ot">req</span>.<span class="fu">query</span>) {
<span class="ot">res</span>.<span class="fu">json</span>({ <span class="dt">errors</span>: [<span class="st">'no parameters supplied'</span>] });
<span class="kw">return</span> <span class="fu">cb</span>(<span class="kw">false</span>);
} <span class="kw">else</span> {
<span class="kw">var</span> errors = [];
<span class="ot">async</span>.<span class="fu">each</span>(params, <span class="kw">function</span>(p, callback) {
<span class="kw">if</span>(!<span class="ot">req</span>.<span class="fu">query</span>[p]) {
<span class="ot">errors</span>.<span class="fu">push</span>(<span class="st">'parameter '</span> + p + <span class="st">' is required'</span>);
<span class="fu">callback</span>();
} <span class="kw">else</span> {
<span class="kw">if</span>(p === <span class="st">'token'</span> && <span class="ot">req</span>.<span class="ot">query</span>.<span class="fu">token</span>) {
<span class="kw">if</span>(<span class="ot">params</span>.<span class="fu">indexOf</span>(<span class="st">'userId'</span>) > -<span class="dv">1</span> && <span class="ot">req</span>.<span class="ot">query</span>.<span class="fu">userId</span>) {
<span class="fu">matchTokenToUser</span>(<span class="ot">req</span>.<span class="ot">query</span>.<span class="fu">token</span>, <span class="ot">req</span>.<span class="ot">query</span>.<span class="fu">userId</span>, errors, callback);
} <span class="kw">else</span> {
<span class="fu">ensureTokenExists</span>(<span class="ot">req</span>.<span class="ot">query</span>.<span class="fu">token</span>, errors, callback);
}
} <span class="kw">else</span> {
<span class="fu">callback</span>();
}
}
}, <span class="kw">function</span>(err) {
<span class="kw">if</span>(<span class="ot">errors</span>.<span class="fu">length</span> > <span class="dv">0</span>) {
<span class="ot">res</span>.<span class="fu">json</span>({ <span class="dt">errors</span>: errors });
<span class="kw">return</span> <span class="fu">cb</span>(<span class="kw">false</span>);
} <span class="kw">else</span> {
<span class="kw">return</span> <span class="fu">cb</span>(<span class="kw">true</span>);
}
});
};
}</code></pre>
<p>Take a moment to read through that and see what’s going on. The good news is it can’t get any worse than this - things are only going to get better from here!</p>
<h2 id="abstracting-functions">Abstracting functions</h2>
<p>Before I even begin to look at the main block of code, that starts with the call to <code>async.each</code>, I like to immediately abstract out small blocks into functions. This is the kind of change that I might undo at a later point, but I find it really helps as a starting point to just split one large method up into a bunch of smaller functions if possible. The first bit we can do that for is the first part of our function, the <code>if(!req.query)...</code> part:</p>
<pre class="sourceCode javascript"><code class="sourceCode javascript"><span class="kw">var</span> noParamsPassed = <span class="kw">function</span>(req, res) {
<span class="kw">if</span>(<span class="ot">req</span>.<span class="fu">query</span>) {
<span class="kw">return</span> <span class="kw">false</span>;
} <span class="kw">else</span> {
<span class="ot">res</span>.<span class="fu">json</span>({ <span class="dt">errors</span>: [<span class="st">'no parameters supplied'</span>] });
<span class="kw">return</span> <span class="kw">true</span>;
}
};
<span class="kw">var</span> validateParamsExist = <span class="kw">function</span>(params, req, res, cb) {
<span class="kw">if</span>(<span class="fu">noParamsPassed</span>(req, res)) <span class="kw">return</span> <span class="fu">cb</span>(<span class="kw">false</span>);
<span class="kw">var</span> errors = [];
<span class="ot">async</span>.<span class="fu">each</span>(params, <span class="kw">function</span>(p, callback) {
<span class="kw">if</span>(!<span class="ot">req</span>.<span class="fu">query</span>[p]) {
<span class="ot">errors</span>.<span class="fu">push</span>(<span class="st">'parameter '</span> + p + <span class="st">' is required'</span>);
<span class="fu">callback</span>();
} <span class="kw">else</span> {
<span class="kw">if</span>(p === <span class="st">'token'</span> && <span class="ot">req</span>.<span class="ot">query</span>.<span class="fu">token</span>) {
<span class="kw">if</span>(<span class="ot">params</span>.<span class="fu">indexOf</span>(<span class="st">'userId'</span>) > -<span class="dv">1</span> && <span class="ot">req</span>.<span class="ot">query</span>.<span class="fu">userId</span>) {