-
Notifications
You must be signed in to change notification settings - Fork 0
ExperimentView Refactoring Highlights
This document will only highlight the more interesting and important information.
Although this is for the ExperimentView the goal is that this should be the same across all views. Other views will be adjusted as needed and as time permits (although some of this work has been done as required through refactoring, but only what was required).
Firstly the view should be limited to just GUI components and custom sub-components. Whenever there is a group of basic GUI components that go together they should be grouped together in a sub-component. There should be NO inner eventbus subscriber classes, no code to handle button presses, and so on. All subscribers should be in their respective subscriber classes and all buttons logic (or other components) should be in their respective Action classes (more on these later). The ExperimentView code should ideally just but some basic components combined with subcomponents. And all components should be limited to gui code. This not only keeps the logic nice and tidy and separated but it will also greatly minimize merge conflicts which had been plaguing us.
Subscribers vs Actions -> Subscribers are used when something outside of the current tabbed browser (view) updates the view such as an experiment being updated by the backend. Actions are classes to store the code of an individual user initiated action such as clicking on the save draft button, start training, and so on.
ExperimentView and NewExperimentView extend from DefaultExperimentView (SharedExperimentView extends ExperimentView). The default view manages the setting and updating of the experiment for all components. All subcomponents of these views need to implement ExperimentComponent which consists of two methods: setExperiment() and updateExperiment(). This is to allow the DefaultExperimentView to be able to loop through all the experimentComponents and/or experimentComparisonComponents and update accordingly with minimal code.
The DefaultExperimentView also includes several getters that are commonly used such as getSegmentIntegrator(), getExperimentId(), getExperimentDAO(), and so on so that we can pass the view to the Subscribers and Actions to reduce the number of method parameters.
All data is fully loaded before the components are rendered. That is to say the components no longer need to know what has been updated within the component such as the status, etc. and can safely assume that everything is fully loaded. This removes the need for all components to try and re-render items multiple times (which can be expensive). The biggest one was training status which was re-calculated all over the place.
The experiment instance is shared across all components rather than being cloned everywhere. This not only saves in memory but also means that say a subscriber updates the experiment then all components can safely just re-render themselves and no longer have to worry about updating different parts and data of the experiment. It also greatly simplifies the code.
The setExperiment() method both reloads the experiment from the database and calls ExperimentComponent.setExperiment() on each component from the experiment lists. In contrast the updateComponents() just calls ExperimentComponent.setExperiment() but does NOT do any database calls. The updateComponents() assumes that the experiment just re-renders itself. An example of this is an eventbus subscriber update in which there is no need to reload everything from the database where we can instead just update the experiment instance and call experimentView.updateComponents() or experimentView.updateComparisonComponents().
The updateExperimentFromComponents() method is used mainly by the NewExperimentClass to update the experiment instance by the components through the ExperimentComponent interface's updateExperiment() method. As we don't always use Vaadin binders we sometimes have to manually update the Experiment instance and this guarantees it works consistently throughout. So for example the main SaveDraftAction can just call newExperimentView.updateExperimentFromComponents() and then saves the Experiment instance to the database. It doesn't need to know or understand how each components updates the data within the Experiment instance.
Whenever possible I moved data items within the Experiment class. Especially if they were being loaded and passed around in parallel with the Experiment such as the RewardVariables.
Extra methods and flow have been added to DefaultExperimentView to guarantee certain code and methods are called in the right order. Eventually as the rest of the application is refactored some of this code will be pushed up to PathMindDefaultView.
initLoadData() is overriden and calls loadFullExperimentData() so that we can guarantee the loading and modifying of the experiment instance is atomic (synchronized). In fact any code that modifies the experiment instance needs to get the lock through getExperimentLock() and getComparisonExperimentLock().
isValidView() is now called in PathMindDefaultView after the data is loaded. Ideally this could be done before all the data is loaded (where only key data is loaded) but due to time constraints it's done after (keep in mind loading an Experiment with full chart data can currently take 1-2 seconds). Ideally we'd want to load just what we need to confirm and then forward to the appropriate page. This is used for example for when a user tries to call NewExperiment/id when it should be Experiment/id, at which point, in which case we auto forward the user to the correct view.
The creation of components is in it's own method called createComponents() which is called before getMainContent(). This is easier to understand, makes sure that everything is consistent, and guarantees that the UI is correctly loaded and available (more details below).
All code that modifies the experiment and/or comparisonExperiments needs to be surrounded by these locks. Keep in mind that components should never modify the experiment instance any more, at best it should only be events coming through Eventbus Subscribers, Actions, and the initial loading of the experiment in the view. This is very important because it was inconsistently done before. Again no internal values such as the training status from the Experiment instance should be calculated and internally modified in the GUI code.
The main change to the eventbus is that PathmindViewBusEvent has been removed. This is because we no longer create events to update other components within the same view, all that code has been moved to Action classes. The previous solution of using the eventbus was a workaround for complexity and fragility of the code. It was meant to be a temporary stage until we could isolated the code into logic blocks. Now that the code has all been cleaned up we can just call the components directly and events are only meant to be fired when they are from the backend OR where a user initiated action requires an updated on another tabbed panel (such as marking an experiment as a favorite or archiving an experiment with a couple of minor exceptions due to time constraints).
All hooks to buttons being clicked and so on now should call a static method in an Action class. There is no interface for now for a number of reasons (subject to change). The first reason is that there is no standard method parameter that could be defined (at least for now) that would meet the needs of all Action classes. Secondly until everything is converted the goal is to avoid holding any state in the Action classes because this was a significant cause of the fragility in the views, components, and eventbus code. By having a static method and the class having no attributes everything must be passed in and be completely self-contained, there are no subtle side effects. This does sometimes lead to slightly longer parameter lists but the trade-off is well worth it. Once we've other parts of the code base we'll be in a better position to make a final decision on how we want to do this. That being said having everything self-contained in private will make any refactoring very easy should we change any architecture. In fact the way the Subscribers and Actions have their code isolated will really help when we add a messaging system to support multiple webapp instances.
A few other benefits is that all the GUI code only contains GUI code. Any button click listener code, etc. is all handled in an Action class, meaning GUI code is only button.addClickListener(click -> SomeAction.performAction(...)). This then means the action code is completely separated from the GUI code. By separating them we will also greatly minimize merge conflicts and it also make it extremely easy to find any code related to an action. And in several cases we've been able to reuse the same Action classes whereas before this was replicated and not always consistently.
There has been a lot of cleanup in ExperimentUtils as well as other data model Utils classes. In essence a lot of code that was replicated (and spread across many places) was combined in single ExperimentUtils methods. There was also some code within the Experiment class that would update the state of the Experiment such as updateTrainingStatus() but this has been moved to ExperimentUtils to keep it all consistent. Only the set() and get() methods are in Experiment, all other methods go in ExperimentUtils.
ExperimentUtils has also been moved to the shared module and ExperimentGuiUtils remains in the webapp. One of the big issues we had previously is that the DAO (database) layer needed to modify the Experiment instance, and since ExperimentUtils wasn't available to it we would return values from the DAO layer, run some code on ExperimentUtils, and then call the DAO layer again. This completely removes any possibility of transactional database calls. Not only that but it forced a lot of backend logic in the webapp UI code layer that shouldn't have been there. And even worse a lot of code had to be repeated many times across the application whenever an Experiment because it couldn't just be done before being passed to the GUI.
Many Subscribers were removed as they are no longer needed. As well subscribers modifying any internal value of a data model object such as the Experiment (or comparison experiment) needs to acquire their appropriate lock. For that matter Action classes as well.
All Subscribers now have push automatically enabled. That is rather than to explicitly wrap all the code in every Subscriber around PushUtils this is automatically done as every subscribers needs some kind of push or other (the goal is to update the GUI after all). This saves a lot of ceremonial code and prevents subtle bugs from appearing. It also removes a lot of extra push code that was all over the code base that was not needed but put there just in case. We can also guarantee that the correct UI instance is loaded and pushed to.
It is very important that any filtering on the Subscriber be done in filterBusEvent() which wasn't always done because it will prevent threads from being created in the eventbus. It is a lot more efficient to prevent a thread from being created then creating one only to have it do nothing.
All components within the ExperimentView should extend ExperimentComponent. This is so that the view can automatically loop through them all and just call setExperiment() or updateExperiment() without having to manually keep track of all the components everywhere.
Important -> No ExperimentComponent should have a constructor with the Experiment in it. That is to say all ExperimentComponents should be constructible without an Experiment. This is now a necessity because the comparison components will not have an Experiment at construction time. Previously a lot of components had loaded constructors.
All code within the view and any components within that should call the view.getUISupplier() instead of UI.getCurrent() or whatever means was used. The getUISupplier() is defined in PathmindDefaultView. This is because the UI is properly and consistently loaded in PathmindDefaultView in the beforeEnter() lifecycle method so that it can be correctly used in the view, it's components, the subscribers, actions, and so on. This also makes handling the TimeZoneId a lot easier and much more consistently through the code.