For and If with one line: to brace or not to brace

Hi, Community!

Coding guidelines discussion. Consider you have For with one command in the cycle. Here are the options:

One

For i=1:1:1000 set ^Test(i)=""

Two

For i=1:1:1000 {

 set ^Test(i)=""

}

Same for the "If":

if a set b=a

or

if a {

 set b=a

}

Your choice? Or maybe even other options? 

Comments

For one-line if I prefer postconditionals:

set:a b=a

 

As for For - the option with braces. It's more readable. And one line inside For loop quickly and often becomes several lines, so braces become mandatory anyway.

But...

Can one condition in "postconditionals" "quickly and often" convert into several lines?

Maybe postconditionals shoud be If with braces too?

Postconditionals are to be used only for the most simple checks, for example:

set:a="" a=10

instead of

if a="" {
  set a=10
}

But several conditions (or several lines inside) should be as if, I agree.

I tend to use "if" rather than postconditionals because it makes the code read more like spoken language, and I think it's easier to understand for those who are not seasoned Caché veterans.

However I make exceptions for "quit" and "continue":

quit:$$$ISERR(tSC)

continue:x'=y

I like to have these out in front, rather than buried in a line somewhere, so that when I'm scanning code I can easily see "this is where the execution will branch if a certain condition is met."

I also like this style, as it helps me spot the lines where execution at the current level might terminate.

Yep, brace always.

Because you will never know if you need to update your condition or chain another. The brace shows what is the scope of the condition, also for loops, for, for each, etc.

It is a good practices and it is more readable.

As Eduard Lebedyuk   has commented, a single IF for use to set a variable, the postconditionals is a good idea, coz you know the condition before to set.

Agreed, and if you want to have your cake and eat it, too:

if (a) { set b = a }

Brace always for me too, unless I am writing some quick debugging code or something along those lines

Agree with brace always. This really helps with new developers as the syntax is something they've seen before in other languages. 

Postcondition is preferred
especially  quit:error     or   continue:ignore

for i=1:1:5 write i," "     is as fine as    for i=1:1:5 {  write i," " } write "end"

but this is really bad:

         f i=1:1:5 d  w "? "
          .w i," "
         w "end"

 

Besides all that is mentioned, the If without braces has a side-effect that the If with braces does not have : it affects the system variable $Test.

USER>set $test=0
 
USER>w $test
0
USER>if 1=1 { write "one" }
one
USER>w $test
0
USER>if 1=1 write "one"
one
USER>w $test
1
USER>

Not important, only if you use the ELSE (legacy) command...

 

Thanks, Danny!

Is there any "non-legacy" case when $test can be useful?

I mean, should we consider using $test in a code as a bad practice?

If you want to check on the result of a LOCK command for example 

LOCK +<SOMERESOURCE>:<timeout>

IF $T {do something}

So I wouldn't call it bad practice just yet.

Yes, same here, i also use following code a lot:

Open <some device>:"R":0 Else  Write "could not open device" Quit

I always use Else on the same line just after the command that changes $test,

having too much instructions between them creates problems. 

I would do it this way to avoid Else:

Open <some device>:"R":0 If '$test { Write "could not open device" Quit }

C'mon, let's stop mentioning old features like argumentless Do and legacy ELSE

There's just no point in doing so.

In the real world, there are lots of programs still using this style, every developer using Caché Object Script should be able to read this and understand the side-effects, pitfalls, even if we recommend to not use it anymore... 

totally right!!
there is more old code out than you could / would like to imagine
And Caché is backward compatible for 40 years (since DSM V1)

New developers should not write it. But they need to understand it. Or at least recognize it.
It's pretty similar to reading the bible in original language with no translation. 

Danny and Robert, of course I understand what you are saying (we are the same generation!). Also, I am not trying to be the Developer Community police.

In posts that are asking "what's the recommended way to do this?" we should answer that question. There's no point in mentioning old syntax in that context. Saying "don't do this, but you might see it someday" helps the subset of people that might work with old code, but doesn't help and may confuse the people that are working with new code. It doesn't belong.

On the other hand, if there's a post asking "what are some older/confusing syntaxes I might encounter when working with M or ObjectScript?" we should answer that question. We all have examples of that.

I wonder if it would be possible to have a more rigid syntax check (now in Atelier / and switchable)
to enforce all kind of brackets (eg: condition like in JS). 

I tried to recommend this while teaching COS myself.
With very limited success as some Dinos ignored it.
 

Everyday I read legacy code like this, but if we're talking about the ideal (like coding guidelines) we want to avoid these. I agree that every Object Script developer should be able to read this code, but they should also know that there are better modern ways to write this. 

I think that part of growing the community of Object Script developers is to lower barriers to entry. Having worked with a lot of new Object Script developers (and been one myself), one of the largest barriers to entry is the prevalence of cryptic legacy syntax in existing code. 

Remember, just because we can read it doesn't mean that everyone can.

Argumentless DO is still the simplest way to introduce a scope to NEW a variable, including $namespace or $roles. Yes, you can extract that out to another procedure or method, but I often prefer to do it inline.

