Handle data checks at object creation time (constructor)

[steven-esser]

In many class functions we have data checks like this:

if self.object is None and self.other_thing is None:
  return

We should really handle these in the constructor to reduce LOC.

[johnmhoran]

@MaJuRG What is the technique to handle these in the constructor? In the Delta class, for example, our _license_diff() begins with this:

        if not self.new_file or not self.old_file:
            return

Similarly, the Delta to_dict() method begins with this:

        if self.new_file is None and self.old_file is None:
            return

If we remove these blocks from the methods, what is the corresponding code in the constructor?

[JonoYang]

@johnmhoran When you create the object, you check to see if the file you are going to create an object for exists before you create the object, e.g.

class Delta(object):
    def __init__(self, new_file=None, old_file=None, delta_type=None):
        # Check to see if the file is not None. There should also be code to see if the file exists on the 
        # system.
        if not self.new_file or not self.old_file:
            self.new_file = new_file
            self.old_file = old_file
        else:
            # raise an exception here if needed

        self.category = delta_type
        ...

From there, you do not need to do the check in the class methods because you already performed the check when you initialized the class.

[johnmhoran]

@JonoYang Assuming the code blocks are removed from the methods, what's the process by which this code in the constructor would result in a bare return if, for example, _license_diff() were called and either the new_file or the old_file did not exist?

[JonoYang]

@johnmhoran From my cursory glance at the code, I would say that if either new_file or old_file is None, then do not create the object, i.e. return out of the constructor. If there is no delta to be performed for a particular file, we do not need to make a Delta object for it. @MaJuRG Is my line of thinking correct?

[johnmhoran]

@JonoYang Thanks Jono. I saw your edited reply. I still don't follow how the process works. It's late, but if you (and/or Steven) have time tomorrow morning, perhaps we could do a quick uberconf to explain.

What I still don't follow is precisely what happens when one of those methods is called and one of the pair of files does not exist. With the current code, we get a return. How do we get the same result with the code in the constructor?

[steven-esser]

@JonoYang For that object, probably not. We just need it do be something iterable, like an empty list or dict.

In reality, it probably should be an empty file object.

[steven-esser]

@johnmhoran The point is, we want to change the code to where we no longer need a return statement, or those checks of object existence.

[johnmhoran]

@MaJuRG I get that much, but not how we accomplish that in the constructor. I'll do some research, see what I find.

[steven-esser]

@johnmhoran We currently do some of this in our Scan object:

class Scan(object):
    """
    Processes the files contained in an incoming ScanCode scan to test whether
    they are valid, retrieve the scan's 'files_count' value, create a list of
    File objects, and generate a dictionary of File objects indexed by a
    selected key.
    """
    def __init__(self, path=''):
        self.path = '' if path is None else path
        self.files_count = self.get_files_count(self.path)
        self.files = self.load_files(self.path)
        self.options = self.get_options(self.path)

i.e. we set path to '' if path is None. In our case, we would want to replace the '' with some sort of iterable, (an empty File object in the perfect world)

[steven-esser]

@johnmhoran its not the greatest example, but thats what is meant by 'handling in the constructor'

[johnmhoran]

@MaJuRG Maybe I'm thinking about this incorrectly. I'm assuming we want to generate the same result as the current method code generates -- a simple return -- if one of those 2 methods is called and one of the pair of files is missing. Is that not the result we're after when we 'handle this in the constructor'?

[steven-esser]

@johnmhoran The problem is this: We want the _license_diff() function to work no matter if there is one, two, or no file objects passed when creating a Delta object. Currently, we have to have these two lines of code in license_diff() (and elsewhere in the codebase in various forms).:

if self.new_file is None and self.old_file is None:
    return

This is duplication we want to avoid. So how do we go about doing this? We structure our code properly and set object attributes properly when, say, one side of our delta object has None for self.new_file or self.old_file.

We have ultimate control over everything, so why not handle the if blahblah is None: checks at the constructor level? This removes the need for checking in every object method, and makes stuff easier to test.

Or, take a look at the Delta object's to_dict() function. We have that exact same check here as well. How should we fix this? Well, looking at the to_dict() code, its accessing the file objects directly, so we know we should probably set self.new_file and old_file to an empty File() object if the constructor is not passed a file object directly. something like:

__init(new_file=None, old_file=None, category=''):
  self.new_file = new_file if new_file else models.File()
  self.old_file = old_file if old_file else models.File()

Now, we will have to verify how we are creating this empty file object, but if we did it correctly, all the various object accessing calls in to_dict() should work after removing the two lines of checking.

Additionally, all subsequent new methods can operate on the assumption that self.new_file and old_file are always file objects, even when one or both file objects were not passed to the Delta object's constructor.

[johnmhoran]

@MaJuRG The issue I'm struggling with is how the current method code blocks emitting a return is equivalent to using empty File objects in the constructor. If they were equivalent, would we not have already been using (redundant) empty File objects in those 2 methods?