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
Since the function interface for the
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:
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
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.clean_table are menbers of
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
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
object. This is because its accessing two instance variables,
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
On the other hand, if I had test
gen_statistics_vA, I could do that much
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
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
- To me it seems, that
gen_statistics_vAis 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?
- None of the books talk about designing functions which are easily testable.
To me, it seems that
gen_statistics_vAis better encapsulated so its easier to test. Should I consider the testability of functions when I write functions?