[Tutor] Looking for some comments on my code segment...

[Cameron Simpson]

On 23Feb2023 19:14, Dave (NK7Z) <dave at nk7z.net> wrote:
>def createtelnet():
>    global tn, failcode

Try not to use globals, it makes things harder to resue. You could just 
return these from the function, and have the caller store them in 
whatever fashion it prefers.

>    try:
>        tn = telnetlib.Telnet(HOST, PORT, TIMEOUT)
>        logdata = "Telnet object created"
>        logit(logdata)
>
>    except socket.timeout:  # Times out, exit with error.
>        failcode = 1        # Let error handler know what is happening.
>        logdata = 'Unable to create telnet object, timed out'
>        logit(logdata)
>        exitscript()        # Leave script.
>
>    except EOFError:
>        logdata = 'Timeout reached in login.'
>        logit(logdata)
>        failcode = 10
>        exitscript()        # Leave script.

As a general principle you should keep try/except clauses as small as 
possible. I'd shuffle the bove to be:

     try:
         tn = telnetlib.Telnet(HOST, PORT, TIMEOUT)
     except socket.timeout:  # Times out, exit with error.
         failcode = 1        # Let error handler know what is happening.
         logdata = 'Unable to create telnet object, timed out'
         logit(logdata)
         exitscript()        # Leave script.

     except EOFError:
         logdata = 'Timeout reached in login.'
         logit(logdata)
         failcode = 10
         exitscript()        # Leave script.

     else:
         logdata = "Telnet object created"
         logit(logdata)

The more you have in the try/except, the less certainty you have that 
any exception you catch cme from what you intended to handle i.e. the 
telnetlib.Telnet() call. Supposing there was an exception during the 
logit("Telnet object created") call? You'd treat t as though there had 
been a telnet failure, not a logging failure.

>Later on I am accessing the connection via:
>
>        try:
>            result = tn.read_until(b"\r\n", TIMEOUT)     # Get a line.
>            if result == 'b\'\'':
>                logdata = 'Stream has failed...'
>                logit(logdata)
>                failcode = 7
>                exitscript()  # Leave script.

Again, your try/excepts are too broad. Enclose _only_ the code whose 
errors you are handling.

Two other things: by using global variables and calling exitscript() 
from your set up function you're embedding policy in the function i.e.  
that this function will only ever be called once and therefore globals 
can store the result, and that failure of this function should exit the 
entire programme.

Usually a function might be called from many different situations, and 
maybe the caller has their own opinion about how failure should be 
handled, or might open 2 telnet connections, etc. By embedding policy 
about globals and aborting the programme in the function you remove the 
caller's flexibility.

I'd change the function to be something like this:

     # get rid of the "global" statement, so "tn" is a local
     tn = None  # placeholder values
     failcode = None
     try:
         tn = telnetlib.Telnet(HOST, PORT, TIMEOUT)
     except socket.timeout:  # Times out, exit with error.
         failcode = 1        # Let error handler know what is happening.
         logdata = 'Unable to create telnet object, timed out'
         logit(logdata)

     except EOFError:
         logdata = 'Timeout reached in login.'
         logit(logdata)
         failcode = 10

     else:
         logdata = "Telnet object created"
         logit(logdata)

     return tn, failcode

Then the caller can go:

     tn, failcode = createtelnet()
     if tn is None:
         logit('createtelnet failed, failcode = {failcode}')
     else:
         ... use tn to do stuff ...

Cheers,
Cameron Simpson <cs at cskk.id.au>