6
\$\begingroup\$

I am trying to figure out whether I am following best practices while passing arguments to functions.

These are my class definitions (Please note, that I have just provided the high level interfaces for the Source and Database class definitions here, to keep this post focused on the part of the code that I would like to get reviewed).

Class interface for Source and Database:

"""
class Source():
    """
    This class represents the source database from which the raw data is
    extracted. It opens the source file containing the raw data, extracts
    the data and then closes the Source database once the processing is
    done

    """
"""

"""
class Database():
    """
    This class represents the final database to which the data is written
    to. This class opens the final database on the harddrive, prints the
    cleaned up data into the database and then closes the database and
    quits excel

    """
"""

Class interface for PatternA:

class PatternA():
    """
    PatternA represents a set of images each of which contains a set of
    holes. Using a program `appA` I generate numerical data corresponding
    to each of the holes which is stored in a csv file, represented by the
    Source class.  

    The PatterA class extracts that data, cleans that data of any
    non-numerical values, and also removes some datapoints that are
    obviously incorrectly measured by `appA`. Then it stores it back into
    another database, represented by the Database Class
    """

    def __init__(self, Source_instance):
        self.Source_instance=Source_instance

    def move_data_to_database(self):

        #opens the excel file containing the raw data
        self.Source_instance.open(self)

        #extracts the data from the excel file containing the raw data and
        # and converts it into the xlwings library format
        Source_rng_excel=self.extract_data(Source_instance)

        # cleans the data from any non-numerical data and from obviously
        # incorrect data points
        clean_table = self.clean_hole(Source_rng_excel)

        # generates statistics based on the data in tabular format
        hole_characteristic = self.gen_statistics(clean_table)

        # the data then has to be reformatted into a form so that 
        # xlwings can print it out into into the final database
        label, stat = self.generate_label(
            hole_characteristic, clean_table)

        # generate_clean_table prints out the raw data extracted minus any 
        # spurious data in a format list that can be directly printed out 
        # to the database
        clean_data = self.generate_clean_table(clean_table)

        # the tuple label, stat, clean_data is returned, to the original 
        # point from which move_data_to_database is called so that the 
        # Database class can print these out to the database
        return label, stat, clean_data

    """
    The following functions are implemented in my actual code. Since the
    function interface for all these functions are similar, I have provided
    the detailed implementation only for gen_statistics in the next
    section, but only provided the function interfaces for the rest of the
    functions

    def extract_data(self, Source):
        .
        .
        .

    def clean_hole(self, source_range):
        .
        .
        .

    def gen_statistics(clean_table):
        .
        .
        .

    def generate_label(hole_characteristic, clean_table):
        .
        .
        .

    def generate_clean_table(clean_table):
        .
        .
        .
    """


if __name__== "__main__":
    aSource=Source('/path/to/Book1.rrf')
    aDatabase=Database('/path/to/database.xlsx')
    aPatternA=PatternA(aSource)
    label, stat, clean_data = aPatterA.move_data_to_database(aSource)
    aDatabase.print_data(clean_data)
    aDatabase.pretty_print_data(label, stat)

My question is related to how to pass arguments to the functions extract_data,clean_hole, gen_statistics, generate_label, generate_clean_table.

Since the function interface for the extract_data,clean_hole, gen_statistics, generate_label, generate_clean_table, are similar I will take one of the functions gen_statistics, and describe my conflict.

Implementation of gen_statistics

