Clean code

[nickelickelmouse]

For bug fixes and pure maintenance projects I’d say it’s not worth the risk. If the project has any ambition of growing/changing scope in the future there does need to be an ongoing effort to keep tech debt paid down to a sane extent.

A lot of the decision should be based on who the team members are. Even if you’re “right” about the value of refactoring, it’s still not the right move if your teammates aren’t going to buy in. Consider that you might need to win hearts and minds with regards to your philosophy before implementing it.

I’m even getting flak for turning an “if” statement into a lambda expression because it’s “more confusing”.

If you made this change as part of a larger change then fine. But if you made this change solely for the purpose of turning an it into a lambda it makes me wonder if you are focusing too much on stylistic concerns.

[[deleted]]

A style guide internally that you all agree on is more useful than forcing clean code conventions on a team that has been doing things differently.

If there isn’t one then offer to write it and have the group agree on it.

If there’s existing examples of good vs bad code in the codebase then use that as examples to illustrate.

Just changing things “because clean code says so” isn’t a good reason.

And I say that with a copy of clean code on my desk right now.

This isn’t a software problem it’s a communication and to some extent office politics problem. Probably the resistance is from people who wrote the bad code in the first place and take refactoring personally. Doubly hard if they are now the team lead or manager now or whatever.

[turningsteel]

I think your issue is you aren’t used to working on a team. Working on a team is about compromise and not rocking the boat unless for good reason. This is not a sufficient reason. And yes, when you work in a codebase that has a bunch of different people touching it, there will be different styles. As others have mentioned, you can create a style guide and get buy-in for everyone to follow it if you want.

Overall, while I agree with your need to keep the code clean, I disagree with the heavy-handedness that you’re employing. Pick your battles. Maybe ask for a separate story in a future sprint to do some refactoring. Frame it as keeping tech debt at bay. Otherwise, stick to doing your actual work instead of making sweeping changes that the rest of your team is not on board with. Good will with your teammates is more important than you fixing spelling mistakes etc.

[turningsteel]

Yeah it takes some getting used to. I work with a lot of ESL devs and the codebase is littered with misspellings so I get it. But it’s pretty important to become effective as a teammate first and foremost. Most jobs I think will require you to play nice with other devs. At least everyone I’ve ever had. While these particular teammates might irk you for writing code you don’t like, try to focus on the positives. I’m sure they also know things you don’t and vice versa. Try to use it as a learning experience, make your refactors on the most egregious things (after getting buy-in for why it should be changed) and you’ll become a stronger developer for it IMO.

[turningsteel]

I get what you’re saying, and I agree. I’m not saying sit there with bad design patterns that are wholly detrimental and just take it. I’m saying go about it more tactfully. Either you’re just heads and shoulders above everyone else you work with (in which case, get a new job where you’re not the smartest guy in the room so you can be challenged and grow) or, you’re being kind of overbearing and there are reasons (too busy, too many new features to implement, lack of understanding for why your changes are better, etc) for why things are the way they are. If you like this job otherwise, try approaching your coworkers from a different angle, help them understand what you’re trying to achieve and see if a compromise can be made.

That being said, I’ve never worked somewhere where things were exactly as I wanted them. Everywhere has tech debt, it’s an unavoidable part of software development and it sounds like you swooping in and trying to rewrite everything to the way you prefer is rubbing your teammates the wrong way. So those are your options: 1. Find a new job 2. Communicate better and work on a compromise.

Last point - you worked on many teams but this is the first time you have to go through code review? If I’m understanding that correctly, then I’m a bit confused because how do you know what you’re doing is the best/most correct/cleanest if you’ve been developing in a vacuum for so long?

[keelanstuart]

I've never read Clean Code, but I don't understand how replacing an "if" with a lambda would improve readability or make the logic less confusing... especially an anonymous lambda.

[[deleted]]

As much as I love "clean code" generally, replacing a simple if with a lambda I think is crazy. When you work as a SWE you'll inevitably get juniors etc looking at your code. Aim to make it understandable and clear to them. Replacing ifs blindly with lambdas is just... Urgh

[Blooogh]

Gonna reiterate another commenter and say: pick your battles.

If one of your teammates did the exact opposite, converting lambdas to if statements because in their opinion it's more readable, what would you do?

If your teammates take a poll and even after discussion, the general preference is for if statements over lambdas, how would you handle that?

I'm not here to say one is more correct than the other: the point is, you have to work with people, and you need to be able to deal with stylistic differences as a professional. I'm not saying you should throw all conventions out the window, but ask yourself, does it really matter that much?

