4 ms·

I echo the author's point in "Review the code with its author in mind". Without comments, sometimes it's really really difficult to navigate the code. I have b

I echo the author's point in "Review the code with its author in mind". Without comments, sometimes it's really really difficult to navigate the code. I have been adding more comments than ever: don't assume every line is obvious, write a comment to explain what the next few lines really do. # base case: stop dividing when we find the largest square. if width == height: return width, height else: # otherwise, we know that we can break the land into several # pieces, some are already square-sized, but there must be # left over, which is the difference of the width and height, # and can be divided again. remain = abs(width - height) return largest_square_plot(remain, min([width, height])) ^ This code is only for me to read so I didn't really care much about grammar... but a year from now I shouldn't have trouble understand the code in a minute or two. Some of my function/method has a pretty long docstring which may include explaining the rationale, and perhaps even some ascii diagrams. If you have trouble understand a piece of code after a few passes, that's a bad code. Also, use more newlines... > I check every Github email I get; I make sure that I don’t get notified for everything that happens in the repo Not sure about others, but I am tired of reading PR notifications in my mailbox. I don't know how kernel developers can live with this. I have been thinking about just build a bot. * receives PUSH from GitHub * adds events to a queue * notifies based on priority * pings me once in a while to remind me that I have outstanding PRs to review (as reviewer and as author of the patch). If someone needs me to review right away, he/she can reach out to me directly in chat.

yorwba 9y ago
On a tangent: Assuming the first line is "def largest_square_plot(width, height):", you may be interested to know that your code computes the greatest common divisor [1]. If I'd been the one to write this function, I would have done it like: def largest_square_plot(width, height): """Computes the largest square to tile a plot of the given width and height.""" # because we want the grid of squares to fit exactly # the size of the squares needs to divide both width and height # to get the largest square, we use the greatest common divisor from math import gcd square_size = gcd(width, height) return (square_size, square_size) ... but now I realize that probably not everyone is intuitively familiar with the kind of math that involves the GCD, so your code is actually much more intuitive without that background. [1] https://en.wikipedia.org/wiki/Greatest_common_divisor https://en.wikipedia.org/wiki/Greatest_common_divisor
- deleted 9y ago
  [deleted]
