GitHub taught me to micromanage(matthewrocklin.com)
matthewrocklin.com
GitHub taught me to micromanage
https://matthewrocklin.com/feedback.html
65 comments
The gist you're trying to convey is fine: Prioritize those things that _directly_ cause meaningful damage to maintainability.
But a code base that is stylistically inconsistent is itself a source of unmaintainability. That's probably a case of "perfection is the enemy of good enough"; a multi-year, multi-member project perhaps cannot reasonably be expected to consistently pick e.g. a list comprehension instead of a for loop under similar circumstances.
However, just stating casually that it is _irrelevant_, or that it is hopeless, feels like giving up a bit too quickly.
One would assume that the cost to morale etc is overblown (if your team treats every comment as a minor 'error' that was caught, or worse, gets defensive about them - yes, _of course_. The solution is to tell the team that a directive suggestion in a code review simply isn't a complaint, error, oversight, or otherwise to be taken as negative feedback in any way. Code review is a process, it has multiple goals). Furthermore, if you spend the time to try it, presumably the team will coalesce, and the frequency of such comments will decrease.
In practice a pointless style fight might break out where one side of the team insists 'situations like these should use for loops, not comprehensions!' and another side vehemently argues for the opposite. At which point, yes, that is obviously an extremely bad outcome. But perhaps _that_ is the right time to throw in the towel and agree together to allow a modicum of personal preference.
Of course, if there are all sorts of maintainability issues, by all means, _do not_ spend much time on fighting such esoteric style battles at all. But not all teams are dysfunctional :)
But a code base that is stylistically inconsistent is itself a source of unmaintainability. That's probably a case of "perfection is the enemy of good enough"; a multi-year, multi-member project perhaps cannot reasonably be expected to consistently pick e.g. a list comprehension instead of a for loop under similar circumstances.
However, just stating casually that it is _irrelevant_, or that it is hopeless, feels like giving up a bit too quickly.
One would assume that the cost to morale etc is overblown (if your team treats every comment as a minor 'error' that was caught, or worse, gets defensive about them - yes, _of course_. The solution is to tell the team that a directive suggestion in a code review simply isn't a complaint, error, oversight, or otherwise to be taken as negative feedback in any way. Code review is a process, it has multiple goals). Furthermore, if you spend the time to try it, presumably the team will coalesce, and the frequency of such comments will decrease.
In practice a pointless style fight might break out where one side of the team insists 'situations like these should use for loops, not comprehensions!' and another side vehemently argues for the opposite. At which point, yes, that is obviously an extremely bad outcome. But perhaps _that_ is the right time to throw in the towel and agree together to allow a modicum of personal preference.
Of course, if there are all sorts of maintainability issues, by all means, _do not_ spend much time on fighting such esoteric style battles at all. But not all teams are dysfunctional :)
> But a code base that is stylistically inconsistent is itself a source of unmaintainability
Can you elaborate? The context here is a few lines of code that iterate over a list. I don’t know any developer who understands one that can’t understand the other.
Can you elaborate? The context here is a few lines of code that iterate over a list. I don’t know any developer who understands one that can’t understand the other.
A codebase where some developers use spaces, another use tabs. Changes to the other person's code always has whitespace changes on large blocks of text that are otherwise unmodified resulting in much more in a code review than is needed.
One developer does simple for loops without {} and another always puts them in everywhere. When the second developer touches the block of code, there's a large amount of {} being added with no other functional changes in the block.
One developer uses guard statements at the start of a method, the other always writes single exit point code.
---
Changing between these or having two different styles within the same code base makes it more difficult to maintain it since there are non-functional changes mixed in with the functional changes.
Its not a "can't understand the other" but rather "inconsistent style results in non-functional changes in the code as style changes are being made - making it harder to maintain the code and review it."
One developer does simple for loops without {} and another always puts them in everywhere. When the second developer touches the block of code, there's a large amount of {} being added with no other functional changes in the block.
One developer uses guard statements at the start of a method, the other always writes single exit point code.
---
Changing between these or having two different styles within the same code base makes it more difficult to maintain it since there are non-functional changes mixed in with the functional changes.
Its not a "can't understand the other" but rather "inconsistent style results in non-functional changes in the code as style changes are being made - making it harder to maintain the code and review it."
This mostly describes anti-social developer behavior I think, someone who reformats code for no reason? As their manager why aren’t you taking this up directly with them? Why aren’t you helping them configure their editor correctly?
Not for no reason. I've got my IDE formatter set up {this way}. You have it set up {that way}.
We're working on a code base, and I need to work in a method... and my editor formats all the lines that I change. And now we're mixing up lines.
I've also seen devs do a "reformat file" each time they work in the file... which if there are conflicting formats reformats everything. If they didn't do that commit as a separate one, this makes reviewing changes much more difficult.
This is why the .editorconfig ( https://editorconfig.org ) that I check in to is at least 60 lines long with a lot of ij_java_ lines in there - so that we all use the same one.
I'm not their manager - I'm one of the senior developers on the team that gets the bulk of the code reviews to do. I am working on getting templates set up correctly in gitlab (and .editorconfig files in all the projects) so that new projects start out with correct formatting.
There has been a lot of "create the project, check it in, it needs to be deployed {Real Soon Now}" and it was never formatted correctly to begin with (with a mix of their own editor being tabs and copying and pasting from Stack Overflow having spaces - its very easy to see where they copied and pasted if I turn on show whitespace). Editorconfig doesn't reformat code that was pasted in and if they don't reformat it themselves, then the other style remains.
Yes, I do help them set up their editor correctly... though I will point out that most developers don't seem to care about formatting (or even editor warnings). In the past (current manager is different) I've had a lot of pressure to approve if it works because of time / business pressure which ingrained a significant amount of bad habits that didn't get rectified promptly.
We're working on a code base, and I need to work in a method... and my editor formats all the lines that I change. And now we're mixing up lines.
I've also seen devs do a "reformat file" each time they work in the file... which if there are conflicting formats reformats everything. If they didn't do that commit as a separate one, this makes reviewing changes much more difficult.
This is why the .editorconfig ( https://editorconfig.org ) that I check in to is at least 60 lines long with a lot of ij_java_ lines in there - so that we all use the same one.
I'm not their manager - I'm one of the senior developers on the team that gets the bulk of the code reviews to do. I am working on getting templates set up correctly in gitlab (and .editorconfig files in all the projects) so that new projects start out with correct formatting.
There has been a lot of "create the project, check it in, it needs to be deployed {Real Soon Now}" and it was never formatted correctly to begin with (with a mix of their own editor being tabs and copying and pasting from Stack Overflow having spaces - its very easy to see where they copied and pasted if I turn on show whitespace). Editorconfig doesn't reformat code that was pasted in and if they don't reformat it themselves, then the other style remains.
Yes, I do help them set up their editor correctly... though I will point out that most developers don't seem to care about formatting (or even editor warnings). In the past (current manager is different) I've had a lot of pressure to approve if it works because of time / business pressure which ingrained a significant amount of bad habits that didn't get rectified promptly.
So, again, this describes a scenario that can be addressed with tooling and management/communication.
The question I asked was why reviewers should flag code that correctly iterates over a list but does so in a different way than other code in the codebase. Are we straying into “foolish consistency” if we flag this, or is there a tangible benefit?
The question I asked was why reviewers should flag code that correctly iterates over a list but does so in a different way than other code in the codebase. Are we straying into “foolish consistency” if we flag this, or is there a tangible benefit?
Code formatting should be able to be applied from CLI as a pre-commit hook IMO. Relying on a specific plugin/configuration and specific editor/ide is a mistake and will lead to pain in the future.
For example a VS only plugin as you take a project to add cross-platform support and onboard developers using say WSL or MacOS as opposed to JUST windows.
For example a VS only plugin as you take a project to add cross-platform support and onboard developers using say WSL or MacOS as opposed to JUST windows.
Why do you need to pick consistently list comprehension vs. for loop at all? What are you achieving with this consistency, except, well, consistency?
I’m with you. If everything has to be consistent all the time, then nothing evolves.
1 - time wide use a of a code formatter with an opinionated style setting to preempt such discussions is a huge productivity boon
2 - for "pattern" oriented changes that aren't easily checked in a linter, have an internal guide defining what you want in the codebase and then reference that. That again pushes the feedback back to the impersonal, and keeps it terse.
3 - leave the more substantial feedback for things like discussions around the business logic, or architecture of system, or implications of a particular change will have consequences outside the internal context of the subsystem
2 - for "pattern" oriented changes that aren't easily checked in a linter, have an internal guide defining what you want in the codebase and then reference that. That again pushes the feedback back to the impersonal, and keeps it terse.
3 - leave the more substantial feedback for things like discussions around the business logic, or architecture of system, or implications of a particular change will have consequences outside the internal context of the subsystem
+1 on code formatting. It definitely takes care of a class of issues in practice. While I do have my own preferences, I think it's better to simply have something that can apply on save/paste and that can be the end of it. Changes to the code formatter/version and even swapping out a formatter can be relatively easy and still completely consistent.
I will state that I will prefer a formatter than can apply from a command line as well as integrated plugin. I don't like formatters that rely on an IDE only plugin, I've seen this with VS in particular.
I will state that I will prefer a formatter than can apply from a command line as well as integrated plugin. I don't like formatters that rely on an IDE only plugin, I've seen this with VS in particular.
s/time wide/team wide/
I’m with you. The presumption to “educate,” the author is annoying and tedious. Unless the author asks for an opinion assume they know what they’re doing.
Same goes for review “comments,” that interrogate the author. “Why did you choose to use a for loop here?” is such a waste of time.
I agree, stay within stylistic guidelines and lacking any try to use the same style as the file being edited.
And just focus on the code itself: does it meet the specifications, is it clear that it does what it’s supposed to, are all the T’s crossed and I’s dotted? Good, go.
Brevity is it’s own form of clarity when used well.
Update ... and remember to leave your ego at the door! Talk about the code like an object of study.
Same goes for review “comments,” that interrogate the author. “Why did you choose to use a for loop here?” is such a waste of time.
I agree, stay within stylistic guidelines and lacking any try to use the same style as the file being edited.
And just focus on the code itself: does it meet the specifications, is it clear that it does what it’s supposed to, are all the T’s crossed and I’s dotted? Good, go.
Brevity is it’s own form of clarity when used well.
Update ... and remember to leave your ego at the door! Talk about the code like an object of study.
> a list comprehension or a map is just nitpicking
Absolutely. It’s a style choice. Same with the variable names. Ironically the whole article is a great example of what a waste of time code reviews have become.
If style needs to be consistent, it should be enforced with a style guide or ideally automated formatting tools. If it’s not in the style guide, you can bike shed it in a separate meeting, but don’t block a PR on this crap.
If you’re submitting a PR to an OSS project as an outsider, then I can understand more that authors want discretions. A pattern I like is where a maintainer cleans up stylistic things and then just says thanks and commits.
In either case, the overall priority should be to reduce unnecessary round trips.
Absolutely. It’s a style choice. Same with the variable names. Ironically the whole article is a great example of what a waste of time code reviews have become.
If style needs to be consistent, it should be enforced with a style guide or ideally automated formatting tools. If it’s not in the style guide, you can bike shed it in a separate meeting, but don’t block a PR on this crap.
If you’re submitting a PR to an OSS project as an outsider, then I can understand more that authors want discretions. A pattern I like is where a maintainer cleans up stylistic things and then just says thanks and commits.
In either case, the overall priority should be to reduce unnecessary round trips.
> Same with the variable names
No. Code is harder to read than it is to write. It's also read countless of times. Obviously single letter variables are ok for short scoped variables, or for conventional names[1] but other than that I will definitely ask for a clearer variable name. I will also ask to write full named variables (eg. user instead of usr), it's easier to read and less ambiguous in some cases. Variable names can make all the difference between unreadable code and completely fine code.
[1]: For example I write Go, and in my Services layer the pointer receiver (akin to "this" in other languages) is always named "s" for convenience. And in handlers the pointer receiver is always "h".
No. Code is harder to read than it is to write. It's also read countless of times. Obviously single letter variables are ok for short scoped variables, or for conventional names[1] but other than that I will definitely ask for a clearer variable name. I will also ask to write full named variables (eg. user instead of usr), it's easier to read and less ambiguous in some cases. Variable names can make all the difference between unreadable code and completely fine code.
[1]: For example I write Go, and in my Services layer the pointer receiver (akin to "this" in other languages) is always named "s" for convenience. And in handlers the pointer receiver is always "h".
Yeah sure but would you spend a round trip and block it? It’s not hard to discuss coding processes but this constant going back and forth asynchronously is dreadful. People get notification damaged by this.
> would you spend a round trip and block it
Yes. If I spend time reviewing some code and I don’t understand some part of it because of naming, it’s definitely an issue that must be addressed during the code review. You just need to learn not to take it personally, reviewing is literally a third party coming to review your code without your biases, so they benefit from the "fresh eye". You should listen, because in 3 months you’ll be the fresh eyes trying to understand the code you wrote 3 months ago, cursing yourself for not using more descriptive names.
Yes. If I spend time reviewing some code and I don’t understand some part of it because of naming, it’s definitely an issue that must be addressed during the code review. You just need to learn not to take it personally, reviewing is literally a third party coming to review your code without your biases, so they benefit from the "fresh eye". You should listen, because in 3 months you’ll be the fresh eyes trying to understand the code you wrote 3 months ago, cursing yourself for not using more descriptive names.
> Whether you use a for loop, a list comprehension or a map is just nitpicking, it has almost zero effect on maintainability.
It makes a big difference in languages where for loops are subject to off by one errors. Also many libraries have algorithms for what you do with the list comprehension. If you have one function ApplySomeAlgorithmToEntireList(list) that is a lot clearer than trying to find what you are doing in the body of the for/list comprehension.
It makes a big difference in languages where for loops are subject to off by one errors. Also many libraries have algorithms for what you do with the list comprehension. If you have one function ApplySomeAlgorithmToEntireList(list) that is a lot clearer than trying to find what you are doing in the body of the for/list comprehension.
That should be the approach if you already working in a kitchen-sink project where every possible way of doing the same thing already has several examples, but if the project currently has 0 for loops in the codebase, I think you can say that is a norm for that project that should be upheld. (By the way, I'd often prefer for loops myself).
Good developers will generally pick up a lot of norms from the existing code, and will even read style guides but some people always write code the same way regardless of the language or project they are working in, and I think that sort of feedback is appropriate for them. But I also think waaaay too much time was spent on the "Good Review". I'd have just said - "in this project we use comprehensions instead of for loops".
Good developers will generally pick up a lot of norms from the existing code, and will even read style guides but some people always write code the same way regardless of the language or project they are working in, and I think that sort of feedback is appropriate for them. But I also think waaaay too much time was spent on the "Good Review". I'd have just said - "in this project we use comprehensions instead of for loops".
One of the tendencies I see in reviews is that the reviewer focuses on the wrong stuff. For e.g. when reviewing the design document, they focus on the validity of the requirements and they may even stray into commenting on formatting and grammar in the document; when reviewing code, they focus on design; when reviewing test cases, again they focus on reviewing requirements; when reviewing deployment scripts, they focus on the choise of infrastructure. Basically, people tend to pay attention to things that are already past the decision stage while ignoring the matter at hand.
Another part of a good code review is to pick your battles. I used to comment on every little thing. Now I have a threshold below which I won't bother unless it would significantly improve the readability or runtime characteristics. Your coworkers' sanity is just as important as the code to ensure that the team achieves their goals.
If I feel like the author might just not be aware of some "better" (subjectively) way of doing things, then I'll leave a "FYI: could also do this in X way" type of comment.
If I feel like the author might just not be aware of some "better" (subjectively) way of doing things, then I'll leave a "FYI: could also do this in X way" type of comment.
The “suggest change” feature in github is a great way to suggest nitpick fixes without coming off like a jerk. You actually do the work, and the author can easily merge in all those changes with one click. They’ll avoid making those small mistakes / style choices over time. Also it feels more collaborative and less “do this because i said so”.
It's bugged and doesn't allow more than 6 to be batched, but yes
My threshold includes time/delay. Knowing that asking for some fix is going to delay this thing getting done by a long time - sometimes days - impacted my caring about certain things. Sometimes it was easier to just make a small fix myself than 'request' it and wait for 3 days just to get something fixed.
Personally, I would prefer someone actually make the suggested fix they're describing - doing the actual code - so I could see it fleshed out. Commenting "use a generatorInterface" to someone who's probably not used it before is less helpful than actually implementing the generatorInterface in the PR and then discussing the actual code. Not always time for that, but if it's never considered, there's never any time for that approach.
tanget: I had a PR blocked because... "use more descriptive variable name" was applied to a 'for (i=0; i<upperBound; i++) ...' loop. The complaint was about using 'i' in the for loop. This was in a test file - the first test file on the project that had been live for 7 months - and I think this nitpick was just a bit over the top. And it wasn't enforced later, just ... someone making a stink over 'a new guy' joining and trying to exert some influence.
Personally, I would prefer someone actually make the suggested fix they're describing - doing the actual code - so I could see it fleshed out. Commenting "use a generatorInterface" to someone who's probably not used it before is less helpful than actually implementing the generatorInterface in the PR and then discussing the actual code. Not always time for that, but if it's never considered, there's never any time for that approach.
tanget: I had a PR blocked because... "use more descriptive variable name" was applied to a 'for (i=0; i<upperBound; i++) ...' loop. The complaint was about using 'i' in the for loop. This was in a test file - the first test file on the project that had been live for 7 months - and I think this nitpick was just a bit over the top. And it wasn't enforced later, just ... someone making a stink over 'a new guy' joining and trying to exert some influence.
Code review has become a bad joke. Nothing in TFA should be blocking feedback. Depending on seniority and politics it will be ignored in the best case, and waste someone's time in the worst case.
The single most important thing to discuss during code review is whether the new code does what the author thinks it does. And whether what the author thinks it does is part of what the team wants to accomplish. Typically there is some sort of plan, either strewn across a ticket tracker, or in a design doc, or unfortunately stuck in someones head. Make sure the new code is in service to that plan--the real goal.
The single most important thing to discuss during code review is whether the new code does what the author thinks it does. And whether what the author thinks it does is part of what the team wants to accomplish. Typically there is some sort of plan, either strewn across a ticket tracker, or in a design doc, or unfortunately stuck in someones head. Make sure the new code is in service to that plan--the real goal.
I'm all in for good reviews, but "micromanagement" isn't something anyone should wear with pride. Speaking from experience, working with one erodes your soul. From Wikipedia:
Micromanagement is a management style characterized by behaviors such as an excessive focus on observing and controlling subordinates and an obsession with details.
Micromanagement generally has a negative connotation, suggesting a lack of freedom and trust in the workplace, and an excessive focus on details at the expense of the "big picture" and larger goals.
Micromanagement is a management style characterized by behaviors such as an excessive focus on observing and controlling subordinates and an obsession with details.
Micromanagement generally has a negative connotation, suggesting a lack of freedom and trust in the workplace, and an excessive focus on details at the expense of the "big picture" and larger goals.
the article is less micromanag'ey than the title implies, and maybe describes some constructive precision that a newer manager who had come up out of tech might have some hesitancy to apply. it seemed more like self deprecation to me.
however, agreed that actual micromanagement is an antipattern. when one understands management as "to extract value from," micromanagement is the definition of doing it poorly. micro-value-extraction is as obtuse as it sounds.
doing work through other hands instead of taking the output and delivering it to who they are managing on behalf of is almost always a waste of value. I think of it as being as weird as living in a one bedroom apartment and taking time away from work to supervise your housekeeper while they work.
however, agreed that actual micromanagement is an antipattern. when one understands management as "to extract value from," micromanagement is the definition of doing it poorly. micro-value-extraction is as obtuse as it sounds.
doing work through other hands instead of taking the output and delivering it to who they are managing on behalf of is almost always a waste of value. I think of it as being as weird as living in a one bedroom apartment and taking time away from work to supervise your housekeeper while they work.
Biggest danger of code reviews is bikeshedding. Bonus on top of that is wall-of-text bikeshedding. Avoid if you want to get anything useful done.
Examples and personal anecdotes aside, there's a nugget of truth here about how people receive feedback.
In my opinion, it's less to do with the context of OSS and corporate, as I'm sure if the same corporate folks were working in OSS, they would perceive the wall of text feedback negatively there as well.
In my opinion, it's less to do with the context of OSS and corporate, as I'm sure if the same corporate folks were working in OSS, they would perceive the wall of text feedback negatively there as well.
We have the rule that commenting on syntax is disallowed. All syntax must be enforced by tooling (prettier, linter). This speeds up code review, because you review what actually matters (patterns used, regressions, bugs) and reduces friction between team members. Also a common syntax is learned way faster, as you get the feedback right in your IDE (or you don't even have to waste brain energy on it, in case of prettier).
If a syntax is not enforceable via linter because the rule does not exist, then you either write your own rule, or have to let go of the idea and have to surrender that there is a bit of wiggle room in expression.
If a syntax is not enforceable via linter because the rule does not exist, then you either write your own rule, or have to let go of the idea and have to surrender that there is a bit of wiggle room in expression.
This is a huge deal, I've tried to implement this anywhere I've had any clout and it always saves a ton of time on reviews. Automated lint, formatting and coverage requirements (just not 100%!) cut so much wasted time out of reviews.
Of course, the flip side is organizations that want to go crazy with Sonar/Snyk/etc, where every PR ends up being dragged down by over-opinionated tools.
Of course, the flip side is organizations that want to go crazy with Sonar/Snyk/etc, where every PR ends up being dragged down by over-opinionated tools.
Oh my. functional style is good for functional languages, sure. But in python?
total = sum([f(record) for record in housing_records])
x = 0
for element in data:
x = x + f(element)
The best one would be:
total = sum(map(f, data))
Not only does it take less resources as it only has to loop once because map creates a generator. This is absolutly the use case of map, if you want to apply the same function, which is already defined, to all members of a list/iterator, map is the correct tool for the job. Now if you wanted to apply a transformation that is not in a function there would be an argument to use a comprehension as it would be better than using an inline lambda in a map. But if you already have the method you want to apply, just use map() and then pass that to sum()
total = sum(map(f, data))
Not only does it take less resources as it only has to loop once because map creates a generator. This is absolutly the use case of map, if you want to apply the same function, which is already defined, to all members of a list/iterator, map is the correct tool for the job. Now if you wanted to apply a transformation that is not in a function there would be an argument to use a comprehension as it would be better than using an inline lambda in a map. But if you already have the method you want to apply, just use map() and then pass that to sum()
A generator expression would also avoid looping twice - this is valid python:
total = sum(f(record) for record in housing_records)Python is expressly designed to be non-functional. Guido van Rossum believed that the paradigm for the language should instead be “pythonic.” [1]
[1] http://neopythonic.blogspot.com/2009/04/tail-recursion-elimi...
[1] http://neopythonic.blogspot.com/2009/04/tail-recursion-elimi...
This won't keep the list in memory like you do:
sum(i for i in range(4))( instead of [ in the list comprehension turns it into a generator, avoiding the problem you describe.
Maybe it's just too early in the morning for me or I don't know enough about Python, but why would the OPs code snippet keep the list in memory, and why is yours an improvement?
Also, is this something you could handwave because of Python's garbage collection, or is that not going to help in this case?
Also, is this something you could handwave because of Python's garbage collection, or is that not going to help in this case?
The OP's version is creating the list, then processing it.
The second version is an generator; it skips the 'creating the list' part.
The second version is an generator; it skips the 'creating the list' part.
Ohhhhh. The coffee's kicked in. Thanks for explaining. To recap, a list comprehension:
1. Creates a new, duplicate list containing every record in housing_records
2. Loops through the new list
3. Applies f() to each element and updates the new list as it loops
4. Sum() sums all the elements in the list by accessing each element and adding it to a total
Note: All returned values of f() are stored in memory at the end of step 3. This is a waste.
A generator:
1. Creates a generator object that contains nothing but acts like a list
2. When sum() accesses an element in the generator object, the generator object applies f() to the element in housing_list and returns the value
3. Sum() is therefore able to sum all the elements by "accessing" each element and adding it to a total
Note: Only one result of f() is stored in memory at any given time. Much better.
1. Creates a new, duplicate list containing every record in housing_records
2. Loops through the new list
3. Applies f() to each element and updates the new list as it loops
4. Sum() sums all the elements in the list by accessing each element and adding it to a total
Note: All returned values of f() are stored in memory at the end of step 3. This is a waste.
A generator:
1. Creates a generator object that contains nothing but acts like a list
2. When sum() accesses an element in the generator object, the generator object applies f() to the element in housing_list and returns the value
3. Sum() is therefore able to sum all the elements by "accessing" each element and adding it to a total
Note: Only one result of f() is stored in memory at any given time. Much better.
The top one using list comprehension is easily understood for anyone who did non trivial work in Python. And more concise, no run away variables, etc.
The bottom one is understood also by developers with no Python experience.
So I guess it depends on the audience.
The bottom one is understood also by developers with no Python experience.
So I guess it depends on the audience.
I feel I am the first type of audience, and have no problem understanding both versions(having programmed in multiple functional languages).
But when I see the bottom version I instantly understand what it does, its like I dont need my concious brain involved at all. I can just glance at the "shape" of the code, with my eyes just focusing at a few significant locations (the function call and the +), and I know what it does.
Maybe its because I grew up speaking imperatively. Maybe its that with the imperative version there is extra information in the "shape" of the code which my brain can use. But beeing more concise dont seem to help the second version, I dont read a constant number of characters per second.
But when I see the bottom version I instantly understand what it does, its like I dont need my concious brain involved at all. I can just glance at the "shape" of the code, with my eyes just focusing at a few significant locations (the function call and the +), and I know what it does.
Maybe its because I grew up speaking imperatively. Maybe its that with the imperative version there is extra information in the "shape" of the code which my brain can use. But beeing more concise dont seem to help the second version, I dont read a constant number of characters per second.
[deleted]
Notice the comment "In this codebase", and all the other the other caveats in the good code review. Consistency can be good.
[deleted]
Just say you are in founder mode. It's better.
This is incredibly off topic, but I have to assume all the shade being thrown at "founder mode" is because it requires an aptitude for your company's core competency, which many leaders cannot demonstrate.
It's not a nugget of management-foo that a talentless leader can add to their mode of operation, like SCRUM, 10 hacks for better 1:1s, cross-functional jamboree, etc.
It's not a nugget of management-foo that a talentless leader can add to their mode of operation, like SCRUM, 10 hacks for better 1:1s, cross-functional jamboree, etc.
Hey folks, original author here. It seems like people here are really connecting with the specifics of the code review example. The main point of the article is really "what we learn in reviewing code in community open source might not transfer well to providing feedback to human behaviors in work environments".
Please feel free to keep engaging on the code review bits (a timeless topic among programmers for sure) but I'd also encourage people to expand discussion out to how we manage humans and give them feedback in a way that both helps them grow and makes them feel supported at the same time.
Cheers, -matt
Please feel free to keep engaging on the code review bits (a timeless topic among programmers for sure) but I'd also encourage people to expand discussion out to how we manage humans and give them feedback in a way that both helps them grow and makes them feel supported at the same time.
Cheers, -matt
OK, is this a commercial for Github? Because none of the subject matter in the article has anything to do specifically with it.
It’s an article about communication styles and how they differ between open source software development and the corporate world.
And quite an interesting article at that!
And quite an interesting article at that!
Yes, the article is not bad, but it has nothing to do specifically with the Microsoft product it mentions in its title.
Oh man I've gotten a few PR reviews that, in addition to really great feedback about correctness and performance, also include "remove this single blank line" comments for code that was already autoformatted.
I'm trying to work on getting less annoyed about these, but I feel like they're really not worth anyone's time.
I'm trying to work on getting less annoyed about these, but I feel like they're really not worth anyone's time.
I'm not in agreement with the code review example. Both approaches seem fine, although some variable renaming might be helpful; particularly of `f` (what does it do?). I find the `for` example slightly easier to read than the list comprehension. But, to me, it's a stylistic choice.
The corporate communication example is better. The feedback is correct, and it improves the language. Had I written the original, my take-aways from the review would be: it's better as suggested, the reviewer is correct, and I shouldn't do it again. That is the reviewer's (and author's) purpose, it's constructive, and it "lands". If the receiver views this as an "... I hate you and want you to suffer" message without an argument as to why the original text was better, well, they might be in the wrong line of work.
The corporate communication example is better. The feedback is correct, and it improves the language. Had I written the original, my take-aways from the review would be: it's better as suggested, the reviewer is correct, and I shouldn't do it again. That is the reviewer's (and author's) purpose, it's constructive, and it "lands". If the receiver views this as an "... I hate you and want you to suffer" message without an argument as to why the original text was better, well, they might be in the wrong line of work.
If "good feedbacks" like that were the norm on the project I was working on, my interest in that project would drop very, very quickly.
I agree with the gist of it, educate and improve. But the example review is just faffing around and borderline philosophical ponderings. Get to the point.
I agree with the gist of it, educate and improve. But the example review is just faffing around and borderline philosophical ponderings. Get to the point.
I was hoping this would be about using the metrics GitHub easily provides to figure out if people are doing their job or not. If it's solely up to a manager to come through with review feedback at this minute level of detail, then it's time to hire more senior people.
I don’t get these NIT-review at all. The best review would have been to push directly the change to their branch. That way the reviewe would have seen the fix, while gaining time and saving a back-and-forth.
When writing a review takes as long as changing the code, always prefer the latter.
When writing a review takes as long as changing the code, always prefer the latter.
I agree, but that removes the authors creativity and agency - which is why I opt to leave diff comments.
I would not amount choosing between a for-loop and list-comprehension as creativity.
Nor generally any work done on a CRUD app.
Nor generally any work done on a CRUD app.
I backup all my repositries to Dropbox and external hardrive everyday at 3am via a cronjob (it's a python script)
Also it depends on the team culture. I had a manager who used to slack me and passive aggressively taunt me about the number of comments on my PR without even bothering to look at what they are. He used to assume that all of it is because of some bad code. Because of this behavior overtime the team would secretly ask each other to reach out over slack instead of adding comments in the PR, beating the whole purpose of code reviews.
Yikes. Was your manager previously a software engineer or did he come from a different background?
I was on a project as an SME that consisted of software developers that were very junior and their manager was also several rungs down the totem poll from me and my peers. The devs were treated so poorly with things like this and worse i raised an HR case. There seems to be this gauntlet that software devs have to go through before they can work with actual competent leadership (at least at my firm). I felt bad for them and coached them through the soft skills required to deal with stupid.
[deleted]
The best code review is the one which does not spend its time on stylistic nitpicks but focuses on the overall architecture of the change and the assumptions of the changed code. Getting the assumptions or the abstractions wrong has a much severe impact in future maintainability than any kind of stylistic deviation will ever have.