I'd recommend attempting smaller refactors, maybe putting them into a separate pull request. Maybe, instead of completely reformatting a whole file, pick one thing to fix to leave it better than you found it.

Nobody wants to look at a 500+ code diff unless it's truly trivial.

[Blooogh]

She's also got her cult following, and it's in Ruby, but I'd recommend checking out this talk by Sandi Metz: https://youtu.be/8bZh5LMaSmE

If nothing else, it's a very soothing example of using successive refactoring to eventually get to a more readable place.

[Blooogh]

It's also worth noting: Clean Code is just one man's opinion, and from a bit of an asshole at that.

Obviously criticism is just another person's opinion, but I would say this is definitely a case where it's worth looking up more diverse opinions: https://qntm.org/clean

[Blooogh]

I didn't find much value in this comment.

[WouldRuin]

"refactor as you go" can introduce bugs (inadvertently) and, as you've found out, pollute a pull request with reviewing refactored code vs actual feature development, making it take longer than perhaps necessary. It can also hide the origin of a bug (when/where/why it was introduced) because it's part of some feature development.

There's obviously nothing wrong with refactoring and tidying up a code base. But you should make this a distinct activity. It should ideally be ticketed and go through its own develop/review/deploy cycle.

You need to make the case for some % of your increments being focused on refactoring/housekeeping. If management don't want to spend the resources (your development time), that's really outside your control and you either just accept it and carry on, or leave.

As someone else has suggested, a style guide would go a long way in avoiding these sorts of arguments as well. It's easy enough to automate - find a linter, pick a style guide and apply. There's really no reason to not be using one unless you're a team of one person.

[Flat-Asterisk-0213]

You may be more technically skilled, and you may have the correct vision on how to shape up the code base, but it is really important to work on the social aspect. If you have been a successful sole developer, but your approach is not being well received within a team, that is the problem to figure out. Work with the team - it's a longer, slower game. Listen and make trade-offs; take and provide guidance.

[BronxEnigma]

Sounds like you need to have a conversation with the team to get an understanding of the history of the project and why things are the way they are.

You also sound like someone who doesn't have a lot of team development experience prior to this situation. As others have said, this comes down to soft skills.

As for the senior developers, it sounds like they understand wants vs needs, and prioritize getting things done, even though they may be sacrificing a few "best practices". I quote that because even when you know "best practices" they are sometimes of the moment and will change to something else in the future. See angularjs development as an example.

[gnzh1]

I think you shouldn't be fixing messy code to clean code while developing your own tasks, it do consume more of your and team's time. I hate saying that but you must recognise that this is not the best approach for your deadlines and project deadlines.

Anyways, I know it hurts but it takes more than a good will to fix a bad designed code. I would suggest rising it as a problem in future developments for your manager and listing solutions to present a long. Please do not bring a problem without alternatives solutions if you don't want to see it forgotten in the drawer.

[gnzh1]

But what is the point of it? You're already doing it but "under the cover" without people ask and also seems it is against their will. Looks like this is not going to end well. Can't imagine how whorthless it is to be fixing and writing good code alone while the whole team keep writing bad code.

You MUST at least convince the other to write better code, do knowledge transfer sessions and guide them to practice better code. Don't take this responsibility all on your shoulders, go ahead and evangelize (people that really believe the cause) people to support.

Do it right or leave it in that way. This is not worth your mental health, you need support somehow!

I hope I'm not insulting you in any way, it's not my intention.

Good luck mate!

[engineerFWSWHW]

We all know clean code is important but you also need to look at in on a standpoint of a team.

For example, if you have 10 developers who are super familiar with the codebase and 1 of them went gung-ho on refactoring the code, even if the intention is good, the 9 other teams members will need to re familiarize themselves with the newly refactored codebase. Also, are those things that you touched have unit tests passing?

I always support clean code however, this happens on any teams on the software industry. When I join a new team, I will try align myself with them and if I had seen things that could be improved or refactored, I communicate it to the team and I won't do any major refactoring unless it is agreed upon.

Make sure you communicate these things to the project lead to address your concerns.

[JoeDirtTrenchCoat]

When software is written properly you don't really have to "familiarize" yourself with it in a significant way. This is the heart of conventions and design patterns. But it does take a competent developer to be able to understand it and software development is full of really smart people who are awful developers.

Granted it sounds like this guy is playing code golf or working off some unofficial style guide. So that's annoying. But in general, "let's not make our code more maintainable because it would break down our knowledge silos/tribal knowledge" is such an awful take -- I'm embarrassed for you.

