Architectural problems and anti-patterns in our code.

[cjcenizal]

Goal

Let's use this issue as a meta issue to track fundamental software design issues within Kibana.

• Find examples of our code which people have found confusing, e.g. Courier being confusing/difficult to use, @weltenwort mentioning problems with searchSource in Discover.
• Identify why they're confusing, e.g. anti-patterns, code smells.
• Suggest solutions via refactoring or redesigning.
• Link out to relevant issues and PRs.

[weltenwort]

Great idea, @cjcenizal. I'll dump a list in here as a starting point. Maybe we can move that into the issue's main body after sorting through it.

Behaviour and data are attached directly to $scope

What is the problem?

The functions and other properties that make up the controller's API are not namespaced. Since the scope is prototypally inherited by child directives, they might (unwittingly or not) read or write the properties of a parent directive's controller, thereby coupling the directives closely and implicitly.

Possible Solution

Attaching the functions and data properties to the controller object keeps the scope clean. The controllerAs property of the directive definition object allows for the controller to be made available in the directive's scope under a descriptive name. Using the bindToController property even the directive's attributes can be attached directly to the controller object.

Example Occurrences

• Almost every directive that has a controller.

[weltenwort]

Directives trigger actions implicitly by modifying scope data

What is the Problem?

User intent is translated into system actions in at least three (inconsistent) ways. One of them is to modify an object available on the scope in the expectation that some other part of the system will watch that object and react to the change correctly. This has several drawbacks:

1. It is hard to trace the action from the source of the user intent (the originating directive) to any of the reacting code segments. This implicit side effect makes debugging and refactoring very difficult.
2. There is no way to notice (or protect against) cascades or even endless loops of reactions, possibly leading to bad performance.

Possible Solution

Model user interactions as explicit function calls that push the side effects to the very boundary of the system. These functions can be injected into the directive as an attribute (via an isolate scope binding).

Example Occurrences

• Sorting in the document table header

[weltenwort]

Business logic is scattered across many controllers

What is the Problem?

Many changes to the application state are defined inline in the controllers. This has the effect that some controllers are extremely large (~500 lines in example 1) and hard to read. Any inclusion of state change logic in the controllers limits the possibility of reuse by other parts of Kibana or its plugins, thereby reducing consistency. In the same way, documenting and testing this functionality might be more difficult, inhibiting the malleability of the codebase.

Possible Solution

Encapsulate common business logic in state changes in functions or services and limit code in the controllers to performing the minimal adaption to view-specific data structures and functions.

Example Occurrences

• Sorting in the document table header

[weltenwort]

I'm trying to not restate too many of the SOLID principles and other general best practices for large codebases here. Many of the fundamentals like function/argument naming and function purity could keep us busy for a while. 😉

[cjcenizal]

Decouple code from Angular as much as possible

• Extract business logic into regular JS modules (out of controllers, services).
• Migrate away from using scope to share data to another pattern (e.g. shared services/singletons, event bus / observer pattern, Flux Stores).
• Consistent patterns of writing directives, which will at least support a future migration if necessary.

[stacey-gammon]

Unintuitive Folder Structure

Currently it is difficult to quickly know where to look for certain files, or logic. For example, we have:

• ui/public/directives/saved_object_finder.js
• ui/public/saved_objects/saved_object_registry.js (the only file in this folder)
• ui/public/courier/saved_object.js

The angular style guide (see #9049) has some suggestions around folder and file structure

[cjcenizal]

Define clearer Top Nav "menu" interface

The system

Currently, apps configure the Top Nav by setting a configuration array on the scope, commonly called topNavMenu, and also scope vars to be later referenced by the instantiated Top Nav:

// app_example.js
$scope.topNavMenu = [{
  key: 'fooDropdown',
  description: 'Foo',
  template: require('path/to/foo_dropdown.html')
}];

$scope.opts = {
  sayHello: () => console.log('Hello, world'),
};

The app's root template instantiates the Top Nav and passes in the configuration array:

<!-- app_example.html -->
<kbn-top-nav name="dashboard" config="topNavMenu">
  <!-- Other top nav stuff -->
</kbn-top-nav>

The kbnTopNav controller contains logic for building the Top Nav menu buttons and applying this configuration to them, as well as logic for displaying the relevant dropdown for the selected menu button.

The templates that are used to build the Top Nav have access to the inherited scope:

<!-- foo_dropdown.html -->
<button ng-click="opts.sayHello()">Say Hello</button>

Problems

1. Two interfaces are used to serve one purpose. We're using the kbnTopNav directive to create the top nav, but we're also providing it with data and functionality by setting properties on the inherited scope. This makes it difficult to follow the data flow and understand how to use the top nav effectively.
2. Data and functionality is implicitly provided to the top nav, instead of explicitly. By using the inherited scope as a mechanism for providing data and functionality to the top nav, we're implicitly providing dependencies, making it difficult to follow the data flow and use the top nav effectively.

Solution

Ideally, we'll use a single interface for instantiating the top nav, and this will be the only interface we use to explicitly provide dependencies.

[cjcenizal]

Closing this as all of these issues will be addressed by moving to React.