when a new guy joins a team...

Not so long ago, I was browsing merged pull requests to check quality of the work that was done. I don't know why, but in 202X C# is rather a modern language and some language syntax shall be adjusted to newer standards. When I was starting my developer journey - I was told to always add a code that looks similar to the one that already is there, unless it is total crap and adding new stuff needs more effort today, to not to die tomorrow.
But in this case, we have a project that is created in in 202X and there is a lot of good looking and working lines of code around, so anyone can follow it.

First time, when I was a student and helped my colleague, I was in shock that the teacher recognized my help, I turned all red and asked him "how you were able to recognize that?". He said that most of us have a kind of style that identifies us as developers, so this is spacing, the way how we organize braces (even when autoformated), how we write and solve problems,  and finally variable names. Then in 3 we have a look at the code and he pointed out areas when I helped my colleague. It was rather embarrassing, but truly a great lesson how to deep look on the code. We had a happy end as my colleague was able to answer all questions and explain why it is working, but the wild and angry look of all other students at us was in us for next 3 minutes...

Now the text is based on a fact that a new person joined a team, and as far as I know the developer has a lot of experience in the area. So, let's have a look at a code snipped below, where I renamed the DTO to "SomeElement" and cut some stuff that is not needed to make it simpler. This code was added, not modified.

...just read it twice and think what can be better there...

OK, so let's think:

• looks like we are dealing with a data access layer 
• we have an object mapping 
• and wtf is "SplitValue" doing there?

Let's deal with the SplitValue function first: refactor it as a helper object in a static class sounds a solution (there is no logic, just some string processing done in a shitty way). Why I am so easy to say... If the guy have the ability to be paid by number of committed lines, he will be reach after one month in every company. The code looks safe as we are checking if we have any elements, but looks like the person did not learn what is the output of a Split method click. As I was doing the bench, I was wondering if the First() linq operator will be a choice, but it was always only 25% slower than primitive array reference. So now one need to make decision if this need to be a separate helper or just use it at the end of the field to extract needed data.

So now we removed the ugly multiple responsibility from the class and we have only one method fetching and processing our list.

Now let's have a look line by line.

line 6:

naming: - sometimes I am wondering that why we humans are so mad to other humans creating those crypto names... yeah I hear that: we are just in the context of SomeElement why to write it down so many times... Now we can see a single element named a s(hit)List - nothing can stick to that. Be explicit or just cryptic (and call it "s") - but don't call a single entity a list...

declaration of a variable - imho it is not needed here, as we are re-creacting this object in line 14 on each iteration step. I am wondering if that makes any difference. Let's check it and execute another benchmark. 

Wow, the benchmark says use var close to materialized instance!

line 7

just please use var as in other cases and classes (and maybe add s before List...) 

line 8

we don't have ability to change legacy in some cases, as the DB is not under our control, but we can simplify Where statement and remove "== true" as we are dealing with bool element.

line 10

we don't need that check here, .ToList() will produce a list that we can always iterate even with 0 iterations count, so that check is just CPU tick killer and more code to read and maintain.

line 25+

we just moved it to helper class (if that need a separate class)

Summary:
Every piece of code shall adhere to other elements that are already there, so we humans can read and understand it faster (executing CPU does not care about that so much). We need to ask: is that check line needed?  Is there a way to make it simpler for a person with less experience?
When someone joins new team there is a need for a co-operation so all standards are provided and adhered. What funny I found (and this is not this case) that most seniors are not adhering to the codebase standard but rather to their's ones, as we come here with experience to do our job.

In a few places, when I was working, a code review was always a fun event, as we were using one screen to go over the code to find if there is something to fix, or we were measuring WTF/min ratio as the common delimiter of code quality. Now after the COVID era, I found that PR is the only interaction of a senior person with other members when the work is created (and that's very interesting).