11 ms·

Liam, this is incredible! I thought it might be useful for you to incrementally see how someone in industry would review or change this code, so here's a littl

by sgk284 8y ago

Liam, this is incredible! I thought it might be useful for you to incrementally see how someone in industry would review or change this code, so here's a little code review via video: https://youtu.be/UkVOrcS--04 https://youtu.be/UkVOrcS--04 We very incrementally build up to the final code, which can be found here: https://gist.github.com/stevekrenzel/b490564bf1c7f98e232a6c863bd066dd https://gist.github.com/stevekrenzel/b490564bf1c7f98e232a6c8... Hope you find it helpful!

LifeQuestioner 8y ago
I'm subscribing to you, just incase you do more video's on code reviews.
SmallDeadGuy 8y ago
Instead of this: (rule & (1 << i)) === 0 ? 0 : 1 You could just do: (rule >> i) & 1 Otherwise it was a good review! I'd probably also extract parts of this long line so it's not as cumbersome to read: return state.map((item, i) => rules[((state[i - 1] || 0) * 4) + (item * 2) + (state[i + 1] || 0)] ); to return state.map((item, i) => { const prevItem = state[i - 1] || 0; const nextItem = state[i + 1] || 0; return rules[(prevItem << 2) | (item << 1) | nextItem]; }); It also uses bitwise operations instead of arithmetic, even though they are equivalent in this context, simply because we are effectively operating on arrays of bits.
- DonHopkins 8y ago
  I like to break expressions across multiple lines, and line up any patterns that are repeated, so your eye can easily scan up and down and obviously see what's the same, and what's different. For example, instead of: var length = Math.sqrt((x * x) + (y * y)); You can go: var length = Math.sqrt((x * x) + (y * y)); That way you can see the similarity of squaring x, and squaring y. But breaking up the multiplications because they repeat the x's and y's would be taking it too far -- it always requires balance and thinking about how can I communicate my intent and the SHAPE of the problem I'm solving. You can also use indentation and formatting to bring out and make obvious other two-dimensional patterns and regularities, especially with two-dimensional cellular automata and graphics code. There are some examples of that technique in my 2D cellular automata machine code. (There's a lot of code, so don't be overwhelmed, since I've been working on that since around 1985 or so. So never give up and just stick to it! You don't need to understand the code, which is pretty esoteric, just notice the patterns.) https://github.com/SimHacker/CAM6/blob/master/javascript/CAM6.js#L2955 https://github.com/SimHacker/CAM6/blob/master/javascript/CAM... // Load the right two columns of the 3x3 window. n = cells[cellIndex - nextCol - nextRow]; ne = cells[cellIndex - nextRow]; c = cells[cellIndex - nextCol ]; e = cells[cellIndex ]; s = cells[cellIndex - nextCol + nextRow]; se = cells[cellIndex + nextRow]; [...] // Scroll the 3x3 window to the right, scrolling the middle and right // columns to the left, then scooping up three new cells from the right // leading edge. nw = n; n = ne; ne = cells[cellIndex + nextCol - nextRow]; w = c; c = e; e = cells[cellIndex + nextCol ]; sw = s; s = se; se = cells[cellIndex + nextCol + nextRow]; [...] var sum8 = (nw & 1) + (n & 1) + (ne & 1) + (w & 1) + (e & 1) + (sw & 1) + (s & 1) + (se & 1); [...] var tableIndex = (((c >> plane) & 0x03) << 0) | (((e >> plane) & 0x03) << 2) | (((w >> plane) & 0x03) << 4) | (((s >> plane) & 0x03) << 6) | (((n >> plane) & 0x03) << 8) | (((cellX ) & 0x01) << 10) | (((cellY ) & 0x01) << 11) | (((phaseTime ) & 0x01) << 12) | (((plane >> 1 ) & 0x03) << 13); [...] When the sub-expressions won't fit on one line, you can still use indentation to help the reader understand the spatial patterns: error += (nw * kernelBytes[4 - kernelDown - kernelRight]) + (n * kernelBytes[4 - kernelDown]) + (ne * kernelBytes[4 - kernelDown + kernelRight]) + (w * kernelBytes[4 - kernelRight]) + (c * kernelBytes[4]) + (e * kernelBytes[4 + kernelRight]) + (sw * kernelBytes[4 + kernelDown - kernelRight]) + (s * kernelBytes[4 + kernelDown]) + (se * kernelBytes[4 + kernelDown + kernelRight]) + frob; But that would probably be more readable if I used intermediate variables for the kernelBytes[...] expressions, gave them descriptive names matching their corresponding directions, and lined up all their calculations so you could see the similarities and differences, because right now all the kernleBytes[4 +/- kernelDown +/- kernelRight] expressions are jumbled around horizontally, when they could be lined up nicely by themselves if they weren't interleaved with the n/s/e/w/ne/nw/sw/se multiplications. (I'll leave that improvement as an exercise for the reader. ;) The point is you want to vertically line up as many of the repeated letters and symbols in nice neat columns as possible, so that your eye can easily see which are the same, and the ones that aren't stand out obviously so you can ignore all the same things and focus on the differences. (Which are typically variations like +/-, sin/cos, x/y/z, -1/0/1, nw/n/ne/w/c/e/sw/s/se, and longer series of names and numbers. Whatever changes should stick out visually, and be lined up and grouped together so your eyes can scan over them!) That not only helps other people understand your code, but it also helps you be sure that you wrote the right thing, what you actually meant, instead of making an easy to miss typo. The difficulty of reading you own code, which is HARD even for professional programmers, is that you usually see what you MEANT to write, not what you actually wrote. That's why it's so important to get other people to read your code, and to read other people's code, like pair programming and code reviews, or even explaining it to a duck. https://en.wikipedia.org/wiki/Rubber_duck_debugging https://en.wikipedia.org/wiki/Rubber_duck_debugging It takes a lot more effort and concentration to slow down and look at what's actually there, instead of what you want to be there. (That's true for many aspects of life...)
  - tokyodude 8y ago
    it infuriated me that Google's style guides forbid this kind of formatting. Anytime someone else tried to get them to allow it it was shouted down as ASCII art
    - SmallDeadGuy 8y ago
      If I'm honest, I like that kind of formatting for small 2-3 line, 2-3 part expressions. But I am not a big fan for it in much bigger parts (like that huge index calculation above). I always see them and think that I'd rather see a loop over an array of values, or split into many smaller expressions.