xerophyte12932 9y ago
A good counter argument to Comments: https://blog.codinghorror.com/coding-without-comments/ https://blog.codinghorror.com/coding-without-comments/
- Stratoscope 9y ago
  That article doesn't make a compelling case. It suggests taking this code: // square root of n with Newton-Raphson approximation r = n / 2; while ( abs( r - (n/r) ) > t ) { r = 0.5 * ( r + (n/r) ); } System.out.println( "r = " + r ); And refactoring it to this function: private double SquareRootApproximation(n) { r = n / 2; while ( abs( r - (n/r) ) > t ) { r = 0.5 * ( r + (n/r) ); } return r; } System.out.println( "r = " + SquareRootApproximation(r) ); I'm all for this refactoring, but something was lost in the process. What kind of square root approximation is being used? Does the algorithm have a name? What would I search for if I wanted to read more about it? That information was in the original comment.
  - macobo 9y ago
    There's an infinite amount of detail that's impossible to capture in a comment and which invariably changes over time and doesn't hold in the future. For my team, the solution has been writing longer commit messages detailing not only what has changed, but also the why and other considerations, potential pitfalls and so forth. So in this case, a good commit message might read like: ``` Created square root approximation function This is needed for rendering new polygons in renderer Foo in an efficient way as those don't need high degree of accuracy. The algorithm used was Newton-Raphson approximation, accuracy was chosen by initial testing: [[Test code here showing why a thing was chosen]] Potential pitfalls here include foo and bar. X and Y were also considered, but left out due to unclear benefit over the simpler algorithm. ``` With an editor with good `git blame` support (or using github to dig through the layers) this gives me a lot of confidence about reading code as I can go back in time and read what the author was thinking about originally. This way I can evaluate properly if conditions have changed, rather than worry about the next Chthulu comment that does not apply.
yaccz 9y ago
* receive email from GitHub / git-send-email / whatever * Maildir/new is queue * more emails in the same thread bump priority * emails marked unread are outstanding PRs
snarf21 9y ago
I recently read an article that was linked from HN comments on a different topic. It spoke about how software is not about code or documentation but rather about building complex mental models of the systems, which the author called the Theory Building View. It was interesting to read but one thing popped into my head while reading it. Too often we make comments about what a function does, which may be necessary but is not sufficient. My light bulb clicked on and I thought that what we really need is to document the function's reason for existing. This doesn't change even if the code inside does and is the thing that you actually really care about as it gives you some idea about the architecture.
DenisM 9y ago
FYI: If your variables are floats you will be splitting hair by the time this code terminates.
darkerside 9y ago
> I echo the author's point in "Review the code with its author in mind". I don't understand how the rest of your post relates to that, although I think it's an interesting point and following discussion. On the topic of reviewing code with the author in mind, I'm not sure I agree at all with the linked article. Does it matter who wrote the code in any way? Good code is good, and bad code is bad. It may be a helpful hint to remember the author was a senior engineer (who may "know more" than you do), but is it really something to keep in mind the entire time you review?
- cobbzilla 9y ago
  If you know your team well, it will help you keep an eye out for common mistakes they've made in the past. It may also help adjust your tone, as developers you've worked with for a long time will understand light humor or other well-intended comments that might be read as off-putting by newer devs.
  - darkerside 9y ago
    The anti-pattern to avoid here is assuming code written by senior engineers is inherently "better" in some way than that by a junior. Yes, it typically is, but the code should speak for itself. Ad hominem assumptions add little value. Similar to blind testing in musical auditions. http://gap.hks.harvard.edu/orchestrating-impartiality-impact-“blind”-auditions-female-musicians http://gap.hks.harvard.edu/orchestrating-impartiality-impact...

yorwba 9y ago

On a tangent: Assuming the first line is "def largest_square_plot(width, height):", you may be interested to know that your code computes the greatest common divisor [1]. If I'd been the one to write this function, I would have done it like: def largest_square_plot(width, height): """Computes the largest square to tile a plot of the given width and height.""" # because we want the grid of squares to fit exactly # the size of the squares needs to divide both width and height # to get the largest square, we use the greatest common divisor from math import gcd square_size = gcd(width, height) return (square_size, square_size) ... but now I realize that probably not everyone is intuitively familiar with the kind of math that involves the GCD, so your code is actually much more intuitive without that background. [1] https://en.wikipedia.org/wiki/Greatest_common_divisor https://en.wikipedia.org/wiki/Greatest_common_divisor

deleted 9y ago
[deleted]

xerophyte12932 9y ago

A good counter argument to Comments: https://blog.codinghorror.com/coding-without-comments/ https://blog.codinghorror.com/coding-without-comments/

Stratoscope 9y ago
That article doesn't make a compelling case. It suggests taking this code: // square root of n with Newton-Raphson approximation r = n / 2; while ( abs( r - (n/r) ) > t ) { r = 0.5 * ( r + (n/r) ); } System.out.println( "r = " + r ); And refactoring it to this function: private double SquareRootApproximation(n) { r = n / 2; while ( abs( r - (n/r) ) > t ) { r = 0.5 * ( r + (n/r) ); } return r; } System.out.println( "r = " + SquareRootApproximation(r) ); I'm all for this refactoring, but something was lost in the process. What kind of square root approximation is being used? Does the algorithm have a name? What would I search for if I wanted to read more about it? That information was in the original comment.
- macobo 9y ago
  There's an infinite amount of detail that's impossible to capture in a comment and which invariably changes over time and doesn't hold in the future. For my team, the solution has been writing longer commit messages detailing not only what has changed, but also the why and other considerations, potential pitfalls and so forth. So in this case, a good commit message might read like: ``` Created square root approximation function This is needed for rendering new polygons in renderer Foo in an efficient way as those don't need high degree of accuracy. The algorithm used was Newton-Raphson approximation, accuracy was chosen by initial testing: [[Test code here showing why a thing was chosen]] Potential pitfalls here include foo and bar. X and Y were also considered, but left out due to unclear benefit over the simpler algorithm. ``` With an editor with good `git blame` support (or using github to dig through the layers) this gives me a lot of confidence about reading code as I can go back in time and read what the author was thinking about originally. This way I can evaluate properly if conditions have changed, rather than worry about the next Chthulu comment that does not apply.

yaccz 9y ago

* receive email from GitHub / git-send-email / whatever * Maildir/new is queue * more emails in the same thread bump priority * emails marked unread are outstanding PRs

snarf21 9y ago

I recently read an article that was linked from HN comments on a different topic. It spoke about how software is not about code or documentation but rather about building complex mental models of the systems, which the author called the Theory Building View. It was interesting to read but one thing popped into my head while reading it. Too often we make comments about what a function does, which may be necessary but is not sufficient. My light bulb clicked on and I thought that what we really need is to document the function's reason for existing. This doesn't change even if the code inside does and is the thing that you actually really care about as it gives you some idea about the architecture.

DenisM 9y ago

FYI: If your variables are floats you will be splitting hair by the time this code terminates.

darkerside 9y ago

> I echo the author's point in "Review the code with its author in mind". I don't understand how the rest of your post relates to that, although I think it's an interesting point and following discussion. On the topic of reviewing code with the author in mind, I'm not sure I agree at all with the linked article. Does it matter who wrote the code in any way? Good code is good, and bad code is bad. It may be a helpful hint to remember the author was a senior engineer (who may "know more" than you do), but is it really something to keep in mind the entire time you review?

cobbzilla 9y ago
If you know your team well, it will help you keep an eye out for common mistakes they've made in the past. It may also help adjust your tone, as developers you've worked with for a long time will understand light humor or other well-intended comments that might be read as off-putting by newer devs.
- darkerside 9y ago
  The anti-pattern to avoid here is assuming code written by senior engineers is inherently "better" in some way than that by a junior. Yes, it typically is, but the code should speak for itself. Ad hominem assumptions add little value. Similar to blind testing in musical auditions. http://gap.hks.harvard.edu/orchestrating-impartiality-impact-“blind”-auditions-female-musicians http://gap.hks.harvard.edu/orchestrating-impartiality-impact...