The Beauty of Bad Code

Given at Web Rebels in Oslo, Norway, 1-2 June 2017

Transcript

1. The Beauty of Bad Code brought to you by @rockbot
2. @rockbot #jsnz npm is a package manager for JavaScript
3. @rockbot #jsnz npm is a package manager
4. @rockbot #webrebels our #1 priority is to reduce friction
5. @rockbot #webrebels npm i npm@latest -g npm @5
6. @rockbot #webrebels we’ve written a lot of code
7. @rockbot #webrebels we’ve rewritten our code
8. @rockbot #webrebels we’ve rewritten our code several times
9. @rockbot #webrebels we know bad code
10. @rockbot #webrebels courtesy of npm wombats what is bad code?
11. @rockbot #webrebels functionality
12. @rockbot #webrebels functionality maintainability
13. @rockbot #webrebels functionality maintainability readability
14. @rockbot #webrebels functionality
15. @rockbot #webrebels super-villain code
16. @rockbot #webrebels 110 /* here's the potential situation: let's say 111 I'm a hacker and I make a 112 package that does Something Evil™ then I 113 add you as a collaborator `npm 114 adduser isaacs evil-pkg` and then I publish 115 the package and then I remove 116 myself from the package so it looks like YOU 117 are the one who made the 118 package well, that's nasty so we blocked that 119 from showing because 120 hypothetically your friends would be like, 121 hey! this evil-pkg from isaacs 122 looks awesome, let me use it! and then I get 123 all their bank account numbers 124 and get super duper rich and become a VC and 125 create LinkedIn for Cats */
17. @rockbot #webrebels doesn’t do the thing it’s supposed to do
18. @rockbot #webrebels 1 const tap = require('tap') 2 const urlOf = require('./lib/url') 3 4 require('./lib/sharedNemo').then(nemo => { 5 tap.test('User with feature flag gets to the 6 new org creation page', t => { 7 return nemo.driver.get(urlOf('/express')) 8 .then(() => nemo.view.page.h1WaitVisible() 9 ) 10 .then(() => nemo.view.page.h1TextEquals( 11 'express')) 12 }) 13 14 tap.test('quit', () => nemo.driver.quit()) 15 }) npm-website/integration/package-redirect.js
19. @rockbot #webrebels 1 const tap = require('tap') 2 const urlOf = require('./lib/url') 3 4 require('./lib/sharedNemo').then(nemo => { 5 tap.test('User with feature flag gets to the 6 new org creation page', t => { 7 return nemo.driver.get(urlOf('/express')) 8 .then(() => nemo.view.page.h1WaitVisible() 9 ) 10 .then(() => nemo.view.page.h1TextEquals( 11 'express')) 12 }) 13 14 tap.test('quit', () => nemo.driver.quit()) 15 }) npm-website/integration/package-redirect.js
20. @rockbot #webrebels 1 const tap = require('tap') 2 const urlOf = require('./lib/url') 3 4 require('./lib/sharedNemo').then(nemo => { 5 tap.test('User with feature flag gets to the 6 new org creation page', t => { 7 return nemo.driver.get(urlOf('/express')) 8 .then(() => nemo.view.page.h1WaitVisible() 9 ) 10 .then(() => nemo.view.page.h1TextEquals( 11 'express')) 12 }) 13 14 tap.test('quit', () => nemo.driver.quit()) 15 }) npm-website/integration/package-redirect.js
21. @rockbot #webrebels 1 const tap = require('tap') 2 const urlOf = require('./lib/url') 3 4 require('./lib/sharedNemo').then(nemo => { 5 tap.test('User with feature flag gets to the 6 new org creation page', t => { 7 return nemo.driver.get(urlOf('/express')) 8 .then(() => nemo.view.page.h1WaitVisible() 9 ) 10 .then(() => nemo.view.page.h1TextEquals( 11 'express')) 12 }) 13 14 tap.test('quit', () => nemo.driver.quit()) 15 }) npm-website/integration/package-redirect.js
22. @rockbot #webrebels security risks
23. @rockbot #webrebels 10 function getPage (type) { 11 12 return function (name, next) { 13 14 var url = fmt('https://raw. 15 githubusercontent.com/npm/' + 16 type + '/master/%s.md', name); 17 18 request(url, function (err, resp, content) { 19 ... 25 return next(err, marked.parse(content)); 26 }); 27 } 28 }
24. @rockbot #webrebels 10 function getPage (type) { 11 12 return function (name, next) { 13 14 var url = fmt('https://raw. 15 githubusercontent.com/npm/' + 16 type + '/master/%s.md', name); 17 18 request(url, function (err, resp, content) { 19 ... 25 return next(err, marked.parse(content)); 26 }); 27 } 28 }
25. @rockbot #webrebels 10 function getPage (type) { 11 12 return function (name, next) { 13 14 var url = fmt('https://raw. 15 githubusercontent.com/npm/' + 16 type + '/master/%s.md', name); 17
26. @rockbot #webrebels 10 function getPage (type) { 11 12 return function (name, next) { 13 14 var url = fmt('https://raw. 15 githubusercontent.com/npm/' + 16 type + '/master/%s.md', name); 17 system-set
27. @rockbot #webrebels 10 function getPage (type) { 11 12 return function (name, next) { 13 14 var url = fmt('https://raw. 15 githubusercontent.com/npm/' + 16 type + '/master/%s.md', name); 17 user-set system-set
28. @rockbot #webrebels 10 function getPage (type) { 11 12 return function (name, next) { 13 14 var url = fmt('https://raw. 15 githubusercontent.com/npm/' + 16 type + '/master/%s.md', name); 17 https://raw.githubusercontent.com/npm/newww/master/policies.md user-set system-set
29. @rockbot #webrebels 10 function getPage (type) { 11 12 return function (name, next) { 13 14 var url = fmt('https://raw. 15 githubusercontent.com/npm/' + 16 type + '/master/%s.md', name); 17 https://raw.githubusercontent.com/npm/newww/master/policies.md https://raw.githubusercontent.com/npm/newww/master/../../ corporate_secrets/all_the_passwords.md user-set system-set
30. @rockbot #webrebels happy path only
31. @rockbot #webrebels 9 browse.packagesByKeyword = function(request, 10 reply) { ... 20 21 Package(request.loggedInUser) 22 .list(options) 23 .then(function(result) { 24 context.items = chunk(result.results, 3); 25 paginate(request, options, result, 26 context); 27 return reply.view('browse/keyword', 28 context); 29 }); 30 }; 31
32. @rockbot #webrebels 9 browse.packagesByKeyword = function(request, 10 reply) { ... 20 21 Package(request.loggedInUser) 22 .list(options) 23 .then(function(result) { 24 context.items = chunk(result.results, 3); 25 paginate(request, options, result, 26 context); 27 return reply.view('browse/keyword', 28 context); 29 }).catch(err => reply(err)); 30 }; 31
33. @rockbot #webrebels maintainability
34. @rockbot #webrebels “spaghetti” code
35. @rockbot #webrebels
36. @rockbot #webrebels
37. @rockbot #webrebels code with unknown side effects
38. @rockbot #webrebels 65 const checkCache = getAccess( 66 this.authCacheUrl, 67 name, 68 pkg, 69 creds.method 70 ).then(reply => { 71 if (reply.status !== 'not-found' || creds.method !== 'PUT') { 72 return reply 73 } 74 return createPackage(this.userAclUrl, this.billingApiUrl, name, 75 pkg, { 76 access: creds.body ? creds.body.access : null, 77 team: creds.team ? creds.body.team : null 78 }).then(() => { 79 // retry the cache! 80 return getAccess( 81 this.authCacheUrl, 82 name, 83 pkg, 84 creds.method 85 ) 86 })
39. @rockbot #webrebels 65 const checkCache = getAccess( 66 this.authCacheUrl, 67 name, 68 pkg, 69 creds.method 70 ).then(reply => { 71 if (reply.status !== 'not-found' || creds.method !== 'PUT') { 72 return reply 73 } 74 return createPackage(this.userAclUrl, this.billingApiUrl, name, 75 pkg, { 76 access: creds.body ? creds.body.access : null, 77 team: creds.team ? creds.body.team : null 78 }).then(() => { 79 // retry the cache! 80 return getAccess( 81 this.authCacheUrl, 82 name, 83 pkg, 84 creds.method 85 ) 86 })
40. @rockbot #webrebels 65 const checkCache = getAccess( 66 this.authCacheUrl, 67 name, 68 pkg, 69 creds.method 70 ).then(reply => { 71 if (reply.status !== 'not-found' || creds.method !== 'PUT') { 72 return reply 73 } 74 return createPackage(this.userAclUrl, this.billingApiUrl, name, 75 pkg, { 76 access: creds.body ? creds.body.access : null, 77 team: creds.team ? creds.body.team : null 78 }).then(() => { 79 // retry the cache! 80 return getAccess( 81 this.authCacheUrl, 82 name, 83 pkg, 84 creds.method 85 ) 86 })
41. @rockbot #webrebels readability
42. @rockbot #webrebels “clever” code
43. @rockbot #webrebels 173 function transform (type, arg, data, skip, 174 limit) { ... 187 // normally has an arg. sort, and then 188 manually paginate. 189 if (!arg && transformKeyArg[type]) { 190 data = data.sort(function (a, b) { 191 return a.value === b.value ? ( 192 a.name === b.name ? 0 : a.name < b.name 193 ? -1 : 1 194 ) : a.value > b.value ? -1 : 1 195 }).slice(skip, skip + limit) 196 } 197 198 return data 199 }
44. @rockbot #webrebels 173 function transform (type, arg, data, skip, 174 limit) { ... 187 // normally has an arg. sort, and then 188 manually paginate. 189 if (!arg && transformKeyArg[type]) { 190 data = data.sort(function (a, b) { 191 return a.value === b.value ? ( 192 a.name === b.name ? 0 : a.name < b.name 193 ? -1 : 1 194 ) : a.value > b.value ? -1 : 1 195 }).slice(skip, skip + limit) 196 } 197 198 return data 199 }
45. @rockbot #webrebels too long for context
46. @rockbot #webrebels
47. @rockbot #webrebels
48. @rockbot #webrebels doesn’t match style standards
49. @rockbot #webrebels 1 var 2 BORING_EMOJI = require('emoji-list'), 3 FS = require('fs'), 4 MARKOV = require('markoff'), 5 MARK = new MARKOV(), 6 SLACK = require('@slack/client'), 7 SLACK_EVENTS = SLACK.CLIENT_EVENTS.RTM, 8 RTM_EVENTS = SLACK.RTM_EVENTS, 9 LOG = console.log 10 ; 11 12 function ISLOUD(MSG) 13 { 14 return MSG !== MSG.toLowerCase() && MSG === 15 MSG.toUpperCase(); 16 }
50. @rockbot #webrebels 1 var 2 BORING_EMOJI = require('emoji-list'), 3 FS = require('fs'), 4 MARKOV = require('markoff'), 5 MARK = new MARKOV(), 6 SLACK = require('@slack/client'), 7 SLACK_EVENTS = SLACK.CLIENT_EVENTS.RTM, 8 RTM_EVENTS = SLACK.RTM_EVENTS, 9 LOG = console.log 10 ; 11 12 function ISLOUD(MSG) 13 { 14 return MSG !== MSG.toLowerCase() && MSG === 15 MSG.toUpperCase(); 16 } tabs!
51. @rockbot #webrebels 1 var 2 BORING_EMOJI = require('emoji-list'), 3 FS = require('fs'), 4 MARKOV = require('markoff'), 5 MARK = new MARKOV(), 6 SLACK = require('@slack/client'), 7 SLACK_EVENTS = SLACK.CLIENT_EVENTS.RTM, 8 RTM_EVENTS = SLACK.RTM_EVENTS, 9 LOG = console.log 10 ; 11 12 function ISLOUD(MSG) 13 { 14 return MSG !== MSG.toLowerCase() && MSG === 15 MSG.toUpperCase(); 16 } tabs! 4 spaces!
52. @rockbot #webrebels 1 var 2 BORING_EMOJI = require('emoji-list'), 3 FS = require('fs'), 4 MARKOV = require('markoff'), 5 MARK = new MARKOV(), 6 SLACK = require('@slack/client'), 7 SLACK_EVENTS = SLACK.CLIENT_EVENTS.RTM, 8 RTM_EVENTS = SLACK.RTM_EVENTS, 9 LOG = console.log 10 ; 11 12 function ISLOUD(MSG) 13 { 14 return MSG !== MSG.toLowerCase() && MSG === 15 MSG.toUpperCase(); 16 } tabs! 4 spaces! weird new-lined curly braces!
53. @rockbot #webrebels 1 var 2 BORING_EMOJI = require('emoji-list'), 3 FS = require('fs'), 4 MARKOV = require('markoff'), 5 MARK = new MARKOV(), 6 SLACK = require('@slack/client'), 7 SLACK_EVENTS = SLACK.CLIENT_EVENTS.RTM, 8 RTM_EVENTS = SLACK.RTM_EVENTS, 9 LOG = console.log 10 ; 11 12 function ISLOUD(MSG) 13 { 14 return MSG !== MSG.toLowerCase() && MSG === 15 MSG.toUpperCase(); 16 } tabs! 4 spaces! semicolons! weird new-lined curly braces!
54. @rockbot #webrebels 1 var 2 BORING_EMOJI = require('emoji-list'), 3 FS = require('fs'), 4 MARKOV = require('markoff'), 5 MARK = new MARKOV(), 6 SLACK = require('@slack/client'), 7 SLACK_EVENTS = SLACK.CLIENT_EVENTS.RTM, 8 RTM_EVENTS = SLACK.RTM_EVENTS, 9 LOG = console.log 10 ; 11 12 function ISLOUD(MSG) 13 { 14 return MSG !== MSG.toLowerCase() && MSG === 15 MSG.toUpperCase(); 16 } tabs! 4 spaces! hanging semicolons! semicolons! weird new-lined curly braces!
55. @rockbot #webrebels 1 var 2 BORING_EMOJI = require('emoji-list'), 3 FS = require('fs'), 4 MARKOV = require('markoff'), 5 MARK = new MARKOV(), 6 SLACK = require('@slack/client'), 7 SLACK_EVENTS = SLACK.CLIENT_EVENTS.RTM, 8 RTM_EVENTS = SLACK.RTM_EVENTS, 9 LOG = console.log 10 ; 11 12 function ISLOUD(MSG) 13 { 14 return MSG !== MSG.toLowerCase() && MSG === 15 MSG.toUpperCase(); 16 } tabs! 4 spaces! hanging semicolons! semicolons! weird new-lined curly braces! STOP
56. @rockbot #webrebels there’s nothing wrong with this code
57. @rockbot #webrebels style is personal
58. @rockbot #webrebels use a linter. get over it.
59. @rockbot #webrebels how to minimize bad code
60. @rockbot #webrebels common code smells
61. @rockbot #webrebels many branching paths
62. @rockbot #webrebels context-dependent understanding many branching paths
63. @rockbot #webrebels context-dependent understanding weird/long variable names many branching paths
64. @rockbot #webrebels “works on my machine”
65. @rockbot #webrebels “not invented here” (NIH) syndrome “works on my machine”
66. @rockbot #webrebels “not invented here” (NIH) syndrome know it will break “works on my machine”
67. @rockbot #webrebels useful code cleansers
68. @rockbot #webrebels pair programming
69. @rockbot #webrebels commit early and often
70. @rockbot #webrebels write tests
71. @rockbot #webrebels use a linter
72. @rockbot #webrebels code reviews
73. @rockbot #webrebels holistic overview
74. @rockbot #webrebels commit by commit holistic overview
75. @rockbot #webrebels commit by commit holistic overview line by line
76. @rockbot #webrebels when is bad code good ?
77. @rockbot #webrebels the beauty and horror of code is you get to see how other people think — CJ Silverio (@ceejbot) CTO of npm, Inc
78. @rockbot #webrebels don’t know it’s bad at the time
79. @rockbot #webrebels learning opportunity
80. @rockbot #webrebels “good enough”
81. @rockbot #webrebels signal to refactor
82. @rockbot #webrebels more than just code
83. @rockbot #webrebels 999 // ----------------------------------------------------------- 1000 // apply a given rotation to all the fingers in the hand 1001 // ----------------------------------------------------------- 1002 function applyRotToAllFingers(fingers, origin_palm, rotAngles) 1003 { 1005 for (var f = 0; f < fingers.length; ++f) 1006 { 1007 var thisFinger = fingers[f]; 1008 1010 for (var p = 0; p < thisFinger.points3D.length; ++p) 1011 { 1013 var vip = RotVectorAboutOrigin(thisFinger.points3D[p], 1014 origin_palm, rotAngles, true); 1015 thisFinger.visPoints[p] = new Vector(vip[0], vip[1], vip[2]); 1016 } 1017 1020 var pt0 = thisFinger.visPoints[1].v; 1021 var pt1 = thisFinger.visPoints[2].v; 1022 vip = [pt1[0] - pt0[0], pt1[1] - pt0[1], pt1[2] - pt0[2]]; 1023 thisFinger.visPoints[1] = new Vector(pt0[0] + vip[0] / 3, pt0[1] 1024 + vip[1] / 3, pt0[2] + vip[2] / 3);
84. @rockbot #webrebels 999 // ----------------------------------------------------------- 1000 // apply a given rotation to all the fingers in the hand 1001 // ----------------------------------------------------------- 1002 function applyRotToAllFingers(fingers, origin_palm, rotAngles) 1003 { 1005 for (var f = 0; f < fingers.length; ++f) 1006 { 1007 var thisFinger = fingers[f]; 1008 1010 for (var p = 0; p < thisFinger.points3D.length; ++p) 1011 { 1013 var vip = RotVectorAboutOrigin(thisFinger.points3D[p], 1014 origin_palm, rotAngles, true); 1015 thisFinger.visPoints[p] = new Vector(vip[0], vip[1], vip[2]); 1016 } 1017 1020 var pt0 = thisFinger.visPoints[1].v; 1021 var pt1 = thisFinger.visPoints[2].v; 1022 vip = [pt1[0] - pt0[0], pt1[1] - pt0[1], pt1[2] - pt0[2]]; 1023 thisFinger.visPoints[1] = new Vector(pt0[0] + vip[0] / 3, pt0[1] 1024 + vip[1] / 3, pt0[2] + vip[2] / 3); lol python-y
85. @rockbot #webrebels 999 // ----------------------------------------------------------- 1000 // apply a given rotation to all the fingers in the hand 1001 // ----------------------------------------------------------- 1002 function applyRotToAllFingers(fingers, origin_palm, rotAngles) 1003 { 1005 for (var f = 0; f < fingers.length; ++f) 1006 { 1007 var thisFinger = fingers[f]; 1008 1010 for (var p = 0; p < thisFinger.points3D.length; ++p) 1011 { 1013 var vip = RotVectorAboutOrigin(thisFinger.points3D[p], 1014 origin_palm, rotAngles, true); 1015 thisFinger.visPoints[p] = new Vector(vip[0], vip[1], vip[2]); 1016 } 1017 1020 var pt0 = thisFinger.visPoints[1].v; 1021 var pt1 = thisFinger.visPoints[2].v; 1022 vip = [pt1[0] - pt0[0], pt1[1] - pt0[1], pt1[2] - pt0[2]]; 1023 thisFinger.visPoints[1] = new Vector(pt0[0] + vip[0] / 3, pt0[1] 1024 + vip[1] / 3, pt0[2] + vip[2] / 3); so C++ lol python-y
86. @rockbot #webrebels 999 // ----------------------------------------------------------- 1000 // apply a given rotation to all the fingers in the hand 1001 // ----------------------------------------------------------- 1002 function applyRotToAllFingers(fingers, origin_palm, rotAngles) 1003 { 1005 for (var f = 0; f < fingers.length; ++f) 1006 { 1007 var thisFinger = fingers[f]; 1008 1010 for (var p = 0; p < thisFinger.points3D.length; ++p) 1011 { 1013 var vip = RotVectorAboutOrigin(thisFinger.points3D[p], 1014 origin_palm, rotAngles, true); 1015 thisFinger.visPoints[p] = new Vector(vip[0], vip[1], vip[2]); 1016 } 1017 1020 var pt0 = thisFinger.visPoints[1].v; 1021 var pt1 = thisFinger.visPoints[2].v; 1022 vip = [pt1[0] - pt0[0], pt1[1] - pt0[1], pt1[2] - pt0[2]]; 1023 thisFinger.visPoints[1] = new Vector(pt0[0] + vip[0] / 3, pt0[1] 1024 + vip[1] / 3, pt0[2] + vip[2] / 3); so C++ too-clever variable name lol python-y
87. @rockbot #webrebels 999 // ----------------------------------------------------------- 1000 // apply a given rotation to all the fingers in the hand 1001 // ----------------------------------------------------------- 1002 function applyRotToAllFingers(fingers, origin_palm, rotAngles) 1003 { 1005 for (var f = 0; f < fingers.length; ++f) 1006 { 1007 var thisFinger = fingers[f]; 1008 1010 for (var p = 0; p < thisFinger.points3D.length; ++p) 1011 { 1013 var vip = RotVectorAboutOrigin(thisFinger.points3D[p], 1014 origin_palm, rotAngles, true); 1015 thisFinger.visPoints[p] = new Vector(vip[0], vip[1], vip[2]); 1016 } 1017 1020 var pt0 = thisFinger.visPoints[1].v; 1021 var pt1 = thisFinger.visPoints[2].v; 1022 vip = [pt1[0] - pt0[0], pt1[1] - pt0[1], pt1[2] - pt0[2]]; 1023 thisFinger.visPoints[1] = new Vector(pt0[0] + vip[0] / 3, pt0[1] 1024 + vip[1] / 3, pt0[2] + vip[2] / 3); context-specific so C++ too-clever variable name lol python-y
88. @rockbot #webrebels
89. @rockbot #webrebels
90. @rockbot #webrebels bad code is part of our origin stories
91. @rockbot #webrebels we’ve all done it
92. @rockbot #webrebels change languages === start over* PSA:
93. @rockbot #webrebels change languages === start over* PSA: * varies with experience
94. @rockbot #webrebels what’s wrong with bad code?
95. @rockbot #webrebels we mock bad code
96. @rockbot #webrebels beginners are afraid to publish for fear of mocking
97. @rockbot #webrebels unhealthy attachment to code
98. @rockbot #webrebels source of shame
99. @rockbot #webrebels code is ultimately zeros and ones FYI:
100. @rockbot #webrebels code is ultimately zeros and ones FYI: BUT
101. @rockbot #webrebels code is an act of communication to your colleagues and your future self FYI:
102. @rockbot #webrebels writing good code is hard FYI:
103. @rockbot #webrebels accept the bad code to become great
104. @rockbot #webrebels Code http://rckbt.me/2014/10/code/
105. @rockbot #webrebels Your code is not a reflection of you. It isn’t a reflection of your beliefs, your upbringing, or your ability to be a good person.
106. @rockbot #webrebels Your code is, however, a reflection of your thinking process at the time that you wrote it.
107. @rockbot #webrebels Given our innate ability to change our minds, consider other viewpoints, and play with new ideas, why do we hold our code so dear?
108. @rockbot #webrebels Your code can change. Your code will change. Your code must change, if it’s ever going to get better.
109. @rockbot #webrebels Stop marrying your code.
110. @rockbot #webrebels The sooner you accept this, the happier you will be and the better programmer you will become.
111. Raquel Vélez @rockbot http://rckbt.me raquel@ js.com