explicit check for true?

[rmccu]

NebulaLogger/nebula-logger/core/main/log-management/classes/LogEntryEventHandler.cls

Line 362 in 3b04138

if (String.isNotBlank(tagName) == true) {

I noticed in several places rather than allowing the method to be the condition itself for a condition, there is an explicit comparison to true. Is this necessary? Shouldn't you just take the outcome of isNotBlank() or containsKey() as-is for the condition? What is the benefit of including the == true?

[jongpie]

This is purely a stylistic choice that I've made for this repo, because I think explicitly specifying true or false helps with readability.

When I originally first started working on this repo years ago, I didn't follow that convention, I originally used the style you're referring to of if (someMap.containsKey(key)). But over the last several years, some of the teams I've worked on have found that during code reviews, some developers would misread conditions like if (someMap.containsKey(key)) and if (!someMap.containsKey(key)) because the ! can be easily overlooked. By switching to using if (someMap.containsKey(key) == true) and if (someMap.containsKey(key) == false), we found we had fewer issues during code review, so I've adopted the same convention for Nebula Logger.

[rmccu]

That's fair.

And I apologize for the several posts. I probably should have just created 1 discussion thread with numbered questions or suggestions.

[jongpie]

@rmccu circling back to this ~1 year later - the explicit checks for true have been removed! A few other people have brought up similar feedback over the last year, and I don't use the convention myself anymore in other codebases, so I've removed the == true bits in release v4.11.8.