- sgk284 8y ago
  Thanks for the edits! I can't change the video, but did mention that both of those lines are a little clunky. I did update the gist though! Much cleaner.
earthscienceman 8y ago
I'm a long time coder and still liked watching your video just for watching someone else work in emacs. How do you get emacs to highlight all occurrences of map? Was it a keybinding for "highlight-phrase"?
- jstimpfle 8y ago
  This is vim.
  - cs02rm0 8y ago
    I think I just died.
    - dolessdrugs 8y ago
      WHAT YEAR IS IT
      - DonHopkins 8y ago
        Plausible deniability: It could have been Emacs pretending to be vim.
- sgk284 8y ago
  Thanks, glad you enjoyed it! And like another person already responded, this is vim with a split pane where the right pane is a `:terminal` (a relatively new feature of vim... though prior to that I would have just used tmux). Highlighting searches is enabled with `:set hlsearch`, and you can search for whatever word is under the cursor with ``. So I just type and it highlights all of the matching words. Nothing too magical!
kyberias 8y ago
Incredible code review! I wish this inspires clever people to produce huge amounts of parody-videos "refactoring" the same piece of code. :)
krat0sprakhar 8y ago
Kudos for taking the time for doing this! It's places like these where the HN community really shines. Thanks a lot!
- sgk284 8y ago
  Thanks! I really enjoyed making it. HN has been such a huge part of my life over that last ~10 years that it's truly the least I could do.
hashbig 8y ago
If you would start a channel where you review code of random public repos I would definitely subscribe. Well done.
- windlessstorm 8y ago
  Let us know here if you are planning to do it :)
- lynxaegon 8y ago
  +1
- kenmendin 8y ago
  +1
- magicduck 8y ago
  +1
- irmis 8y ago
  Just registered to +1 this
- fernandopj 8y ago
  +1 count me in
- Jach 8y ago
  Drive-by code reviews (and maybe other types of annotated "audits", like for security bugs) would be a nice feature source repo sites might consider adding one day. It seems like a missing practice in open source development, code reviews (if any) are done by active developers once on check-in/merge acceptance. But as we see here code reviews can be a useful way to pay it forward too, not just new code / issue fixes, and done out-of-band there's no pressure to bow to the reviewer just to get something in; you can just have conversations and learn things. The video format is a nice twist, in my distributed teams sometimes we've done "code review meetings" for some changes that are rather involved to capture some of the benefits of in-person / video code reviews.