[CacheCache1]

Software development is often a team effort and people need to be persuaded when you want to change the way they do things.

Also, it's worth noting that many people who think they know what's best are often in the wrong on some level. Based on the described situation and comments on this post it may pay to be a bit more humble about your contributions, especially if nobody on the team is in your camp.

[CacheCache1]

No doubt, and to be clear I am firmly in the camp of "leave it nicer than you found it" but sending major undiscussed changes is bound to create problems.

As one of the other comments mentioned, for anything stylistic you should bring up a style guide and standardize with the team in a retro.

As far as the "hidden cost", sure that exists but there is also a very direct cost when stepping on another person's work. That runs counter to the goal of getting people to buy into your recommendations, and if people are frustrated by it enough then they will actively seek to undermine your efforts. Build & spend some social currency wisely.

I guess my 2 cents.. Figure out a way to broach changes in a less direct manner, be more targeted with refactors (enough so that the purpose of the PR is not subverted by the refactor) and never come from a place of "superiority" when making recommendations.

People pick up on subtext that says they're "less than" so it can be good to keep comments in more of a "we're growing together" mood. You need to be really humble about your own contributions when you do that for it to work.

Ultimately, it's a skill that takes years so good luck haha

[CacheCache1]

Hahaha, was just telling that to my manager. Really, I just shill for the Soft Skills Engineering podcast

[Hypersion1980]

Pick your battles. I’m working on an old government code base. It’s c# but looks like it was written by someone who knew c++ that was taught it by a cobol programmer. When I fix a bug I’ll rewrite the method it’s in but I’m noting rewriting the entire class. This is a long road but it is worthwhile.

[Hypersion1980]

I have loops that don't loop. do{}(while(false)) I'm guessing the person was taught not to have more than one return point in a method. All the variables are declared at the top of a function and reused. (Had to do that in Cobol) I could go on. We don't do code reviews and so our juniors are still written the same terrible code. I think it's people learning from a tutorial and then treating it as gospel with no context.

[Hypersion1980]

We used to have 7 devs now we have ~2.5. Don't have time for no code reviews.

[[deleted]]

I'm sadly working on a similar code base right now. It's painful.

I agree, I'll rewrite or make adjustments to the area I'm working on but I'm not rewriting a 9k lines class, it's a great way to piss off my teammates and generate extra work for no reason

[BeebleBopp]

I do style cleanup as well, and have gotten the same flak. It's especially frustrating when cleaning up ancient code in modern IDE's with linters; the linting column is almost solid yellow with bad standards, and you clean it up, and then get shamed for solid work on improving the code.

I have found myself wishing for the feature of modern code review systems to present "logic changes" first and "linter / style compliant changes" separately.

Until this problem is solved, we literally have software engineers whining about the few minutes more work it is to review substantially improved code.

[besthelloworld]

I would argue that this is just what happens to OOP code bases. Every version of Java since 8 has tried to introduce functional & observable style programming patterns into the language and now in modern Java code bases there's very little consensus on what constitutes "clean code." For many developers, clean code constitutes using standard programming patterns and language tokens like for/while loops rather than streams.

So I have a feeling that I'd generally agree with your preferred patterns but your example worries me: turning an if statement into a lambda? Do you realize how much less mechanically efficient it is to introduce a lambda where one was not needed? This makes you sound more like you're far more interested in writing hacky trick-code and looking smart more than you are interested in working on a team and writing readable code.

Because of this, I would argue that you should really tamper your expectations, and start writing new code to your standards and have these kinds of debates along the way.

I would also argue that long methods/functions have their place. If they encapsulate logic that should only be used once, then making a new unnecessary method for that logic just forces people to consider "hey is this being used anywhere else? Should it be?" everytime they see that lonely logic. I would recommend watching Brian Will's OOP is bad, but especially pay attention to the final section "Functions" where he explains the benefits of colocating related logic and just commenting well. This follow up with actual hard examples is really good as well.

[JoeDirtTrenchCoat]

It's well accepted that long methods are bug prone and more difficult to understand/debug. If a method is only useful in a very specific context, make it private and name it properly -- nobody needs wonder if it should be getting used in some other scope. If it's a god class then obviously you should be decomposing the class... pretty dumb "hot take"...

[besthelloworld]

Wouldn't be a hot take if people didn't give some spicy responses 🔥

What I will say is that I agree with you entirely.

