Code difference

Code review doesn't do what I hear people tell me it does in every organization I have worked for

For example

• Makes code readable
• Shares knowledge
• Standardizes approach

What it does do in my opinion (read earlier thoughts I had on this here code review)

• Encourages bullying of approach (especially present when lack of tests and linters are used).  Reviewers: This is how things should be done and if you don't then it's bad.  Truth: It's never bad, it's more like unfamiliar is best you will get reviewers to admit to
• Blocks change.  Revamping a codebase will never be achieved if the reviewers are focused on one particular result.  Again lack of automated tests force reviewers to be disincentived to approve the incremental approach to fixing large previous irrelevant code decisions (People call this technical debt.  I do not agree with that term as it is better stated as Technical Risk, calling it the former term allows inapplicable analogies and approaches to be applied to decision making on it's long term viability)
• Forces application of patterns that are irrelevant to the approach and overly formalistic.  I have stopped counting the times someone has used a large company to fill in for authority (Amazon, Google, Microsoft, Facebook) on why they don't like something.  Understanding the application of the code is more important guaranteed over anything including the algorithms all the time.  Don't trust me?  Well core financial and government software you pay bills on is still running on ancient COBOL on AS/400 mainframes that is just a series of Peform Until <Condition> etc.  To further this point there are multiple ways to solve issues and because you have not seen it or heard of it does not make it invalid.  How else are languages and techniques to advance if experimentation and novel approaches are not used regularly?

What does work

• Automated tests (review the tests for completeness and adjust to cause failures)
• Reviewing the code in operation.  In this case I do not mean reading it.  I mean run the code in direct operation and begin to probe for issues as I said above knowing how the code will be applied i.e. the end usage is more powerful than tweaking algorithms as you can do wholesale swapping of entire techniques if you clearly understand the input -> output workflow
• Ask questions.  Even if you think you know the answer the best way to bridge the mind to mind gap of assumptions and particular frame of reference is to establish knowledge sharing is by asking questions about what this does etc.  This is also insightful as not everyone will use the same techniques or reference points case in point Richard Feynman asking mathematician John Tukey about talking and reading

For the lazy, when Feynman told mathematician John Tukey about this, Tukey could do the reverse — talk but not read. The reason was that Feynman would talk to himself in his head, while Tukey would see an image of a clock ticking over. Feynman suggests this could be because people think differently, and if you’re having trouble getting a point across, it might be because what your saying is more difficult to translate into the other person’s favoured modality than it is your own.

• Use a linter.  No more holy wars about spaces, commas, capitalization.  All comments as such are invalid.  If it is in the linter then it works, if it is not then it is allowed even if you don't like it.  This is an excellent point of golang and python (I like python a lot) forcing certain formatting to even run programs
• Use Open Source principles.  I make changes to the code then I know that code until someone else makes changes.  At that point they can scrap my approach and change the code to whatever they want (Oh you can't do that because you have no tests?  Sounds like a situation where the first thing on this code hand off a developer should do is ask the previous one for tests or write their own.  This would be invaluable going forward and comes back to my change needs to happen in small increments all over the codebase to keep it from rotting/rusting)

Concrete example

I wrote code snippet 1 first knowing that the allowed applications is less than 10 and then did direct returns as soon as possible.  

private static ApplicationDetail GetMatchingApplication(string applicationName, List<ApplicationDetail> availableApplications)
{
	var matchedApplication = FirstConfiguredApplication(availableApplications);
	if (!string.IsNullOrEmpty(applicationName))
	{
		return matchedApplication;
	}
	var nameMatchedApplication = availableApplications.FirstOrDefault(z => string.Equals(z.Name, applicationName, StringComparison.OrdinalIgnoreCase));
	if (nameMatchedApplication != null)
	{
		return nameMatchedApplication;
	}
	return matchedApplication;
}

This was objected to and as such I was required to change it to this because checking for null first is more standard before then go searching for a matched application remember that there only less than 10 options.  This kind of stuff was argued around for way too long and for no meaningful change in my opinion.

private static ApplicationDetail GetMatchingApplication(string applicationName, List<ApplicationDetail> availableApplications)
{
    // Try to get ApplicationDetail matched to the applicationName, if available
    if (!string.IsNullOrEmpty(applicationName)
    {
        var nameMatchedApplication = availableApplications.FirstOrDefault(z => string.Equals(z.Name, applicationName, StringComparison.OrdinalIgnoreCase));
    
        if (nameMatchedApplication != null)
        {
            return nameMatchedApplication;
        }
    }
    
    // If there if no applicationName, or if there is no ApplicationDetail for the applicationName, then we'll fall back to the first available ApplicationDetail
    return FirstConfiguredApplication(availableApplications);    
}