How can we make Communicators reliable and portable

[jovanbulck]

Problems with the Communicator pattern

I think the switch to the communicator pattern has been one of the key moves in the kotnetcli project (i.a. discussed in #13). I strongly belief it is the good way to continue on. It nicely decouples core from visualisation concerns, allowing a pluggable visualisation system for everyones needs. It also keeps the code clean, allowing the programmer to focus on his task: the UNIX philosophy - do one thing and do it good.

Currently the pattern merely encapsulates all the visualisation calls. Initiating it can still be messy somehow. As we are porting kotnetcli on various platforms now however, time has come to enhance the pattern once more.

As discussed in various issues already (#48 #43), the current communicator implementations aren't portable / cross platform. This creates a dependency hell and makes keeping everything stable and up-to-date a hassle... :-/

Tackle the problem, not its symptoms

I belief this problem is not due to the communicator pattern an sich though: we shouldn't solve the problem by tackling its symptoms. That is to say: the real problem here is that specific Communicator instantiations currently aren't portable. The CursesCommunicator or DialogCommunicator for example happen to be to worst: i.e. the 'symptoms'. The problem itself is that the current code somehow expects a client to use all of the communicators (by importing them all) which then are instantiated according to the command-line flags passed.

What we should really fix therefore is the portability of communicators, the core of the problem.

Requirements for a solution

In my opinion, a good portable solution to this problem should have the following characteristics:

• crash locally: The code should 'delay' crashing to make sure it only crashes when needed. I.e. the code shouldn't crash for a communicator that you don't use (e.g. if you don't have dialog installed, but you don't use the DialogCommunicator there should absolutely be no problem)
• fail-gracefully: let it crash, but in a controlled way: always leave the system in a desirec state. Clean thing up nicely and show an instructive error message to the user. (e.g. the cursesCommunicator and ColoramaCommunicator have things to do for reinitializing the screen...)
• encapsulation of platform-specific code: most parts should be as platform-neutral as possible, but platform-specific code is not necessarily a problem (different platform ask for different methods). Platform specific code should clearly be encapsulated though, as opposed to having if (os ==) checks wandering everywhere in the code. Platform-specific code should be cleanly separated from platform-independent code.
• pluggable: extending the system with yet-another-communicator should be easy, straightforward and clean

  Vers une architecture logicielle

I'll quickly sketch a possible outline for a solution here:

• inheritance: we should continue further dividing communicators into a class hierarchy. Common platform-independent code should be abstracted in superclasses and platform dependent specific code should be in a proper sub-class. See the encapsulation of platform-specific code and pluggable criteria above.
• Fail gracefully using exitFailure(msg) and exitSucces(): The non-communicator code should always call a function like co.exitFailure(msg) when it catches an exception and wants to exit the system. The communicator can then leave its internal state as desired and if needed, communicate an error message to the end-user. See the fail-gracefully criterion above. Symmetrically, we'll need a function like exitSucces() that also cleans up internal state and -if needed- communicates success to the end-user. Things like #47 are then trivial BTW.
• import dependencies on init(): We should also fix the import statements in a cleaner way: only import when actually used. I.e. import dialog only in DialogCommunicator.init() and colorama only in ColoramaCommunicator.init() etc. See the crash locally criteria above. More specifically, a user should be able to run kotnetcli without first installing colorama and/or dialog etc., be it in 'simple' plaintext mode then.
• crash platform-independent: the system should 'crash' gracefully, in the same way for a mac user who doesn't (and cannot) have dialog installed and a Linux user who just happens not to have the utility installed. This can be done by importing dependencies on init(), as decribed above, and subsequently catch an importexception and exitFailure();
• independance of command line arguments: parsing of command line arguments happens outside the scope of communicators, so they shouldn't rely on it in any way. More specifically, calling a communicator that should not / can not work on a certain platform (eg dialog on mac os x or mac-specific bubbles on linux) should not crash uncontrolled. See the fail-gracefully criterion above. The communicator should simply show a nice informative error message and exit. Thing like having a custom Argumentenparser() per OS, as discussed in issue #43 can surely be done. This should be merely 'syntactic-sugar-like' however: it shouldn't prevent you from using the communicator in the first place, merely saving you from trying communicators that wont work anyway (but wont crash unpredictibaly).

  This is the end - the end

Sorry for the looong post, but I think it's good to share some thoughts on this one ;-) I'll try to come up with more specific implementation ideas / class hierarchies and things soon...

Please comment below for impressions / ideas / comments / etc

[GijsTimmers]

Thank you, this was an interesting read.

Some thoughts:

import dependencies on init()

This is an interesting idea, but note that it explicitly doesn't comply with PEP 8.

crash platform-independent

If I understand correctly, you would like to have something like this:

class ColoramaCommunicator():
    def __init__(self):
        try:
            import colorama
        except ImportError:
            self.exitFailure("De colorama-module is niet beschikbaar.")
        ...
        ...
        ...
    def exitFailure(msg):
        print msg
        show_terminal_cursor()

One thing I see here is that ColoramaCommunicator() is no longer solely a 'visualising' class: it also controls itself (see self.exitFailure()). Could you perhaps expand a bit on this?

independance of command line arguments

If I get it right, what you're trying to say is that you should, for example, be able to use kotnetcli --bubble on a system even if that system doesn't support it. kotnetcli --help shouldn't show the option though. Is that right?

[jovanbulck]

Good catches, some replies below:

import dependencies on init()

This is an interesting idea, but note that it explicitly doesn't comply with PEP 8.

You're right indeed, I wasn't sure this is possible in Python. We should look into another way of realizing the crash locally criterion above (i.e. import only when needed). I think this criterion is crucial and should somehow be accomplished.

I originally picked up the idea of importing on init from this link, where we can maybe look a bit further. This link confirms indeed it can be very ugly.

I looked into it a bit further now. The idea/problem is not unknown to pyhton programmers ;-) It is sometimes called lazy import. Check out some interesting links : 1 2 3

[1] is definitely most interesting for us, it also quotes PEP you mentioned. Moreover the top answer states:

The best reasons I've seen to perform lazy imports are:

Optional library support. If your code has multiple paths that use different libraries, don't break if an optional library is not installed.

Long story short: we should look into a clean way of doing this and then not having the imports at the top of the file is no big deal I think...

One thing I see here is that ColoramaCommunicator() is no longer solely a 'visualising' class: it also controls itself (see self.exitFailure()). Could you perhaps expand a bit on this?

Good point indeed, but the rationale here would be that a Communicator instance is allowed to keep state, in order to do its cohesive job, i.e. visualizing. Being 'allowed' to keep internal state is indeed something you really need for advanced communicators. As you explicitly said above, the communicator "also controls itself", which is exactly what we want I think. Once you allow a communicator to hold internal state, it should also be responsible for it itself. That of course implies that the other code then is responsible to call co.exitFailure() when needed. This again abstracts the internal state solely to the communicator instance. You should think of it as a private thing, only to be managed and known by the Communicator, I think.

independance of command line arguments

If I get it right, what you're trying to say is that you should, for example, be able to use kotnetcli --bubble on a system even if that system doesn't support it. kotnetcli --help shouldn't show the option though. Is that right?

No that's not really what I'm trying to say here. It is indeed a bit fuzzy, but in fact what I want to express here is straightforward: Communicators shouldn't 'rely' in any way on external code that should make sure the correct platform-dependent instance is instantiated. Communicators should always fail gracefully. That is to say, never 'trust' that they are correctly instantiated. More specific example: an OSXBubbleCommunicator should in theory 'work', i.e. fail gracefully, on a Linux system.

About the --help then: I like the idea of hiding (or disabling for that matter; shouldn't matter as I explained above) things that any way won't work on a specific platform. The only pitfall here is that - as explained above - Communicator code shouldn't rely on it in any way. Indeed, the parsing of option flags is completely independent and merely a front-end to the communicators.

[jovanbulck]

I see communicator getting stuffed up with if "inloggen" then blabla elif "uitloggen" otherblabla ... checks. See e.g. the last (very useful I think) enhancement eventSamenvattingGeven.

Such thins are commonly called bad smells, indicating the need for refactoring the code.

Apart from the more drastic changes sketched above and here, I propose the following:

• It's ok for communicator behavior to be dependent on logging in / off / ... This is even desired behavior: the dialog communicator for example shows an overview of what's coming before it's executed. The steps differ when logging in and out. When logging out with a bubble communicator no upload and download should be given. Another example of such behavior: eventSamenvattingGeven etc.
• It's not ok to mix the different behaviors through the code using if checks however. Hence I see different options:
  1. Let the communicator know at construction time what is will be communicating. A dialog can then draw the steps to come on the screen. A method such as eventSamenvattingGeven is then not necessary anymore: beeindig_sessie can be passed an argument indicating success/failure. Downside is that inloggen/uitloggen specific behavior will still be captured inside if checks.
  2. Have different communicator hierarchies that encapsulate specific behavior: eg inloggenDialogCommunicator etc.

Communicators should be thought of as a kind of Strategy pattern or Template method pattern. This would justify option ii: further subclass communicators where needed.

I think option ii is a good one, but we maybe wont want the kotnetcli class to know about which communicators have specific subclasses and which not. We should therefore maybe consider an intermediate communicatorCreator level

[jovanbulck]

I got it! This is a nice example of a Factory pattern The key idea is to encapsulate the login/logout dependent creation of Communicators in another object: the factory. Checkout the wikipedia links to get the overall idea if you're not familiar with it...

• create the extended hierarchy of communicators: a Login/logout/..._Type_Communicator subclass for every communicator having specific behavior (e.g. samevatting bij beeindig_sessie)
• create a CommunicatorFactory class with a subclass for each type: LoginCommunicatorFactory, LogoutCommunicatorFactory, ...

kotnetcli would look like:

def aanstuurderObvArgumenten(argumenten):
    ...
    ############## switch on login-type flags ##############
    if argumenten.worker == "login":
        print "ik wil inloggen"
        fac = LoginCommunicatorFactory()

    elif argumenten.worker == "logout":
        print "ik wil uitloggen"
        fac = LogoutCommunicatorFactory()

    ############## switch on communicator-related flags ##############
    if argumenten.communicator == "dialog":
        print "ik wil fancy dialogs"
        co = fac.createDialogCommunicator()

    elif argumenten.communicator == "bubble":
        print "ik wil bellen blazen"
        co = fac.createBubbleCommunicator()

     ...

Creating of communicators is thus effectively encapsulated in the factory. kotnetcli.py remains responsible for the command line interface only. This will definitely be the next grand step forward for the overall design quality :-) :+1:

[GijsTimmers]

Thank you for looking this up. This should be implemented before releasing 2.0.0.

[GijsTimmers]

So that means that communicator.py will look something like this:

class LoginCommunicatorFactory():
    class CreateQuietCommunicator(self):
        def __init__(self):
            pass
        def eventNetloginStart(self):
            pass

        (...)

    class CreatePlaintextCommunicator(self.CreateQuietCommunicator):
        def __init__(self):
            Fore.RED = ""

        (...)

class LogoutCommunicatorFactory():
    class CreateQuietCommunicator(LoginCommunicatorFactory.CreateQuietCommunicator):
        def some_specific_logout_feature(self):

        (...)

Right?

[jovanbulck]

No, not at whole. I see the pseudo code I posted confused you since the method names are wrongly capitalized. Fixed it above

The idea of a factory pattern is to encapsulate the creation of the correct object in a complex hierarchy in another object: the factory.

The factory provides a method for every communicator and returns the correct communicator depending whether its a login/logout factory.

Checkout the wiki description: https://en.wikipedia.org/wiki/Abstract_factory_pattern

[jovanbulck]

The idea is to remove all uit_te_voeren_procedure like code. Which is as explained above a bad smell. Any uit_te_voeren_procedure-specific code should be implemented in separate sub classes.

In order to create the correct (sub)class communicator, we'll then use a factory:

## Abstract super class (not intended to directly create), encapsulating things common
## to a Login- and LogoutSummaryCommunicator
class SuperSummaryCommunicator(QuietCommunicator):
    def eventPingFailure(self):
        print "Niet verbonden met het KU Leuven-netwerk."
    def eventPingAlreadyOnline(self):
        print "U bent al online."

class LoginSummaryCommunicator(SuperSummaryCommunicator):

    def eventTegoedenBekend(self, downloadpercentage, uploadpercentage):
        print "Download: " + str(downloadpercentage) + "%" + ",",
        print "Upload: " + str(uploadpercentage) + "%"

    def beeindig_sessie(self, error_code=0):
        if error_code == 0:
            print "login succesvol."
        else:
            print "login mislukt."

class LogoutSummaryCommunicator(SuperSummaryCommunicator):

    def beeindig_sessie(self, error_code=0):
        if error_code == 0:
            print "logout succesvol."
        else:
            print "logout mislukt."

## The abstract factory specifying the interface and maybe returning 
## some defaults (or just passing)
class SuperCommunicatorFabriek:
   def createSummaryCommunicator():
     pass

class LoginCommunicatorFabriek(SuperCommunicatorFabriek):
   def createSummaryCommunicator():
     LoginSummaryCommunicator()

class LogoutCommunicatorFabriek(SuperCommunicatorFabriek):
     def createSummaryCommunicator():
       LogoutSummaryCommunicator()

### kotnetcli.py

def aanstuurderObvArgumenten(argumenten):
    ...
    ############## switch on login-type flags ##############
    if argumenten.worker == "login":
        print "ik wil inloggen"
        fac = LoginCommunicatorFabriek()

    elif argumenten.worker == "logout":
        print "ik wil uitloggen"
        fac = LogoutCommunicatorFabriek()

    ############## switch on communicator-related flags ##############
    if argumenten.communicator == "summary":
        print "ik wil fancy dialogs"
        ## next line will auto create correct instance :-)
        co = fac.createSummaryCommunicator()

     ...

[jovanbulck]

the nice thing is that we can then encapsulate all action-specific communication nicely

e.g. print "Netlogout openen....... " instead of "Netlogin openen....... " on reply to a eventNetloginStart()

[GijsTimmers]

Thank you for taking the time and effort to provide this piece of code, it's helped me understand what you want to do exactly. I read the Wikipedia page, but I didn't get it.

The code example you provided looks very interesting, my hands are itching to code again and get this factory model done. Maybe I can do some work tonight.

[jovanbulck]

:+1:

[GijsTimmers]

I'm working on the code now. I think I didn't really get it.

class LoginCommunicatorFabriek(SuperCommunicatorFabriek):
   def createSummaryCommunicator():
     LoginSummaryCommunicator()

Why not just put the contents of LoginSummaryCommunicator() in the LoginCommunicatorFabriek()?

[jovanbulck]

Why not just put the contents of LoginSummaryCommunicator() in the LoginCommunicatorFabriek()?

No the factory is solely responsible for creating the object. It is called a creational design pattern, for outsourcing creation of an object in a complex hierarchy.

When putting the implementation there, you (1) lose cohesion (2) it's impossible, since this is a constructor, there are various other methods, you cant just 'paste in' (eg kuleuvenEvent(), ....). A method ain't a class...

[jovanbulck]

lazy importing as discussed above can easily be achieved in the corresponding factory methods: 980af26c717909b4d01bc007cf84e656efcd2731

This allows kotnetcli to run without first imporiting all packages, even those not used (eg dialog, notify2, ...)

To test: change the colorama import to something non-existing in coloramac.py and try to run kotnetcli.py -t --> should work :)

