My code is well commented. //eslint-disable-line all over the place.
Invoking eslint-disable needs to have comment of it's own justifying why the line in question is being skipped. What use is the linter if we just disable it everytime it complains?
How I review code
61–70 of 144 posts
Re: How I review code
#62Earlier quoted context omitted.
> Once the less than ideal code is in the codebase it's there to stay. That is not objective fact across projects. That means either the process or culture is bad.
I've never seen a project where you go back to fix something just because its implementation is not ideal. If it works it stays, because there is always something more important to do.
I do that regularly. Not really when it is less then ideal, I don't think less then ideal necessary constitutes technical debt. But when it was hard to read and I just had to read it, when it was hard to modify and I had to modify it. Or when I was modifying something nearby and this was for free. We also have occasional actions like "remove this particular code smell from everywhere where you find it" and explicit "refactor that part" issues that are routinely assigned as part of sprints.
Obviously, we do weight how much time it takes to fix it vs how much of the debt it actually is. We would not had that much independence in these decisions if we did not.
Re: How I review code
#63Earlier quoted context omitted.
> Once the less than ideal code is in the codebase it's there to stay. That is not objective fact across projects. That means either the process or culture is bad.
I've never seen a project where you go back to fix something just because its implementation is not ideal. If it works it stays, because there is always something more important to do.
This, this and this.
You get told to quickly write something as a Proof of Concept. Then get told, it works so put it in the code and reelase it. We will rewrite it later.
You never get to rewrite it later.
Over time the code base goes to hell. Every single time
Re: How I review code
#64I 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…
> 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?
Re: How I review code
#65> We have repositories for the PHP backend, our database schemas, our iOS (Swift/Obj-C) and Android (Java/Kotlin) mobile apps, infrastructure projects written in Go, C/C++, Lua, Ruby, Perl, and many other projects written in Scala, Node.js, Python, and more Why do organizations allow this? I realize that some platforms require their own languages (iOS, Android), but outside of that, just pick one or two and hold the…
Totally agree with this. I've had to pick up the pieces on more than one occasion when some key piece of code was written in the flavour of the month and the writer shortly left. I've heard other Devs say you should pick the right language for the job but I think there should be a heavy bias to what the rest of the code base already is using
Re: How I review code
#66I don't understand why you need to know the progamer behind the code ?
You need to be totaly impartial when you judge something.
So i think it's a wrong way to review the code. You don't need the WHO but the WHY.
Re: How I review code
#67> We have repositories for the PHP backend, our database schemas, our iOS (Swift/Obj-C) and Android (Java/Kotlin) mobile apps, infrastructure projects written in Go, C/C++, Lua, Ruby, Perl, and many other projects written in Scala, Node.js, Python, and more Why do organizations allow this? I realize that some platforms require their own languages (iOS, Android), but outside of that, just pick one or two and hold the…
> Why do organizations allow this?
While I generally agree with you on principle, I also feel you need to be pragmatic too. It’s important to also use the right tool for the job as long as you have sufficient engineers who can support that alternative language/framework/platform. Sometimes a language can also be favored at one point, but lose favor later, and in some cases it doesn’t make sense priority wise to rewrite it because it still works.
Re: How I review code
#68Earlier quoted context omitted.
I've never seen a project where you go back to fix something just because its implementation is not ideal. If it works it stays, because there is always something more important to do.
This, this and this. You get told to quickly write something as a Proof of Concept. Then get told, it works so put it in the code and reelase it. We will rewrite it later. You never get to rewrite it later. Over time the code base goes to hell. Every single time
I have hard time to believe you never refactor function you come across. I see how refactoring whole architecture or something major is avoided, but when it comes to smaller functions and pieces of code, refactoring it as you go takes around same amount of time as refactoring it during that detailed code review (e.g. often very little).
The really big mess tend to be emergent - when features and code base grew too much for original architecture over time. You cant prevent that one by detailed code review and it takes a lot of effort to fix it.
Re: How I review code
#69Earlier quoted context omitted.
>If reviewer suspects there is a bug, reviewer should check for it instead of having reviewee explain him or defend every detail. It is not a matter of if the reviewer "suspects there is a bug", but rather a matter of if the reviewer is convinced that there is not a bug. If the reviewer needs to have someone explain why every minor code change is correct, they are either lazy or incompetent (or the code is badly writ…
> However, being "okay" often goes beyond correctness, and into business decisions. Code reviews occur far too late in the development process for an uninvolved developer to provide a good, timely review of “bigger picture” concerns like business consideration (or software design) simply because the reviewing developer needs time to come up to speed on why decisions were made. In these cases it’s better to have the r…
Imo, business reasonability should be reviewed after the coder finished work, but not so much by programmer. It should be done by analyst, project manager or tester who knows a lot about the business. Basically, it should be done by person who communicated with customer or is responsible for overall vision.
Re: How I review code
#70> We have repositories for the PHP backend, our database schemas, our iOS (Swift/Obj-C) and Android (Java/Kotlin) mobile apps, infrastructure projects written in Go, C/C++, Lua, Ruby, Perl, and many other projects written in Scala, Node.js, Python, and more Why do organizations allow this? I realize that some platforms require their own languages (iOS, Android), but outside of that, just pick one or two and hold the…
Why? Having many languages allows to pick the sweetspot for each subsystem. Each language has special strengths and weaknesses, there is no silver bullet that excels at everything. Go, C/C++, Lua, Ruby, Perl, Scala, Node.js, Python... each of these are THE best choice for certain classes of problems (and terrible for others). It may be because of language features that elegantly express a solution, or particular efficiency considerations (speed, latency or memory footprint), or integrations with specific libraries (if language X has a lib that does 90% of the requirement, use it instead of reimplementing from scratch in language Y). Also, it may be a team consideration: the talent pool for front-end engineers has different preferences than the talent pool for some back-end systems.
In a large system like Tumblr, they are likely highly distributed with many different subsystems running in different parts of their infrastructure, each with their own SLA and resilience requirements.
Their team is probably large enough that most engineers focus on only some of the subsystems, and communication between them is via defined interfaces.
Though having a diverse ecosystem fosters an openess of mind, instead of enforcing "One Metaphore To Rule Them All". You find yourself borrowing efficient patterns from other environments, as they eventually start cross-pollinating.