IMO no matter how clean the code is, there should ALWAYS be SOME COMMENTS to anything that might seem not very obvious.

Reading the whole business logic to understand the point to why the piece of code was written seems stupid.

These codes get merged because everyone is lazy to review 2k lines of code for a new feature (including me) lol

I would ask the person to break down their 2k line commit into a reasonable chunk, couple hundred lines tops. Even better in 10s.
Once had a manager that deleted the person's PR because it was too much, they had to start all over!

@dan-pud Often a feature simply takes that much code.

I cannot reduce a risk and compliance engine to less than several thousand lines.

@Root maybe not but there are ways to deliver that feature over several PR's.

@dan-pud Not functional. Not without significant added confusion, and certainly not without significant added effort on the dev’s part.

Stop being lazy and review the bloody PR. I review 20-30k lines almost every week.

@Root I respectfully disagree. It has nothing to do with laziness but more about working professionally. As you say it will take more time to review more PRs. I would bet my house that there are less bugs in production after reviewing 100-20 line PRs as opposed to 1-2000 line PR. Less bugs mean less maintenance further down and a more efficient and profitable system.

@dan-pud The issue isn’t time spent reviewing PRs, it’s time spent breaking up the complexity of a large monolithic feature into small PRs that still make sense to the reviewer and the dev.

There’s also the concern of unexpected behavior when those various smaller modules interact — it’s more difficult to see those behaviors across multiple PRs than in a single larger one.

Do you see what I’m getting at?

I far prefer one PR per feature. It can be larger and unwieldy, but it’s self-contained and all in front of you at once. It’s all on a single branch for you to pull and experiment with and try to break. All of this lowers code review and QA overhead.

I’m not saying large PRa are not a problem. They absolutely are difficult to review. However, if the PRs get too large, that points to a planning problem. Break future features down into more manageable chunks instead, and relegate some of the current feature to a followup ticket.

Now, commits? I absolutely agree with making lots of smaller commits.

@Root agree with about 90% of what you said. And there are times where it can't be avoided. But still think devs can do better in breaking things up and automated unit/integration testing can help too.
🙏

Should consider deployment toggles.

Ideally just write tests around the old feature, then incremental PRs that colleagues can understand behind a togle, then kill the old version.

No one has a hope of reading a 2k line PR. If they do it will boil down to reviewing syntax, a fairly pointless routine.

Also code doesn't need comments to explain itself. Clean code as Robert Martin outlines it, and ubiquitous language as Eric Evans outlines, together create understandable code that captures enterprise and business logic in plain English in code.

Comments are for when something obscure is happening that cannot be made clear in code written. Like a strange behaviour from a third party. Comments are about context, not describing business logic...

@dan-pud since we rebase and do one commit per features with many of my clients. This doesn't help IMHO

@KDSBest If you make small PRs you can still rebase. And your clients insist on one commit per feature? How is that better than multiple small manageable commits into arguably one or more PR?

@Root agree. Sometimes just one feature end up taking 2k lines

@Crost sometimes there's some cust logic that might not be as obvious. I mean, I should be reading the code line by line to understand.

@Crost Yes, though it depends on the dev. Some devs, especially those I work with, write absolutely atrocious code.

@Root seems most. It's really hard to improve a product going for years with complex business logic, but where everything is essentially just data structures with public props, duplicated logic copy pasted over and over into procedures and written like a giant crud app.

@Root dang 20k? I cant even read 30 lines without getting bored. But maybe it’s because spaghetti code bores me bc it’s just a pile of symbols on the screen that don’t mean much. What’s fun about reading @syb6$!!8&‘=new x + gk x===@///;,7!!!$(){|>>?^’} lol

@TeachMeCode It’s not like I enjoy it. But these idiots write insecure crap month in and month out.

In their defense, the system is so insanely complicated that I doubt anyone would be able to take much advantage of any exploits.

@Root ok but it’s amazing you can read all that without exploding. I would die having to read all that spaghetti

@dan-pud I think you might underestimate the team size. There are 50-200 comitters depending on repo. If all do small commits like you mentioned. Noone will read that history at all. It is not a black and white issue. Yes one commit per feature. Also the features might be smaller than you think. Hard to evaluate from distance.

@KDSBest why would I want to read the commit history?
If there are 50-200 people committing to one repo I think the chances of a merge conflict are much higher the longer the code is sitting locally, even worse if that's a 2k line merge. Faster it gets merged (ie faster review) the less conflicts there will be and less time wasted. Small PRs is a great way to do this.

@dan-pud we merge PRs fast enough even if it is 2k lines.

@KDSBest no offense but I dread the quality that is going in if you're quickly merging things.
There is no way you can merge a 20 line change and a 2k line change in the same amount of time and keep up the quality.
A good indication is how much time is spent fixing defects or doing support as opposed to building new features.

@dan-pud you would have 100x 20 line merges. Just because PRs are smaller doesn't mean the feature needs less code. And having 100 PRs for 1 feature is garbage.

We talk about the bred and butter systems of a major company losing millions of € if the system is down or working not correctly. PRs are behind a 99% unit tested gate. You only check readability and convention as a PR checker. Functionality is Unit tested and later tested by a huge testing team.

@KDSBest I can tell we're not going to agree, and that's cool.
👋

@dan-pud Sometimes it's impossible to build everything in smaller chunk.
Now you might say that its not compulsory to merge the complete feature in one go, but then we risk rewriting a component midway, hence doing extra reviews.

Generally, IMO it's fine for the more experienced developers (4+ exp) known for writing decent quality code to take up such a big feature.

@dan-pud I agree with you on that. Never wanted to be rude or something. Sorry if I was. Have a great week my man.

@KDSBest quite the contrary.
I like the debates and hope everyone learns something, even if they don't agree with it.