[GijsTimmers]

Jo, how much work is left on this issue? As far as I can tell, we only need to finish the lazy imports, right?

[jovanbulck]

Jo, how much work is left on this issue? As far as I can tell, we only need to finish the lazy imports, right?

Yes, proof-of-concept (including lazy imports) works indeed. We need to decide on the definitive communicator API and finish things.

[GijsTimmers]

Alright. Jo, you're a lot better than I am when it comes to software design. Could you go ahead and design the API?

[jovanbulck]

Ok, I'll try to look at it this weekend...

[jovanbulck]

106 proposes a simplified communicator interface with cleaner error handling separation of concerns between Worker and Communicator classes -- as also discussed in #59

[jovanbulck]

As one of the final things to be done here, I think we should move the credentials querying (i.e. the user input) to the Communicator interface.

Currently, the front-end (kotnetcli.py and friends) is responsible to ensure the Credentials object passed to the Worker does indeed have credentials. If not, the front-end prompts the user for credentials and stores them in the Credentials object.

Improved design outline:

• front-end creates the appropriate Credentials object (keyring, guest, ...) without having to know anything about its internals. Related to this: front-end should not know about the specifics of the forgetCredentials method. This should be handled via a dedicated ForgetCredentialsWorker object.
• Worker ensures Credentials contain actual values before passing them on to the Browser class.
• If the Credentials object is empty, the worker can retrieve values via the Communicator, which queries them from the user. Likewise, the dedicated ForgetCredentials worker can communicate its result via the Communicator interface.

Advantages:

• Reduced coupling in front-end: it should not know specific about Credentials This is all encapsulated in the Workers.
• Improved cohesion in the front-end: it should solely care about creating the necessary objects to start the Worker:
  • i.e. parsing command line arguments for kotnetcli
  • i.e. parsing GUI input for kotnetgui
• Improved cohesion in the communicators: this is the only place where user input/output is handled after the netlogin process has been initiated:
  • QuietCommunicator should provide some default minimalist implementation to prompt user credentials (i.e. the current prompt_user_creds method from the front-end)
  • Other communicators (e.g. DialogCommunicator or GUICommunicator) can override this default to present another input mechanism to the user

[jovanbulck]

move the credentials querying (i.e. the user input) to the Communicator interface

Done!

[jovanbulck]

I consider this one done! Comminicators have been thoroughly refactored in the dev branch now.