[Asterisk-code-review] app_if: Adds If, EndIf, and ExitIf applications (asterisk[master])

[N A]

Attention is currently required from: George Joseph.
N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16121 )

Change subject: app_if: Adds If, EndIf, and ExitIf applications
......................................................................


Patch Set 1:

(1 comment)

Patchset:

PS1: 
> I'm kinda "iffy" on this review 😊 […]
I don't think that was quite the idea here, it was more simple things like this:

same => n,If($["${feature-a}"="1"])
same => n,Gosub(dosomething,s,1)
same => n,Gosub(dosomethingelse,s,1)
same => n,Set(something=something)
same => n,NoOp(${DB_DELETE(something/thing)})
same => n,EndIf()
same => n,If($["${feature-b}"="1"])

Currently, there are 3 ways this could be done, each of which is a bit hacky in some respect.

same => n,GosubIf($["${feature-a}"="1"]?dosomething,s,1)
same => n,GosubIf($["${feature-a}"="1"]?dosomethingelse,s,1)
same => n,ExecIf($["${feature-a}"="1"]?Set(something=something)
same => n,GotoIf($["${feature-a}"!="1"]?postdbdel)
same => n,NoOp(${DB_DELETE(something/thing)}) ; functions always evaluate even if ExecIf is 0, so this is double trouble. This cannot be encapsulated in a conditional directly because it must never be parsed if false.
same => n(postdbdel),ExecIf($["{feature-b}"="1"]?) ... etc.

Problem: very repetitive. We shouldn't be checking the same condition more than once or twice. A better way:

same => n,GotoIf($["${feature-a}"!="1"]?postfeaturea)
same => n(postfeaturea),GotoIf($["${feature-a}"!="1"]?postfeatureb)
same => n(postfeatureb),GotoIf($["${feature-b}"!="1"]?postfeaturec)

Problem: a) The labels are confusing. The first one is structured differently from the last one, from the middle. You basically need a label for the end of each part, to skip over the block in the middle. Probably the cleanest way to do it now.

Here's a more semantic way to do it:

same => n,While($["${feature-a}"="1"])
same => n,Gosub(dosomething,s,1)
same => n,Gosub(dosomethingelse,s,1)
same => n,Set(something=something)
same => n,NoOp(${DB_DELETE(something/thing)})
same => n,ExitWhile()
same => n,EndWhile()
same => n,While($["${feature-2}"="1"]) etc.

This is the most readable code, as it's very clear what's going on, doing what we want to do, instead of using a jillion GotoIfs in order to skip stuff we *don't* want to do. Problem: The ExitWhile is critical. Otherwise, we have an infinite loop, and it's easy to forget the ExitWhile. Thus:

same => n,If($["${feature-a}"="1"])
same => n,Gosub(dosomething,s,1)
same => n,Gosub(dosomethingelse,s,1)
same => n,Set(something=something)
same => n,NoOp(${DB_DELETE(something/thing)})
same => n,EndIf()
same => n,If($["${feature-2}"="1"]) etc.

Basically, this is intended to be similar to the last case, a semantic way to execute a block just one time, so if there are a few of these, the dialplan is simplified, very easy to read and maintain and update. That was the main objective here, so Lua would not help for this case.

If you're still iffy, though, maybe we can toss this one ;)



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/16121
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I3aa3bd35a5add82465c6ee9bd86b64601f0e1f49
Gerrit-Change-Number: 16121
Gerrit-PatchSet: 1
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Attention: George Joseph <gjoseph at digium.com>
Gerrit-Comment-Date: Tue, 06 Jul 2021 13:49:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: George Joseph <gjoseph at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210706/a5a0394c/attachment-0001.html>