- 8y ago
ronilan 8y ago
Steve, it is outstandingly awesome that you did this and also how you did this! Thanks.
- sgk284 8y ago
  Thank you, much appreciated!
liamilan 8y ago
Thanks Steve! I watched your video and although I don't understand everything, I learned a lot. Separating my program into functions is a good idea. Your video clarified the map function for me. Through your video, I understood that I was running code multiple times, when it only needs to run once (how I parsed my rules was one of these instances that you went over in your video), so thanks for pointing that out. Again, thank you so much for taking the time to review my code. I really enjoyed your feedback.
- giancarlostoro 8y ago
  > Again, thank you so much for taking the time to review my code. I really enjoyed your feedback. Whatever you do, hold on to that. Never be mad about feedback just try your best to understand it, and if it's wrong explain yourself without getting frustrated, this is worth more than a dozen senior developers who explode at feedback (I would argue they're junior developers :) and you will have a wonderful career. Also look for good mentors, and don't be afraid to ask questions about direct feedback, if you didn't understand something he said just ask (this becomes an immediate learning opportunity and you will be better off for it), even if he doesn't get to answer, someone else might. This is a field where learning is endless, you will always learn something new as a software developer, new platforms are made all the time to be programmed on / with. Having soft skills[0] will be the biggest asset you can have as a software developer, whether you do it for money or just for fun, if you have to collaborate in the open you will need to communicate with others. Nothing breaks a team like terrible communication and someone who rejects feedback out of pride. [0]: https://en.wikipedia.org/wiki/Soft_skills https://en.wikipedia.org/wiki/Soft_skills
- nkoren 8y ago
  This whole interaction is really reaffirming my faith in humanity. You guys are awesome! Liam -- this response is just as impressive as the code that you wrote. You're obviously very smart, but that isn't good enough on its own. I've seen very-smart people who aren't open to critique, who get defensive rather than take the opportunity to learn new things. They don't get very far in life. On the other hand, the attitude you're showing here is tremendously valuable, and will serve you incredibly well throughout life. Never let yourself grow out of it!
navinsylvester 8y ago
Nice of you Steve. Liam - whatever Steve suggested are very good pointers but please remember you are getting accolades here because you cared to publish your code. Don't hesitate to publish your initial version without any hesitation in future also.

by sgk284 8y ago

LifeQuestioner 8y ago

I'm subscribing to you, just incase you do more video's on code reviews.

SmallDeadGuy 8y ago

Instead of this: (rule & (1 << i)) === 0 ? 0 : 1 You could just do: (rule >> i) & 1 Otherwise it was a good review! I'd probably also extract parts of this long line so it's not as cumbersome to read: return state.map((item, i) => rules[((state[i - 1] || 0) * 4) + (item * 2) + (state[i + 1] || 0)] ); to return state.map((item, i) => { const prevItem = state[i - 1] || 0; const nextItem = state[i + 1] || 0; return rules[(prevItem << 2) | (item << 1) | nextItem]; }); It also uses bitwise operations instead of arithmetic, even though they are equivalent in this context, simply because we are effectively operating on arrays of bits.

