sonar-openedge icon indicating copy to clipboard operation
sonar-openedge copied to clipboard

"Variables should not be initialized to default values" should also flag fields and parameters

Open movedoa opened this issue 4 years ago • 6 comments

Only the variable definition gets flagged here. The rule should also flag temp-table fields and parameters with the default init value.

DEFINE VARIABLE cFoo AS CHARACTER NO-UNDO INIT "":U.

DEFINE TEMP-TABLE ttFoo NO-UNDO
   FIELD FooChar AS CHARACTER INIT "":U
   FIELD FooInt AS INTEGER INIT 0
   FIELD FooLog AS LOGICAL INIT FALSE
.

PROCEDURE Foo PRIVATE:
   DEFINE OUTPUT PARAMETER FooOut AS CHARACTER INIT "":U NO-UNDO.
   DEFINE OUTPUT PARAMETER FooOutInt AS INTEGER INIT 0 NO-UNDO.
   
   ASSIGN
      FooOut = "Foo":U
      FooInOut = TRUE
   .
END PROCEDURE.

movedoa avatar Sep 15 '21 15:09 movedoa

Thinking about it, INPUT-OUTPUT PARAMETER should be ignored, this is horrible but can lead to side effects. INPUT and OUTPUT parameters and temp-table fields should be fine.

movedoa avatar Sep 20 '21 09:09 movedoa

this is horrible but can lead to side effects.

Now it begins to get interesting! If something is horrible and can lead to side-effects, then it should NOT be ignored! Instead it is something you want to be aware of in a code review, when possible automated in a lint rule.

So, can you elaborate why it is horrible and more importantly: what the side effects are?

jurjendijkstra avatar Sep 20 '21 11:09 jurjendijkstra

this is horrible but can lead to side effects.

Now it begins to get interesting! If something is horrible and can lead to side-effects, then it should NOT be ignored! Instead it is something you want to be aware of in a code review, when possible automated in a lint rule.

So, can you elaborate why it is horrible and more importantly: what the side effects are?

Just to be clear here, what i mean is that the ability to define a default value for input-output is horrible :P

The thing with the rule is that all other cases never lead to a change in runtime behavior, with INPUT-OUTPUT one has to be very careful when removing these default values since the default value here actually does something. Imho this would be a seperate rule since this rule is only about useless code that doesn't do anything.

movedoa avatar Sep 20 '21 13:09 movedoa

this rule is only about useless code that doesn't do anything.

Aha ok I understand, thanks for clarifying that.

My favourite rules are the ones that warn against sourcecode that does do something, and does something different than the programmer intended, or maybe does exactly what the programmer intended but not obvious to the maintenance programmer :-)

jurjendijkstra avatar Sep 20 '21 13:09 jurjendijkstra

The thing with the rule is that all other cases never lead to a change in runtime behavior, with INPUT-OUTPUT one has to be very careful when removing these default values since the default value here actually does something.

In what case does an initial value on an input-output parameter do anything? The only warped example I can come up with is:

procedure foo:
   define input-output parameter io_i as integer no-undo initial {&sequence}.

   message {&sequence}.

end procedure.

stefandrissen avatar Sep 20 '21 19:09 stefandrissen

The thing with the rule is that all other cases never lead to a change in runtime behavior, with INPUT-OUTPUT one has to be very careful when removing these default values since the default value here actually does something.

In what case does an initial value on an input-output parameter do anything? The only warped example I can come up with is:

procedure foo:
   define input-output parameter io_i as integer no-undo initial {&sequence}.

   message {&sequence}.

end procedure.

You are absolutely right, since i never used this strange contruct i was under the impression that initial value is used when the parameter is ? but that doesn't seem to be the case. So INITIAL is completely useless for INPUT-OUTPUT.

Maybe someone knows of a special case where it does something but otherwise, ignore what i said.

movedoa avatar Sep 21 '21 07:09 movedoa