In the current implementation, I have passed the arguments to these functions explicitly such that the output from the above functions depend only on the inputs to the functions. To me, this makes it cleaner to test these functions. However, I could have passed the arguments implicitly, by making the return variables an attribute of the PatternA instance. For example, I could write the gen_statistics function in 2 ways:

  1. current implementation:

    def gen_statistics_vA(self, clean_table)
        """Suppose the dirty table as extracted from the Source table is of the
        the form:
                    -------------------------------------------------------------
                    |  hole1  |  hole2  |  hole3  |  hole4  |  hole5  |  hole6  |
                    -------------------------------------------------------------
        | Image1  | 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4|
        | Image2  | 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4|
        | Image3  | 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4|
        | Image4  | 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4|
        | Image5  | 20 | 0.4| 37 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4|
    
        then after cleaning the table would look something like this:
    
                    -------------------------------------------------------------
                    |  hole1  |  hole2  |  hole3  |  hole4  |  hole5  |  hole6  |
                    -------------------------------------------------------------
        | Image1  | 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4|
        | Image2  | 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4|
        | Image3  | 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4|
        | Image4  | 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4|
        | Image5  | 20 | 0.4|    |    | 20 | 0.4| 20 | 0.4| 20 | 0.4| 20 | 0.4|
    
        As you can see hole2 corresponding to Image 5 has been removed.
    
        The table is then transformed into a form and is passed to
        gen_statistics as clean_table
        [1, 1, 20, 0.4]
        [1, 2, 20, 0.4]
        [1, 3, 20, 0.4]
        .
        .
        .
        In the following piece of code, I first append the 3rd element of the
        all the sublists of clean table which has the form [1, 2, 20, 0.4],
        into 1 list of the form [20, 20, 20, ..], and then calculate the mean.
        I then append it to stat_characteristic. 
    
        To use xlwings and print out the data easily to a excel file, the data
        has to be in a list format, that is why I am appending the
        stat_characteristic to a list. In future, I could have to add other
        characteristics such as 3-sigma, range etc.. but for now, I just have
        mean.
    
        Once I have evaluated stat_characteristic, I return the list to
        move_data_to_database.
        """
    
        characteristic_in_column = [[], []]
        stat_characteristic = []
        for hole in clean_table:
            index_characteristic = 0
            for characteristic in hole[2:]:
                characteristic_in_column[index_characteristic].append(
                    characteristic)
                index_characteristic += 1
    
        for characteristic in characteristic_in_column:
            mean_characteristic = statistics.mean(characteristic)
            stat_characteristic.append(mean_characteristic)
    
        return stat_characteristic
    
  2. The above function could also be written in the following form, which is how I wrote it originally

    def gen_statistics_vB(self)
        characteristic_in_column = [[], []]
        stat_characteristic = []
        for hole in self.clean_table:
            index_characteristic = 0
            for characteristic in hole[2:]:
                characteristic_in_column[index_characteristic].append(
                    characteristic)
                index_characteristic += 1
    
        for characteristic in characteristic_in_column:
            mean_characteristic = statistics.mean(characteristic)
            self.stat_characteristic.append(mean_characteristic)
    

I originally implemented the gen_statistics function and other functions with the second form ie. with gen_statistics_vB(self), where the variables self.stat_characteristic and self.clean_table are menbers of PatternA class. This reduced the number of variables that I needed to pass to gen_statistics, and reduces the complexity in writing the function interfaces. Further, instead of returning a variable stat_characteristic, like in gen_statistics_vA, I can directly assign it to self.stat_characteristic, which also reduces the number of lines of code I have to write.

However, as my code base grew, I started realizing that its difficult to make sure that the function gen_statistics written in the second form is truly independent of the other functions or the state of the PatternA instance object. This is because its accessing two instance variables, self.clean_table and self.stat_characteristic, which could change with the state of the instance object. Further, if I had to test the gen_statistics_vB funciton using the second form , I would have to instantiate an object of type PatternA, assign the correct values to self.stat_characteristic, and only then I could test the function gen_statistics_vB.

On the other hand, if I had test gen_statistics_vA, I could do that much simply with PatternA.gen_statistics(PatternA, small_clean_table). Further, because the variables used in gen_statistics are directly passed through the function interface, they are not dependent on any other variables in the class, or how they change. So its easier to make sure that the gen_statistics works reliably.

I have read Code Complete 2, which states that I think both these rules are simplistic and miss the most important consideration: what abstraction is presented by the routine's interface? If the abstraction is that the routine expects you to have three specific data elements, and it is only a coincidence that those three elements happen to be provided by the same object, then you should pass the three specific data elements individually. However, if the abstraction is that you will always have that particular object in hand and the routine will do something or other with that object, then you truly do break the abstraction when you expose the three specific data elements.

In this case, when the function gen_statistics is called, I do have the object in hand. However, I am unclear as to what he means by If the abstraction is that the routine expects you to have three specific data elements. The routine does expect 1 specific data element, but thats because I wrote it in a different way.