DonHopkins 8y ago
I like to break expressions across multiple lines, and line up any patterns that are repeated, so your eye can easily scan up and down and obviously see what's the same, and what's different. For example, instead of: var length = Math.sqrt((x * x) + (y * y)); You can go: var length = Math.sqrt((x * x) + (y * y)); That way you can see the similarity of squaring x, and squaring y. But breaking up the multiplications because they repeat the x's and y's would be taking it too far -- it always requires balance and thinking about how can I communicate my intent and the SHAPE of the problem I'm solving. You can also use indentation and formatting to bring out and make obvious other two-dimensional patterns and regularities, especially with two-dimensional cellular automata and graphics code. There are some examples of that technique in my 2D cellular automata machine code. (There's a lot of code, so don't be overwhelmed, since I've been working on that since around 1985 or so. So never give up and just stick to it! You don't need to understand the code, which is pretty esoteric, just notice the patterns.) https://github.com/SimHacker/CAM6/blob/master/javascript/CAM6.js#L2955 https://github.com/SimHacker/CAM6/blob/master/javascript/CAM... // Load the right two columns of the 3x3 window. n = cells[cellIndex - nextCol - nextRow]; ne = cells[cellIndex - nextRow]; c = cells[cellIndex - nextCol ]; e = cells[cellIndex ]; s = cells[cellIndex - nextCol + nextRow]; se = cells[cellIndex + nextRow]; [...] // Scroll the 3x3 window to the right, scrolling the middle and right // columns to the left, then scooping up three new cells from the right // leading edge. nw = n; n = ne; ne = cells[cellIndex + nextCol - nextRow]; w = c; c = e; e = cells[cellIndex + nextCol ]; sw = s; s = se; se = cells[cellIndex + nextCol + nextRow]; [...] var sum8 = (nw & 1) + (n & 1) + (ne & 1) + (w & 1) + (e & 1) + (sw & 1) + (s & 1) + (se & 1); [...] var tableIndex = (((c >> plane) & 0x03) << 0) | (((e >> plane) & 0x03) << 2) | (((w >> plane) & 0x03) << 4) | (((s >> plane) & 0x03) << 6) | (((n >> plane) & 0x03) << 8) | (((cellX ) & 0x01) << 10) | (((cellY ) & 0x01) << 11) | (((phaseTime ) & 0x01) << 12) | (((plane >> 1 ) & 0x03) << 13); [...] When the sub-expressions won't fit on one line, you can still use indentation to help the reader understand the spatial patterns: error += (nw * kernelBytes[4 - kernelDown - kernelRight]) + (n * kernelBytes[4 - kernelDown]) + (ne * kernelBytes[4 - kernelDown + kernelRight]) + (w * kernelBytes[4 - kernelRight]) + (c * kernelBytes[4]) + (e * kernelBytes[4 + kernelRight]) + (sw * kernelBytes[4 + kernelDown - kernelRight]) + (s * kernelBytes[4 + kernelDown]) + (se * kernelBytes[4 + kernelDown + kernelRight]) + frob; But that would probably be more readable if I used intermediate variables for the kernelBytes[...] expressions, gave them descriptive names matching their corresponding directions, and lined up all their calculations so you could see the similarities and differences, because right now all the kernleBytes[4 +/- kernelDown +/- kernelRight] expressions are jumbled around horizontally, when they could be lined up nicely by themselves if they weren't interleaved with the n/s/e/w/ne/nw/sw/se multiplications. (I'll leave that improvement as an exercise for the reader. ;) The point is you want to vertically line up as many of the repeated letters and symbols in nice neat columns as possible, so that your eye can easily see which are the same, and the ones that aren't stand out obviously so you can ignore all the same things and focus on the differences. (Which are typically variations like +/-, sin/cos, x/y/z, -1/0/1, nw/n/ne/w/c/e/sw/s/se, and longer series of names and numbers. Whatever changes should stick out visually, and be lined up and grouped together so your eyes can scan over them!) That not only helps other people understand your code, but it also helps you be sure that you wrote the right thing, what you actually meant, instead of making an easy to miss typo. The difficulty of reading you own code, which is HARD even for professional programmers, is that you usually see what you MEANT to write, not what you actually wrote. That's why it's so important to get other people to read your code, and to read other people's code, like pair programming and code reviews, or even explaining it to a duck. https://en.wikipedia.org/wiki/Rubber_duck_debugging https://en.wikipedia.org/wiki/Rubber_duck_debugging It takes a lot more effort and concentration to slow down and look at what's actually there, instead of what you want to be there. (That's true for many aspects of life...)
- tokyodude 8y ago
  it infuriated me that Google's style guides forbid this kind of formatting. Anytime someone else tried to get them to allow it it was shouted down as ASCII art
  - SmallDeadGuy 8y ago
    If I'm honest, I like that kind of formatting for small 2-3 line, 2-3 part expressions. But I am not a big fan for it in much bigger parts (like that huge index calculation above). I always see them and think that I'd rather see a loop over an array of values, or split into many smaller expressions.
sgk284 8y ago
Thanks for the edits! I can't change the video, but did mention that both of those lines are a little clunky. I did update the gist though! Much cleaner.

earthscienceman 8y ago

I'm a long time coder and still liked watching your video just for watching someone else work in emacs. How do you get emacs to highlight all occurrences of map? Was it a keybinding for "highlight-phrase"?