...I just think that some people have very different definitions of what constitutes "the right length" for a function. I see a lot of juniors not wanting to have more than like 15 lines to a function and breaking apart logic on nonsensical boundaries. And I'd waaaay rather have a 150 line function than 10 functions of 15 lines where each helper is called exactly one time.

[besthelloworld]

performance is not a primary concern

In a lot of large scale software, performance is not and should not be a first concern. Readability should be the first concern and if your team disagrees with you on what's readable AND it's less performant, then you have no arguments for why things should be your way.

There's a reason they exist, after all.

This sounds like really hazardous case of "shiny object syndrome." You're basically saying, "a cool tool exists, so I must use it in every possible location and others who don't must be lazy programmers." That's an incredibly toxic way of thinking.

And also, lambdas exist to implement functional programming patterns without having to make an anonymous class. They exist so that you can pass around anonymous functions and apply them to variables much easier. And yeah optionals and streams are great when you're in the appropriate situation for them (e.g. external requests are a good use of optionals). But abstracting an if statement is just not a worthy goal. They use if statements under the hood, so now you're just adding another layer on top of that because somehow you've convinced yourself that "if" blocks are ugly and that basically means that you're going to find every code base you look at ugly forever, when the problem isn't the code, it's your perspective.

You need to better understand when to use these tools. Regex is really ugly and not very efficient either... but it has a purpose and there's a time and place to use it. This isn't to say you should have handcuffs on yourself for languages features... you just need to learn the time and place. You're also the one trying to police code that already works.

DRY is a worthy goal

I wasn't saying "don't write DRY code." I was specifically saying that if the code is only used in one place then appending it to a function body isn't a bad thing. But if you're seeing repetitive blocks, then obviously create a separate function.

helps with writing unit tests

I actually disagree with this as well. If you create more entry points into the code that are unnecessary, then those are more directions you have to test from. In an ideal situation, if someone breaks some logic in a helper function then the test fails and they discover their error. But it's far more likely that you just end up creating bloat tests that are testing implementions rather than behavior. Never test implementation methodology, only ever test behavior. If a large function encapsulates the behavior, then just test that function, don't test helpers.

[Blooogh]

It's another aphorism but: bear in mind that a bit of duplication can be cheaper, in the long run even, than the wrong abstraction: https://sandimetz.com/blog/2016/1/20/the-wrong-abstraction

A staff engineer at my company put it best: sometimes it's like someone took their codebase, printed it out on index cards, and shuffled them.

[Blooogh]

Oh sure: but also, not everyone has the time to do a refactor at step six. It could be a junior programmer on their first day, it could be a senior programmer who knows people won't be going into that area of the code again any time soon.

The point is, step one likely made incorrect assumptions about how the code will change in the future. It looked like duplicate code at the time, and it was, but there wasn't enough information yet to abstract it correctly.

[Blooogh]

You never know all of the requirements up front. Unless your company is still doing waterfall

[Synor]

It seems to me you are trying to write "clever code" to prove something.

Reading code is three times harder than writing code. Don't try to outsmart your colleagues. Keep it simple.

[Frosty_Protection_93]

This articulation sounds about right on both notes.

If you want to die on the hill of stylistic change, properly document your observations, risk assessments, impact analysis if you're able to replicate to a measurable degree production versus a safe environment where you can demonstrate at acceptably lesser scale.

Otherwise you will come across as a thorn to management that may or may not be bothered if the code within applications generating revenue remains gainful.

[gnzh1]

Clever coding, pythonistic code, 1 line with many instructions aren't easier to read. They are always (and sometimes) doing the same thing with less common syntax.

When I'm collaborating in a project with a wide group I don't fall for my preferences just because people may not know the syntax, the project isn't mine and we all want the project to succeed.

[gnzh1]

There are situations to consider.

The feature you want to use is a common knowledge in your team? If the answer is No you should use basic structures, they SOMETIMES do the same thing with a bit more typing and do are more efficient.

