EmailDiscussions.com - View Single Post

JeremyNicoll · 18 Jun 2023, 07:29 PM

Thanks to @xyzzy for such thorough answers.

I am pleased to see someone else who writes the sort of comments I put in my own code. I don't class this as "a lot"; I class nearly every other programmer's code as "inadequately commented".

I do have an observation and some questions though. The first is that for code for one's own use one doesn't have to be terribly careful. I would fully expect that if xyzzy changes this code, he(?) pays full attention to the implications of what gets changed. But when code is made available for other people to use one can't be certain that they will concentrate as much, and/or understand what they are doing. That is, I think this publicly-shared code might need to be written so it doesn't assume parameters are configured correctly.

In the code that splits up a configured SPAM_THRESHOLD or SPAM_DISCARD value, the "else" action is to set values to 0. While for SPAM-THRESHOLD that means that later on every email will satisfy the "is my spam score greater than 0" test and thus have the spam score added to its subject, that's unlikely to matter. But the same thing would happen with SPAM_DISCARD and thus every one of a user's emails would be sent to the discard folder. I think that the score values set here should be SD1/ST1 as 9999 (ie much larger than likely to occur) and SD2/ST2 as a regex that will never match. I'd probably also set an error flag (which I'd have initialised to a default error message, clear only if parameters were ok, and set to a specific error message if specific errors were found), and only execute the following code if the error flag had been cleared. I /would/ set both the shouldn't match SD1/ST1/SD2/ST2 values AND the error message even if in theory only the test values or error message needs set, just to be on the safe side (if someone else changes later code without thinking or I change it without paying enough attention).

It's also not enough to check that individual parameters have valid values. You need to use them only if the combinations of parameters make sense.

Something like

not string :is "${ADD_SCORE}" "0"

is safe for xyzzy's own code (because he will be careful when setting ADD_SCORE's value in the first place), but not safe (albeit the risk of the condition accidentally being true is low for this specific action) for a user who might not have set ADD_SCORE at all or mis-set it. This "not string" is a shorthand for ADD_SCORE being set to 1 or 2, but really you should explicitly check that the value IS 1 or 2, not that it's not 0. Lots of things are not 0, but only two things are either 1 or 2.

I understand completely why you "addheader" error messages into the mails - but a user who never looks at headers won't ever see them (until, maybe, directed to look for them by you). For a user who screws up using code like this, a safer action might be the addheader plus stopping the sieve script completely (so that it can't take any unintended action).

Also... irrelevant in a way bearing in mind what I posted above about the "else" values of SD2 and ST2, but I was curious why you set those to "0-0". That makes no sense in regex terms; why not just code "0"?

18 Jun 2023, 07:29 PM	#59
JeremyNicoll Essential Contributor Join Date: Dec 2017 Location: Scotland Posts: 492	Thanks to @xyzzy for such thorough answers. I am pleased to see someone else who writes the sort of comments I put in my own code. I don't class this as "a lot"; I class nearly every other programmer's code as "inadequately commented". I do have an observation and some questions though. The first is that for code for one's own use one doesn't have to be terribly careful. I would fully expect that if xyzzy changes this code, he(?) pays full attention to the implications of what gets changed. But when code is made available for other people to use one can't be certain that they will concentrate as much, and/or understand what they are doing. That is, I think this publicly-shared code might need to be written so it doesn't assume parameters are configured correctly. In the code that splits up a configured SPAM_THRESHOLD or SPAM_DISCARD value, the "else" action is to set values to 0. While for SPAM-THRESHOLD that means that later on every email will satisfy the "is my spam score greater than 0" test and thus have the spam score added to its subject, that's unlikely to matter. But the same thing would happen with SPAM_DISCARD and thus every one of a user's emails would be sent to the discard folder. I think that the score values set here should be SD1/ST1 as 9999 (ie much larger than likely to occur) and SD2/ST2 as a regex that will never match. I'd probably also set an error flag (which I'd have initialised to a default error message, clear only if parameters were ok, and set to a specific error message if specific errors were found), and only execute the following code if the error flag had been cleared. I /would/ set both the shouldn't match SD1/ST1/SD2/ST2 values AND the error message even if in theory only the test values or error message needs set, just to be on the safe side (if someone else changes later code without thinking or I change it without paying enough attention). It's also not enough to check that individual parameters have valid values. You need to use them only if the combinations of parameters make sense. Something like not string :is "${ADD_SCORE}" "0" is safe for xyzzy's own code (because he will be careful when setting ADD_SCORE's value in the first place), but not safe (albeit the risk of the condition accidentally being true is low for this specific action) for a user who might not have set ADD_SCORE at all or mis-set it. This "not string" is a shorthand for ADD_SCORE being set to 1 or 2, but really you should explicitly check that the value IS 1 or 2, not that it's not 0. Lots of things are not 0, but only two things are either 1 or 2. I understand completely why you "addheader" error messages into the mails - but a user who never looks at headers won't ever see them (until, maybe, directed to look for them by you). For a user who screws up using code like this, a safer action might be the addheader plus stopping the sieve script completely (so that it can't take any unintended action). Also... irrelevant in a way bearing in mind what I posted above about the "else" values of SD2 and ST2, but I was curious why you set those to "0-0". That makes no sense in regex terms; why not just code "0"?