jstimpfle 8y ago
This is vim.
- cs02rm0 8y ago
  I think I just died.
  - dolessdrugs 8y ago
    WHAT YEAR IS IT
    - DonHopkins 8y ago
      Plausible deniability: It could have been Emacs pretending to be vim.
sgk284 8y ago
Thanks, glad you enjoyed it! And like another person already responded, this is vim with a split pane where the right pane is a `:terminal` (a relatively new feature of vim... though prior to that I would have just used tmux). Highlighting searches is enabled with `:set hlsearch`, and you can search for whatever word is under the cursor with ``. So I just type and it highlights all of the matching words. Nothing too magical!

kyberias 8y ago

Incredible code review! I wish this inspires clever people to produce huge amounts of parody-videos "refactoring" the same piece of code. :)

krat0sprakhar 8y ago

Kudos for taking the time for doing this! It's places like these where the HN community really shines. Thanks a lot!

sgk284 8y ago
Thanks! I really enjoyed making it. HN has been such a huge part of my life over that last ~10 years that it's truly the least I could do.

hashbig 8y ago

If you would start a channel where you review code of random public repos I would definitely subscribe. Well done.

windlessstorm 8y ago
Let us know here if you are planning to do it :)
lynxaegon 8y ago
+1
kenmendin 8y ago
+1
magicduck 8y ago
+1
irmis 8y ago
Just registered to +1 this
fernandopj 8y ago
+1 count me in
Jach 8y ago
Drive-by code reviews (and maybe other types of annotated "audits", like for security bugs) would be a nice feature source repo sites might consider adding one day. It seems like a missing practice in open source development, code reviews (if any) are done by active developers once on check-in/merge acceptance. But as we see here code reviews can be a useful way to pay it forward too, not just new code / issue fixes, and done out-of-band there's no pressure to bow to the reviewer just to get something in; you can just have conversations and learn things. The video format is a nice twist, in my distributed teams sometimes we've done "code review meetings" for some changes that are rather involved to capture some of the benefits of in-person / video code reviews.
8y ago

ronilan 8y ago

Steve, it is outstandingly awesome that you did this and also how you did this! Thanks.

sgk284 8y ago
Thank you, much appreciated!

liamilan 8y ago

Thanks Steve! I watched your video and although I don't understand everything, I learned a lot. Separating my program into functions is a good idea. Your video clarified the map function for me. Through your video, I understood that I was running code multiple times, when it only needs to run once (how I parsed my rules was one of these instances that you went over in your video), so thanks for pointing that out. Again, thank you so much for taking the time to review my code. I really enjoyed your feedback.

giancarlostoro 8y ago
> Again, thank you so much for taking the time to review my code. I really enjoyed your feedback. Whatever you do, hold on to that. Never be mad about feedback just try your best to understand it, and if it's wrong explain yourself without getting frustrated, this is worth more than a dozen senior developers who explode at feedback (I would argue they're junior developers :) and you will have a wonderful career. Also look for good mentors, and don't be afraid to ask questions about direct feedback, if you didn't understand something he said just ask (this becomes an immediate learning opportunity and you will be better off for it), even if he doesn't get to answer, someone else might. This is a field where learning is endless, you will always learn something new as a software developer, new platforms are made all the time to be programmed on / with. Having soft skills[0] will be the biggest asset you can have as a software developer, whether you do it for money or just for fun, if you have to collaborate in the open you will need to communicate with others. Nothing breaks a team like terrible communication and someone who rejects feedback out of pride. [0]: https://en.wikipedia.org/wiki/Soft_skills https://en.wikipedia.org/wiki/Soft_skills
nkoren 8y ago
This whole interaction is really reaffirming my faith in humanity. You guys are awesome! Liam -- this response is just as impressive as the code that you wrote. You're obviously very smart, but that isn't good enough on its own. I've seen very-smart people who aren't open to critique, who get defensive rather than take the opportunity to learn new things. They don't get very far in life. On the other hand, the attitude you're showing here is tremendously valuable, and will serve you incredibly well throughout life. Never let yourself grow out of it!

navinsylvester 8y ago

Nice of you Steve. Liam - whatever Steve suggested are very good pointers but please remember you are getting accolades here because you cared to publish your code. Don't hesitate to publish your initial version without any hesitation in future also.