Good point Jon. Here's a concrete example, taken from the CheckLinkAccess classmethod of %CSP.Portal.Utils (2017.2 version). I have done the macro expansion myself and highlighted the expansions:

  Set tHaveAllRole = 0
  Do 
  . New $roles
  . Set $roles=""
  . If ($extract($roles,1,$length("%All"))="%All") Set tHaveAllRole = 1
  If tHaveAllRole {

  ...

 

yesyesyesyesyesyes

Congratulations!

I new I had seen something similar

We want our code to be as accessible and readable as possible to as many people as possible. We always prefer whitespace formatting and curly brackets to arcane one line syntax. Someone who's not an Object Script expert will know what this does -

if a { 
   set b=a 
}

But there's no guarantee that a novice will necessarily know what these do -

if a set b=a
set:a b=a

Good code is readable and clever one liners will get in the way of that goal.

"We want our code to be as accessible and readable as possible to as many people as possible. "
Do we? ;)
"We always prefer whitespace formatting and curly brackets to arcane one line syntax." -- I don't ;)
Using arcane one liners shows that you are a superiour wizard. One should always use the short forms of commands to make code more terse and save space. If people want to read the code, they can use 'expand code' in studio (which expands the shortform commands to the full version).
Note, there might be some sarcasm in here.
People who know me, might also know that I'm only maybe 80% joking;)

you remind me the joke about hieroglyphs:
-  What's wrong with them?
- Nothing! Priests in old Egypt were reading the "book of death" like you would read a newspaper.
- You just have to learn the 'encryption'.

The legacy versions of If and For commands don't use curly braces. But there are no legacy versions of While and Do/While, which require curly braces. So rather than trying to remember which commands use or don't use curly braces, just use curly braces all the time for these, as well as Try/Catch.

Regarding post-conditionals, it's a good habit to put parentheses around the condition, which allows you to put spaces in the condition.

This is valid syntax: set:a=4 y=10

This is valid syntax and more readable: set:(a = 4) y = 10

This is invalid syntax: set:a = 4 y = 10

 

I just went through the whole conversation on this and find it knowledgeable for me as i am beginner for cache. Now i am able to understand how to use them in with the standards and where i need to careful thanks for all. 

Hi, colleagues!

The sense of the discussion I raised is to get Coding Guidelines for ObjectScript to let us cook the code which suits the goals and team guidelines the best. 

Thanks for a lot of bright thoughts and true experience!

If I am modifying a routine I didn't originally write then I tend to stick to the coding style the original author adopted - even it is using legacy syntax. It is good practice not to mix old and new constructs within the same code block ie. don't mix curly braces with line-oriented syntax.  If you are completely re-writing something or writing new code then I prefer curly braces.

I would have liked to re-factor some of my code better but I found the 'use for' construct breaks when you try that. In this pseudocode, I was using the 'write' statement to write to a file on the file system. There is a condition that is checked to determine whether it gets written to file1 or file2.

// Run an extract based on arrayOfIds
for arrayIndex=1:1:arrayOfIds.Count() 
{
       // Continue to loop through array of you detect an errorStatus
      continue:errorStatus'=1
      open file1:("WNS":::):10
      // Inner-for 1
      use file1 for testIndex=1:1:testSetObj.TestCollection.Count()
       {
       }
       open file2:("WNS":::):10
       // Inner-for 2
       use file2 for testIndex=1:1:testSetObj.TestCollection.Count()
       {
       }
// end outter-for

I miss 3 things in this example:

  • There is a condition: I found no IF nor anything
  • at the end of the inner loop there is no CLOSE file1 7 /  CLOSE file2  for file1 or file2
  • OPEN file:("WNS": ......)   
    • W - Write is ok
    • S - Stream mode is OK
    • N - i s questionable. if you are not on VMS you override the file at each outer loop
      http://docs.intersystems.com/latest/csp/docbook/DocBook.UI.Page.cls?KEY=GIOD_rmsseqfiles#GIOD_rmsseqfiles_use

      New file. If the specified file does not exist, the system creates the file. If the specified file already exists, the system creates a new one with the same name. The status of the old file depends on which operating system you are using. On UNIX® and Windows, Caché deletes the old file. (Note that file locking should be used to prevent two concurrent processes using this parameter from overwriting the same file.)

      So to get all output you may append arrayIndex to filename to have unique file names

Thanks for the comments Robert. It's safe to assume there is an IF statement in each inner-for  to determine whether to do anything or not. Each object in arrayOfIds needs to be opened and a Boolean check is done against a property. I haven't had a problem with the 'N' parameter. The code appears to store 'write' records in process memory before writing everything to file after the outer-for. $ZSTORAGE has been buffed to accommodate this.

// Changes the max-limit on the process memory from the default 16384 Kilobytes
set $ZS=49152

Unique file names have been achieved using the following assumptions:

1) The process will run no more than once a day
2) Append today's date in ODBC format using $zd(+$h,3) to the end of the filename.

The 'close' statement appears after the outer-for but I'm not sure if the curly braces implicitly does it anyway.

// Null check. Close if necessary. (May be implicitly closed by outter-for loop).
if $GET(file1)'="" close file1
if $GET(file2)'="" close file2

Thanks for the clarification.
It's sometimes hard to guess the picture if you miss some pixels.

I'd still recommend moving the CLOSE after each inner loop to have OPEN / CLOSE strictly paired

The implicit CLOSE happens only if you exit the partition. There's no class to act undercover. 
Curly braces just structure the code with no operational side effect.