Nice write up, I agree to most of your comments.

Nowadays I personally try to avoid $Select() and $Case() because I realized that it's often difficult to read, particularly for new comers.

I fully agree on using full command keywords, in my case including for most common ones although I admit that sometime I almost automatically type a single letter Quit with postcondition to exit a loop (Q:key="")! 😁

As for "Use all lowercase for command keywords"..well, this is really a matter of taste, I like commands with capital first letter, I find it increase readability, but, again, it a subjective opinion.
One thing for sure, whatever you choose, stick with it! Be consistent.
In fact when I happen to modify existing code, I try to adapt new code to the existing style (if any 😱 ).

About using full function names for intrinsic functions....well, I agree but meanwhile I have to admit I'm not THAT consistent, I sometime use abbreviated/single letter for some functions like $d, $zdth, $i. Maybe I need to improve on this. 😊

I have mixed feelings about the (relatively) new Return command.
While I understand the usefulness in some circumstances (i.e. error processing) I really (really, really) don't like its usage "in the middle of code".
To me good code should have one, and only one, "exit point" at the end, I consider trowing Return commands here and there in the middle of code (like a method) the same of using a Goto.
That's why I still don't use it that much even now, last line is the only exit point and I use Quit, changing it to Return makes very little difference.

Hi 

I found the rules here.

I understand that at the moment only some of the rules are applied to Open Exchange projects, but as you said, some customer may use the other rules to measure code quality or maybe simply to learn how to write good quality code.

Having published "wrong rules", whatever is the use, I don't think is a good thing. Fix the rule or remove it, before someone think they are good practice or useful info.

Some more notes about ObjectScriptQuality rules.

Rule: SQL Delimited Identifiers with double quotes

Both the "Non-compliant Code Example" and "Compliant Solution" for the Class Query sample contain an invalid SQL statement.
For example the Compliant Solution for the Class Query sample is:

Query Example() As %SQLQuery(CONTAINID = 1, ROWSPEC = "ID,Corporation:%Integer,IDNumber:%Integer,Name:%String, DisplayName:%String")
{
  SELECT ID, Corporation->CorporationID, IDNumber, Name, (IDNumber || ' - ' || Name) As
    FROM General.Contacts
  ORDER BY Name
}


At the end of the first line of the select statement the column alias is missing after "As ...".

In othe samples, I think it's misleading the naming of "SomeCondition" where in fact it represents a value:
 

&SQL(SELECT Val1, Val2
                INTO :val1, :val2
                FROM Table
                WHERE Val1='SomeCondition')


Here 'SomeCondition' is not a condition, it's a value, I'd rather use/name it 'SomeValue'.
I refrain from commenting that in the sample SQL above, the query returns a constant as first column/value (Val1).....

Rule: Use of &sql

The rule states:


The problem with using &sql(...) is that the execution plan will be calculated the first time the query is executed, and never be re-evaluated again.2

This was true, however since 2020.1 IRIS uses "Universal Query Cache" for both embedded SQL and Dynamic Queries.
If &sql() should be avoided (and this can be discussed...), a better, strong, valid, true reason should be provided.

Rule: Use of OPEN is discouraged

The OPEN command has a very complicated syntax which is difficult to get right.

It is preferable to use classes from the %IO package instead which are much easier to use and provide the same functionality.

The proposed solution breaks rule Undocumented class/method

The %IO package is never mentioned in the documentation and is (by default) hidden in class reference.
Depending on the device, different documented classes can be used instead of %IO package.

Maybe we can discuss/ask InterSystems what are the plans for %IO package.
Is it here to stay? Is it going to be deprecated? Is it going to be documented?

More importantly, can we safely use it? Sometime (often?) undocumented means that should not be used by "user code" and/or it can be changed (by ISC) without notice in any future version.

I had a look to the various rules, including non BUGS, and in some case I don't fully agree to the rule, but I understand sometimes it's a matter of opinion.

There are however cases where the rule are simply wrong, other cases where the supplied sample is wrong.

I haven't checked all the rules (yet? 😊), the errors I found so far are:
 

Rule Property name declared with quotes
It's funny that the "Compliant Solution" is simply an invalid/wrong property definition! 😱

Property Hello_World;

While I fully agree to avoid quoted property declarations and the use of "non standard" property names (like "Hello_World") I can imagine some scenario where this is definitely required and cannot be avoided.
 

Rule Incompatible argument type in a method
According to the description, running the provided sample code, in two cases, call to DoWork2() and DoWork3() methods, raise a "PARAMETER error" message on runtime.
In fact, the supplied code never raise any error.
 

Has anyone ever checked/reviewed the rules and found dubious rules or don't fully agree with some rule?

Another option to view/monitor opened transactions directly from System Management Portal is to use the System Dashboard (System Operation -> System Dashboard). No need to write code or SQL, just a few clicks.
In System Dashboard there is a line "Transactions":

In case one or more transactions is/are opened for more then 20 minutes (or so...) the System Dashboard shows a Troubled state.

In System Dashboard, if you click the "Transactions" label (regardless of Troubled state) then at the bottom of the page a link "Click here for more details" is displayed:

If/when "Click here for more details" is clicked a page with top 5 transactions (longer time) are displayed:
 

From there you can click the Process ID and go directly to the Process Details page.

Create a Business Service that use Ens.InboundAdapter and in the OnProcessInput() method call your routine, something like:

Class Community.bs.ServiceTest Extends Ens.BusinessService
{

Parameter ADAPTER = "Ens.InboundAdapter";

Method OnProcessInput(pInput As %RegisteredObject, Output pOutput As %RegisteredObject) As %Status
{
	Set result=$$getTicket^production.etl.getTicket()
	Quit $$$OK
}

}


When you add the Business Service to the production set the setting CallInterval to a VERY LARGE interval (like 99999999), this ensure that the BS is called once.
In the BS settings add a schedule that starts the BS every day at 12 and stops at 13:00 (any time you are sure your call has finished), like: START:*-*-*T12:00:00,STOP:*-*-*T13:00:00

Note that no trace will be visible because the BS does not create any message, you may add some entry in the event log to track execution, something like $$$LOGINFO("Running getTicket") and possibly other info/error handling etc. (depends on your code).

The sample you are posting does not work, %JSON.Adaptor is an abstract class and cannot be instantiated, so the line:
set test = ##class(Test.Json).%New()
returns <METHOD DOES NOT EXIST> error.

In order to instantiate it you need to inherit from a non abstract class like %RegisteredObject, here is my test:

Class Community.TestJSON Extends (%RegisteredObject, %JSON.Adaptor)
{

Property bool As %Boolean;
}

Then this is what I get:

set test=##class(Community.TestJSON).%New()
do test.%JSONExport()
{}

Another test:

set test=##class(Community.TestJSON).%New()
set test.bool=1
do test.%JSONExport()
{"bool":true}

I'm using IRIS 2023.3, same as yours.

Maybe you have a different situation than the sample you posted?