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

[Dave (NK7Z)]

Cameron,

Many thanks to you for this suggestions, and more importantly why you 
suggested what you did!

I will make the suggested changes!

73, and thanks,
Dave (NK7Z)
https://www.nk7z.net
ARRL Volunteer Examiner
ARRL Technical Specialist, RFI
ARRL Asst. Director, NW Division, Technical Resources

On 2/24/23 14:18, Cameron Simpson wrote:
> 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>
> _______________________________________________
> Tutor maillist  -  Tutor at python.org
> To unsubscribe or change subscription options:
> https://mail.python.org/mailman/listinfo/tutor