EddyG wrote: Thursday 25 November 2021 10:09
@rrozema Just curious.
Only 2 devices are used as trigger devices. Why do you check for device.isDevice and why do you check "if nil == sunup then"
Are not those checks redundant? B.t.w. my line would have been "if sunup == nil then"
And why "if not light.active then" could that not be changed in just 1 line "light.switchOn().checkFirst()"? Or leave the checkFirst all together?
Also the function "domoticz.devices().filter(LIGHTS).forEach(" takes a lot more time and memory than just a walk thru a few devices.
This is one script I use myself, only some names have been changed to fit the poster's question and to make it more clear as an example. I like my coding robust and thus I always try to put in the 'safety checks' on the type of the device and the creation of the devices. This saves myself a lot of time in debugging if something is wrong later, when I try to make changes in the code or as in this case, when I give the code to someone else and they try to run the script but forgets some preparation step or make a change/ an addition to script for their own needs.
The device variable is another one than the sunup, so their values may be different (and will be, when the other switch got engaged). device is the parameter passed to the script by domoticz/dzVents, the sunup value is that retrieved from the devices collection given the name that was specified in the top: there may be a typo in the name specified for SUNUP_NAME, the device may have been renamed, or there may be a bug somewhere in domoticz or dzvents. Multiple reasons can be thought of why the names could not match. I like my coding to be robust and if something is not as expected, I want to be informed so that I can check to see if my expectations are wrong or that something else is amiss. I am only human and as such I make mistakes -a lot-, and so are and do the developers of Domoticz & dzVents. We can't blame any of them nor me for that. But we can prepare for it by being explicit in our coding.
The if nil == sunup then is actually a programming habit intended for coding safety too. An often made error is to type a single = instead of a ==, which is an assignment instead of a comparison. I come from a C background (long time ago), and in C "x = nul" is a valid boolean expression that first assigns x the nul value, then evaluates to false. i.e. "if x = nul" is syntactically correct and accepted by the compiler, but most likely not what you intended. This error can be very hard to locate in a large chunk of code. If however you are in the habit of always putting the constant value on the left hand side of every comparison, f.e. nul = x, that
does result in a compile error: a constant can not be assigned a value. And thereby the error of writing a single = where a double == was intended is very easily located. LUA has all sorts of automatic type conversions and language constructs, including some involving assignments being treated as expressions. I'm 100% convinced that I do not know and never will know all of the details of the LUA language, so I apply this same habit to this language to, to save myself a lot of time if I accidently type = instead of ==.
The domoticz.devices().filter(LIGHTS).forEach( construct is there because I want uniformity, again for robustness reasons. If my code works for one light, it should work the same for multiple lights. I treat the devices().filter(<list>) as a black box. Not looking at it's implementation, nor making any assumptions on it's internals. By doing it this way, if we find that filter(<list>) is to slow, we can choose to optimise it and thereby fix the issue in each and every call to that function. If however I would have coded it to explicity to walk the list, then retrieve the device and call that device's switchOn() method, I owuld have to go through each and every script ever made to get the same optimisation. In fact, I did look at the implementation of devices().filter(<list>) some time ago because I had the same reservations you have, and saw that you are right: it simply walks the entire list. Yet, performance of this construct has never been a problem for me, so an optimisation was not really needed. There is another programming mantra for that:
first make sure it works correctly, then optimise it. In this case a huge optimisation could be achieved by keeping the devices in alphabetical order and doing a binary search in filter() instead of looping through all devices as it is implemented now. I'm convinced this would in fact help performance in a lot more places. But, if nobody -including me- notices a relevant performance degradation, why bother optimising it?
Finally, your question on checkFirst() is a very good one, because I'm a bit contradicting my previous point for using filter() here. There is however a reason for that: I've seen checkFirst() fail many times, in my own code and in the forum posts. For multiple reasons, often simple ones like the device type wasn't supported, as is perfectly documented, but he, I'm human and I forget... So that is why I avoid using checkFirst() and put an explicit if in my code to avoid calling switchOn or switchOf or setLevel (one that checkFirst doesn't support), etc if the device already has the value I want it to have.
) way to handle this?