361–370 of 376 posts
When I receive a PR that has actual problems, I usually make a branch from the code in question, and submit a PR to that branch with tests highlighting the problem, or the beginnings of a refactor that I'm asking for. I often don't have time to fully flesh out the problems, but helping to write the software with the requestor makes for much faster and effective feedback than plain text comments.
Earlier quoted context omitted.
Preface: I was also very glad to see this called out specifically, and think it's a great rule. That said... > Note how Google does NOT say "make sure the code is properly architected". is not accurate. The very first paragraph on the same page is: > Design > The most important thing to cover in a review is the overall design of the CL. Do the interactions of various pieces of code in the CL make sense? Does this cha…
But that sure sounds like they care about proper architecture. Of course they care about architecture—I don’t think anyone implied otherwise. But at good companies like Google good architecture is a given. Top developers often fall into the pit of over-engineering, and almost never under-architect. So at top companies code reviewers have to be vigilant about over-engineering and rarely have to worry about under-archi…
>In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect. This is a great rule of thumb
Imagine some new features such as “support exporting to CSV” or “auto-generate avatars for new users”. Implementing them will necessarily add more code, and more complexity, to the codebase. And adding more complexity makes the code harder to work with – in other words, harms “the overall code health of the system”. Businesses develop new features because of the improvements to the product they bring and the reputational or monetary gains that come from that, in spite of the damage done to the codebase. The only way you could get a new-feature CL to pass by the standard you quoted (the standard of “improving the overall code health of the system being worked on”) is to refactor existing messy code written by someone else, and that would not be sustainable.
Does anyone see a way to interpret the rule, or have a preferred variation of the rule, that makes it useful for evaluating new-feature CLs?
Earlier quoted context omitted.
I agree with you. This "Agile" cancer that has infected the industry needs to die now. Someone here called it collectivized micromanagement and that's the best term I have heard for what's become of "Agile".
I don’t think you should accept bastardizations of terms into your lexicon. Yes, acknowledge when someone means “micromanaging” when they say “agile”, and act accordingly. But don’t redefine the word in your own head, otherwise how can you even speak? Like, what word do you use now for the basic principle of agile development now that you’ve changed it to mean micromanagement in your head?
Now, if you've been fortunate to work in places that more closely adhere to the original ideas and concepts that's great; count yourself fortunate.
Earlier quoted context omitted.
I agree with you. This "Agile" cancer that has infected the industry needs to die now. Someone here called it collectivized micromanagement and that's the best term I have heard for what's become of "Agile".
I don’t think you should accept bastardizations of terms into your lexicon. Yes, acknowledge when someone means “micromanaging” when they say “agile”, and act accordingly. But don’t redefine the word in your own head, otherwise how can you even speak? Like, what word do you use now for the basic principle of agile development now that you’ve changed it to mean micromanagement in your head?
----------------------------------
category (n.)
1580s, in Aristotle's logic, "a highest notion," from Middle French catégorie, from Late Latin categoria, from Greek kategoria "accusation, prediction, category," verbal noun from kategorein "to speak against; to accuse, assert, predicate," from kata "down to" (or perhaps "against;" see cata-) + agoreuein "to harangue, to declaim (in the assembly)," from agora "public assembly" (from PIE root *ger- "to gather").
Here's the part that resonated with me most: A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system. Reviewers should be especially vigilant about over-engineering. Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates…
Here's the part that resonated with me most: A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system. Reviewers should be especially vigilant about over-engineering. Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates…
Too bad the Google developer API's didn't go through that review. 200 lines of code (LOC) for a simple "Hello world" and that is using libraries that are probably a million LOC. Which could probably just be a HTTP GET, and run with a simple curl command.
Earlier quoted context omitted.
Too bad the Google developer API's didn't go through that review. 200 lines of code (LOC) for a simple "Hello world" and that is using libraries that are probably a million LOC. Which could probably just be a HTTP GET, and run with a simple curl command.
Any particular APIs you have in mind? I find App Engine’s ‘getting started’ examples quite sane while they are ‘hello world’.
Earlier quoted context omitted.
> Any claims about speed, efficiency, or performance whether in comments or during the review must have accompanying microbenchmarks or they should be deleted. That's way too strict. If I want to suggest moving a statement that's inside a loop to the outside, I don't want to waste my time writing microbenchmarks showing that it improves performance. If the suggestion is complicated or non-obvious, and it's in a reall…
I'm not saying you should write code in a performance-oblivious fashion. I am saying that if you want to do something non-canonical, or you want an exception to style guide or other established guidelines, or if you write a comment in your code that says "This function is faster than std::foo", then you must have evidence. In the absence of evidence, adhere to normal rules. I came across a comment in google base libr…
You would want to ban that sort of comment?
If you see a piece of code written oddly, then someone telling you why they wrote it that way is very helpful. If later you have to refactor it then it's doubly nice to know the reason it was written that way doesn't apply any more.
I suspect your problem is with pre-mature optimisation rather than commenting, and if so I imagine the majority of programmers would share your views. But if that is the case banning comments that make it plain something may have been pre-maturely optimised doesn't seem like a good way of solving the problem.
Earlier quoted context omitted.
Some places I've worked explicitly won't push/deploy/similar on Friday.
I worked at one (horrible) place where we explicitly only deployed on Friday afternoon. The logic was we have fewer users over the weekend, so if something goes wrong, fewer people will notice.
Some who administers some SAP thing for a huge supermarket chain tells me they do all of their upgrades on weekends because their users aren't at work.
Makes sense for them, but it would be a crazy thing to do the consumer-focused interweb company that I work for.