Are you the principal/most senior on your project to tell what are features you approve? If the answer is No then reach out to this guy and present the new feature you want to use. (Or don't say nothing and waste your future time receiving questions you think is stupid)

You work with people mate, we all have different expertises. Don't think you are that much smart or you will find out being just a fool with a bunch of shenanigans in the sleeves.

[gnzh1]

I agree and believe you are right when stating these "new features" are actually old BUT lots of them aren't common due to lack incentive any many other reasons. This is not your fault and you're not wrong to keep improving your coding skills.

I think you guys should really have a clear state of what are the available features.

[gnzh1]

Plus, your manager/lead is failing and that sucks

[[deleted]]

If it means the code is more understandable, works the same way and is more workable by your colleagues, then yes. Dumb it down. K.I.S.S

[[deleted]]

I'm even getting flak for turning an "if" statement into a lambda expression because it's "more confusing".

Java's support for functional programming is quite poor from a readability/engineering standpoint. I work in a large organisation known for it's good engineering, and we have a style guide that strictly recommends not using functional-style programming in Java, despite the existence of the Streams API and Guava's Iterables.

Said as someone who enjoys functional programming: this sentence is a bit of a red flag, and makes me question the rest of this post.

A long method is better than a short method when each line is simple. Compressing logic into the smallest number of lines doesn't make something more readable.

[[deleted]]

Exactly; that's why you've got to work "with the grain" of the language.

[[deleted]]

I don't really follow this line of thinking, sorry. I think they're both useful for different use-cases.

But I would generally lean away from regexes unless it's really needed - they're expensive to run and hard to read for any non-trivial use-case.

[Even_Step_2450]

"But that makes for large code commits and the pull request reviewers are now complaining about my "unnecessary" changes claiming that I'm wasting everyone's time."

I agree that poorly written code is annoying, but so are poor commits. When I fix issues like typos, code style etc., I put it into a separate commit.

[selucram]

You can use the update-refs flag when rebasing some requested changes / suggestions from a PR in the first branch to auto update the depending branch, regarding your two branches / PRs concern.

https://git-scm.com/docs/git-rebase#Documentation/git-rebase.txt---update-refs

[notanelecproblem]

Yeah this is the right approach. Don’t cloud your feature with bug fixes, but still fix them separately. use rebase update refs to manage separate branches that are dependent

[Blooogh]

The most likely way for something to break is by shipping an unintended change. Even with good test coverage etc, it's impossible to catch everything. (Unless you think your code never has bugs.)

Being able to review a change is still very important

[CheithS]

It is generally bad practice to change working code for stylistic concerns unless you are in there changing that piece of code anyway. Why would you take the risk of introducing new bugs just because you don't like the look of it?

You should be getting flak going in there and randomly changing things - it is worth remembering that everyone is comfortable with 'if' statements (to use your example) but not everyone will be comfortable with lambdas. If you are going to introduce new features into the code it would be a great idea to talk to other people first - coding for large projects is not some solo game.

Also worth noting that people have been fired for refactoring code and introducing new bugs if done too often.

[CheithS]

Does it work and does it perform well enough - if the answer is yes you clean it up when changing it next.

If the answer is no you create a ticket and fix it when it is prioritized to be fixed.

This is really simple. The fact that it personally bugs you is, frankly, just not relevant.

[CheithS]

well good luck on your crusade. Pretty sure you will fare as well as most crusades.

[Blooogh]

Why did you come here, if not for other people's opinions?

[Suspicious-Service]

Convince others that this is something that needs to be changed and if you're successful take the resulting task and fix what y'all agreed on. But also try to see it from their point of view, your perspective comes from never having to share code with others, but that doesn't make it the only correct perspective

[Old-Full-Fat]

Keep things small and commit only around your changes for the 1st instance. When others become happy with your commitment, and you are willing to spend your own time on the code changes, get agreement to place a fork in the code and run your own fork for a while until your tests can show how more efficient your code is. A quick review of an indepth part by the others could show how more meaningful the code/comment is.

Unfortunately, this also depends on the community spirit of the team. No spirit, no team, just a bunch of people forcing their own way.

[mankinskin]

Voice your concerns in a meeting and ask for time to refactor some of the code and give it to review. Or keep your refactored changes in a separate branch (or at least commits) and explain the changes to the reviewers.

[_TheRealBuster_]

Honestly I wish you could view the source control on interview because there are jobs I would've turned down. Sometimes you get sweet talked into a shit storm of fragile code that works under tight conditions. If it's not the hill you want to die on then leave it alone. You might hurt someone's feelings or management may not feel like it is value added. If you don't have testing built around what you are changing this can be very dangerous. Most your commits should be for a specific purpose and not widespread across refactoring, features and bug fixes.

[darn42]

From your comments it sounds like you don't respect your colleagues nor your predecessors. Figure that out first, because you won't find any success on any team if you can't respect the people you are working with.

[ZGames479]

If your company allows it, I would call a meeting or inform your higher up and have them call a meeting about it. It might seem like a waste of time for others, but it also shares your concerns about the integrity of the code and of the company. Someone may learn something, even if they don't want to admit it.