1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
|
## Usually modules with instance variables considered harmful
### Background
Rails somehow encourages people using modules and instance variables
everywhere. For example, using instance variables in the controllers,
helpers, and views. They're also encouraging the use of
`ActiveSupport::Concern`, which further strengthens the idea of
saving everything in a giant, single object, and people could access
everything in that one giant object.
### The problems
Of course this is convenient to develop, because we just have everything
within reach. However this has a number of downsides when that chosen object
is growing, it would later become out of control for the same reason.
There are just too many things in the same context, and we don't know if
those things are tightly coupled or not, depending on each others or not.
It's very hard to tell when the complexity grows to a point, and it makes
tracking the code also extremely hard. For example, a class could be using
3 different instance variables, and all of them could be initialized and
manipulated from 3 different modules. It's hard to track when those variables
start giving us troubles. We don't know which module would suddenly change
one of the variables. Everything could touch anything.
### Similar concerns
People are saying multiple inheritance is bad. Mixing multiple modules with
multiple instance variables scattering everywhere suffer from the same issue.
The same applies to `ActiveSupport::Concern`. See:
[Consider replacing concerns with dedicated classes & composition](
https://gitlab.com/gitlab-org/gitlab-ce/issues/23786)
There's also a similar idea:
[Use decorators and interface segregation to solve overgrowing models problem](
https://gitlab.com/gitlab-org/gitlab-ce/issues/13484)
Note that `included` doesn't solve the whole issue. They define the
dependencies, but they still allow each modules to talk implicitly via the
instance variables in the final giant object, and that's where the problem is.
### Solutions
We should split the giant object into multiple objects, and they communicate
each other with the API, i.e. public methods. In short, composition over
inheritance. This way, each smaller objects would have their own respective
limited states, i.e. instance variables. If one instance variable goes wrong,
we would be very clear that it's from that single small object, because
no one else could be touching it.
With clearly defined API, this would make things less coupled and much easier
to debug and track, and much more extensible for other objects to use, because
they communicate in a clear way, rather than implicit dependencies.
### Exceptions
However, it's not all that bad when using instance variables in a module,
as long as it's contained in the same module, that is no other modules or
objects are touching them. If that's the case, then it would be an acceptable
use. Unfortunately it's a bit hard to code this principle in the cop, so
for now we rely on people turning off the cops, if they think that the use
conform this rule.
Here's an acceptable case:
``` ruby
# This is ok, as long as `@attributes` is never used anywhere else.
# Consider adding some prefix or suffix to avoid name conflicts though.
# rubocop:disable Cop/ModuleWithInstanceVariables
module Rugged
class Repository
module UseGitlabGitAttributes
def fetch_attributes(name, *)
@attributes ||= Gitlab::Git::Attributes.new(path)
@attributes.attributes(name)
end
end
prepend UseGitlabGitAttributes
end
end
```
Here's a bad example which we should rewrite:
``` ruby
module SpamCheckService
def filter_spam_check_params
@request = params.delete(:request)
@api = params.delete(:api)
@recaptcha_verified = params.delete(:recaptcha_verified)
@spam_log_id = params.delete(:spam_log_id)
end
def spam_check(spammable, user)
spam_service = SpamService.new(spammable, @request)
spam_service.when_recaptcha_verified(@recaptcha_verified, @api) do
user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true)
end
end
end
```
There are several implicit dependencies here. First, `params` should be
defined before using. Second, `filter_spam_check_params` should be called
before `spam_check`. These are all implicit and the includer could be using
those instance variables without awareness.
This should be rewritten like:
``` ruby
class SpamCheckService
def initialize(request:, api:, recaptcha_verified:, spam_log_id:)
@request = request
@api = api
@recaptcha_verified = recaptcha_verified
@spam_log_id = spam_log_id
end
def spam_check(spammable, user)
spam_service = SpamService.new(spammable, @request)
spam_service.when_recaptcha_verified(@recaptcha_verified, @api) do
user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true)
end
end
end
```
And use it like:
``` ruby
class UpdateSnippetService < BaseService
def execute
# ...
spam = SpamCheckService.new(params.slice!(:request, :api, :recaptcha_verified, :spam_log_id))
spam.check(snippet, current_user)
# ...
end
end
```
This way, all those instance variables are isolated in `SpamCheckService`
rather than who ever include the module, and those modules which were also
included, making it much easier to track down the issues if there's any,
and it also reduce the chance of having name conflicts.
### Things we might need to ignore right now
Since the way how Rails helpers and mailers work, we might not be able to
avoid the use of instance variables there. For those cases, we could ignore
them at the moment. At least we're not going to share those modules with
other random objects, so they're still somehow isolated.
### Instance variables in the views
They're terrible, because they're also shared between different controllers,
and it's very hard to track where those instance variables were set when we
saw somewhere is using it, neither do we know where those were used when we
saw somewhere is setting up them. We hit into a number of 500 errors when we
tried to remove some instance variables in the controller in the past.
Somewhere, some partials might be using it, and we don't know.
We're trying to use something like this instead:
``` haml
= render 'projects/commits/commit', commit: commit, ref: ref, project: project
```
And in the partial:
``` haml
- ref = local_assigns.fetch(:ref)
- commit = local_assigns.fetch(:commit)
- project = local_assigns.fetch(:project)
```
This way it's very clear where those values were coming from. In the future,
we should also forbid the use of instance variables in partials.
|
