Closed dpastoor closed 3 months ago
you have a point. These vpc functions were clean at the start, but they have grown fast :) The general structure you list above is correct, and it is a good idea to extract the parts in single sub-function wherever possible. I think for the plot and "plot" and "customize" parts this should be relatively straightforward. Some parts of the pre_processing (e.g. checking/parsing arguments etc) can also be extracted as a general function, in fact this is already partially done in the function format_vpc_input_data().
However, the processing of data is more tricky, as it is fairly different between vpc for continuous, censored, categorical, and tte data. I propose to leave these functions alone for now and possibly come back to those later. We can start with the "plotting" and "customize" parts, as well as the "pre_processing". That would be a lot cleaner already.
Sounds good - I'll start extracting those components - do you want me to make a branch or just stick directly in the master. My thought is for these 'reorganization' parts just doing in master should be fine, but when we attempt a conversion to OO probably will branch in case sh*t hits the fan trying to figure things out.
Master is fine, changes won't be dramatic and the package is in development stage anyhow.
After reading the code some it looks like we can 'break apart' these monolithic vpc functions and take a more modular approach to make maintaining and updating easier.
As it stands it looks like inside of vpc.R it handles:
The first thing I propose is we break apart the preproccessing, processing, 'base' plotting, and further customizations into individual functions, so the vpc() function would be little more than a wrapper along the lines of
given the overlap across vpc functions this would also help reduce copied code etc...
Thoughts?