In Clean Code the author Robert Martin mentions that we should try to minimize the number of arguments passed, but 2 arguments are ok. In this case, to me it seems justified to use 2 arguments, because it leads to better encapsulation of the functions within gen_statistics.

Question:

  1. To me it seems, that gen_statistics_vA is better encapsulated and is thus a better design. I face this kind of situation a lot, so I wonder what is your opinion on the best design of these functions?
  2. None of the books talk about designing functions which are easily testable. To me, it seems that gen_statistics_vA is better encapsulated so its easier to test. Should I consider the testability of functions when I write functions?
\$\endgroup\$
  • \$\begingroup\$ You've improved your quesiton a lot ;-) however, I'm still not convinced. I suggest quoting the code that is there only for reference purposes so that it's clear that this should not be reviewed. Especially I mean the incomplete classes and methods. Otherwise people will think you've redacted the code too much or that it's not yet written. \$\endgroup\$ – t3chb0t Feb 15 '18 at 20:58
  • \$\begingroup\$ Thanks for your comment. I updated and quoted the sections of the code where I only provided the interfaces. Please let me know if you have any comments \$\endgroup\$ – alpha_989 Feb 15 '18 at 21:44
0
\$\begingroup\$

That was an inspiring read :)

I'll start addressing your questions, starting from the second one. I have to say I'm surprised that the Clean Code does not provide examples of testing functions (peeping in the index I can see some pages dedicated to that and TDD). I would definitely recommend that you think about how to test your code a bit before you write it. Even if you don't test the whole code, or even if you write tests after.

What is easier to test? A method that belongs to a class that you need to initialize using another method, with a whole setup? Or a function that receives an input as a parameter and generates an output?

Obviously second option is easier since we can mock the input and check the output. But which function do you need? That's completely situational and brings us to question 1

What is the best design?

Here your question is: Do you want to keep state in the class?

Depends.

On the first place, I'd say you did a good job separating into functions what each step does. But now you are concerned about which option is best in terms of readability.

One of the sentences you write says This reduced the number of variables that I needed to pass to gen_statistics, and reduces the complexity in writing the function interfaces...

You definitely in this case want to have a nice function interface, that describes what kind of objects the function expects to deal with. And what kind of output is expected from this function.

For saving a couple lines, you're missing a much better description on what the functions are doing and what are the expected outputs on each step, which the docstring would describe properly to potential users.

What you should consider: data size

Since I don't want to jump to conclusions early, it will be also interesting to know how big is the data you're dealing with.

If you handle a huge data source, accessing this object a few times per method would be much more interesting than sending a heavy object around, and that may be a much better indicator of which option you should choose if your program is getting slow (sacrificing some code readability)

If you are going to add some steps in the middle in the future, think if it will slow the process in the end

Would be good if you can show some benchmark on how long it takes each step to run, that way you know how heavily you interact with the data at each point

Cheers ;)

\$\endgroup\$
  • \$\begingroup\$ Thanks for your detailed review. When I first read your answer, I didn't know or think about testing at all, so I didnt undertand what you mentioned. In fact, these classes do have integration tests, but don't have unit-tests. So I didn't have to think about this issue of writing good testable function interfaces. \$\endgroup\$ – alpha_989 Apr 21 '18 at 3:42
  • \$\begingroup\$ I did write a lot more code, and do have unit-tests for the code I wrote for the classes/functions I wrote afterwards, and I went with passing the arguments to the functions directly. I can see looking back, that I did that mainly because it made the functions easily testable.. and what you mentioned makes perfect sense now. \$\endgroup\$ – alpha_989 Apr 21 '18 at 3:44
  • \$\begingroup\$ Also appreciate your comment about the size of the data structure.. yeah. the data structure I am passing could in some cases be large.. but I am kindof dealing with smallish data which doesn't necessarily represent the data size that will be passed around when/if this goes into production. So I will have to think about that. \$\endgroup\$ – alpha_989 Apr 21 '18 at 3:45
  • \$\begingroup\$ I haven't yet run a profiler in python.. so it will take a bit of time to understand how to do that.. but in the coming weeks I will post an update.. once I finish up what I am working on currently... \$\endgroup\$ – alpha_989 Apr 21 '18 at 3:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.