►
Description
Today we attempt a refactor of a complex method.
https://gitlab.com/gitlab-org/gitlab/-/issues/395693
A
So
Chad
and
Cassio
we
were
just
discussing
for
the
pairing
session.
What
to
work
on
and
I
had
shared
two
issues
yesterday,
one
about
changing
some
search
team
application
settings
over
to
a
new
table
because
I
think
application
settings
is
like
570
columns
and
someone
from
my
team
Dimitri.
He
suggested
moving
everything
to
Json,
B
and
Chad.
You
asked
if
that
had
been
discouraged.
I
haven't
seen
it
be
discouraged.
B
C
A
Yeah
my
concern
was
I,
wanted
to
make
sure
that
we
had
like
enough
rules
around
what
could
be
stored
inside
the
field
so
that
it
doesn't
become
like
a
free-for-all.
That's
been
my
experience
with
sometimes
storing
stuff
in
Json
B
fields.
If
there's
not
like
specific,
like
validation
applied
to
it,
that
it
can
be
hard
to
be
confident
about
like
what's
in
there,
but
I
could
I
can
share
those
issues
with
y'all
later,
but
I'm
also
happy
to
work
on
the
refactor
method
that
I
shared
it's
or
chat
or
Cassie.
A
D
B
A
And
I
think
that
was
what
I
was
raising
up
as
like
of
making
sure
that
we
have
that
because
I've
had
times
where
would
didn't,
have
that
and
then
I
look
at
the
table
and
I'm
like
what
is
in
here
right
now.
Do
you
have
been
storing
stuff
in
here?
That's
not
even
related
to
what
the
column
is
supposed
to
be
like
it
yeah
it
just
becomes
very
hard
to
manage,
and
then
once
it's
like
that,
I
don't
know
how
you
kind
of
get
away
from
it,
but.
C
D
D
D
C
But
you
can
index
those
there.
You
can
index
the
fields
inside
of
the
Json
as
well,
but
I.
Think
if
the
Json
structure
is
too
big,
then
you
have
performance.
You
can
have
performance
issues
like
if
you
have
too
many
fields
inside
of
the
Json
beating
but
I'm,
not
sure
about
that.
It's
been
a
while,
since
I
touched
those
things.
A
C
In
my
personal
opinion,
I
think
Json
beats
good
for
very
well
defined
small
set
of
dudes
like
addresses
or
this
kind
of
stuff.
When
you
start
doing
settings
on
Json,
it's
a
little
bit
complicated
because
settings
tends
to
grow
so
I
think
another
table
can
work
a
little
bit
better
because
at
the
end
of
the
day
that
Json
starts
growing,
you
decide
to
move
to
another
table.
It's
two
steps.
Instead
of
one
foreign.
A
A
We
have
a
method
used
in
ee,
lib
elastic
latest
git
class
proxy,
which
is
named
blob
query
and
it's
extremely
difficult
to
read:
I
actually
really
dislike
having
to
come
in
and
work
on
things
in
here,
because
every
time
I
look
at
it
makes
me
feel
bad
and
guilty
because
I
don't
like
how
the
method
reads,
but
we
never
have
time
to
come
back
and
fix
it.
Often
when
we're
making
changes
in
here,
it's
never
a
priority.
A
I
think
for
folks
to
come
and
fix
this,
and
we
can
start
by
looking
at
just
the
five
I
guess,
the
the
method
itself.
We
have
three
rubricot,
disables
and
I
feel
like
every
time.
We
add
something
in
here
it
just
like
makes
the
problem
worse.
So
I
don't
know
when
we
do
refactors
like
this,
like
what's
the
best
way
to
get
started
but
like
this
is
terrible
and
I
yeah
I.
B
D
B
D
You
even
know,
if
simple
have
any
pants
to
flag
to
it,
to
say
every
branch
is
covered,
because
that
will
give
you
a
nice
safety
net
to
know
that
you're
not
breaking
any
of
the
logic,
because
you
can
cover
a
line.
But
if
it's
a
ternary
or
something
else,
you
could
be
missing
some
some
holes
in
your
testing.
B
B
A
C
C
D
D
D
D
A
B
A
D
A
D
A
B
D
A
Feel
like-
and
you
know,
I've
talked
to
you
about
this
before
I
have
noticed
and
Cassie
I,
don't
know.
If
you
read
into
this
a
lot
of
the
files
that
my
team
works
on,
they
are
in
so
many
RoboCop
excludes
that
you
don't
get
the
alerts
in
your
editor
and
then
the
problem
just
gets
worse,
as
we
add
code,
because
I
think
people
don't
realize.
D
D
A
B
D
D
A
B
A
B
A
D
D
B
A
D
A
A
A
I'm
having
a
bad
day
for
showing
things,
but
essentially
when
you
run
a
search
there,
if
it's
back
by
elastic
it
does
it
allows
you
to
do
code
aggregation
like
the
type
of
code,
the
type
of
file
the
interpreted
like
what
the
file
is
contains,
and
this
blob
query
is
used
in
both
cases.
It's
used
from
you
doing
the
actual
search
which
is
like
get
me
the
records,
and
it's
also
used
when
you're
saying,
like
I,
need
to
get
the
aggregations
of
what
type
of
code
is
available
for
this
like
search.
A
So
that's
what
and
then
there
also
was
this
count.
Only
that's
if
we're
doing
account
request,
which
is
what's
used
to
populate
the
number
in
the
tab
or
the
like
the
sidebar
of
how
many
results
came
back
like
we
don't
this.
This
query
is
used
by
like
three
different
types
of
searches
and
maybe
that's
part
of
the
problem.
It's
like
a
very
generalized
like
create
this
blob
query
and
it
has
a
bunch
of
if
statements
in
it
that
are
like
if
it's
a
count.
A
D
Almost
seems
like
this
whole
blob
query
needs
to
be
extracted
to
its
own
class
and
then,
when
we
extract
private
methods
with
it,
it's
not
getting
mixed
in
with
all
of
the
other
non-cohesive
stuff.
That's
in
this
class
and
then
yeah
more
just
all
these
little
parts
of
this
method,
pull
them
out
to
private.
C
A
A
D
C
D
D
D
B
D
D
A
D
A
B
C
A
B
D
A
D
B
A
D
A
A
D
A
D
D
A
C
A
D
A
So
we
have
in
here
this
is
not
a
method,
that's
not
a
method,
this
disabled!
This
is
private,
so
these
I
actually
wrote
not
all
of
these,
but
we've
started
to
make
these
checks,
because
this
line
is
so
long.
We'll
just
make
this
track
that
be
like
okay,
it's
now
a
private
method
that
we're
going
to
use
for
the
checks
that
we
have
to
use.
D
A
I
guess
that
makes
sense,
because
I
feel
like
it's
really
all
of
these
public
methods
aren't
going
to
change.
It's
all
these
private
methods
that
are
used
by
these
and
I
think
we
can't
move
all
of
them
because
these
ones
that
are
used
for
searching
commits
they
might
use
some
of
the
same
things.
But
I
I,
don't
I,
don't
know
and.
A
As
black
so
like
yeah,
we
have
this
concept
of
like
the
git
class
proxy,
which
handles
all
of
the
searching.
That's
done
for
things
that
are
backed
by
Italy.
So
for
us,
that's
wikis
and
blobs
and
commits,
and
those
search,
queries
that
are
built
up
for
elastic
are
all
handled
by
this
particular
class.
Proxy
module.
B
B
B
D
And
so
the
like,
when
the
remote
development
stuff,
like
the
the
refactoring,
we're
doing
to
the
sort
of
a
more
functional
way,
all
of
our
business
logic
is
in
class
methods
there.
It's
all
stateless
class
stuff.
So
not
saying
you
should
do
it
here,
but
it
does
make
it
easier
to
reason
about
this
stuff
because
you're
like
okay,
there
can
be
no
State,
because
everything
is
a
class
method.
So
therefore,
I
can
you
know
just
pass
around
whatever
I
need,
but
you
can
have
the
same
idea
here.
It
doesn't
have
to
be
a
class
method.
A
D
B
D
D
A
A
D
D
Let's
be
methodical
about
it
now
we
need
to
run
run
the
test
for
get
class
proxy
and
make
sure
we
didn't
break
anything
because
Ruby
mine's,
not
it
doesn't
have
any
red
checks
that
I
see
yeah
and
didn't
before
it
had
two
now
it
has
like
when
you
go
back
to
the
here,
there's
like
11
warnings.
Now
when
there
used
to
be
two,
what
are
they
if
you
hit
F2
to
go
through
them?
Okay,
parameter.
A
B
A
D
D
A
Oh,
you
want
to
test
this
thing.
I
will
tell
you
right
now.
I
know
this
is
not
the
only
test
that
tests
this
file,
but
we'll
see
what
happens
well.
This
is
the
one
that,
if
this
works,
I
would
be
more
confident.
I
just
know
like
this
has
the
way
the
tests
are
written
for
these
elastic
search.
Queries.
I've
I've
been
like
working
with
folks
to
see
if
we
to
like
try
to
like
chip
away
at
the
fact
that
the
testing
for
this
is
a
little
bit
lacking
when
it
comes
to
these
direct
proxy
classes.
D
A
Think
a
lot
of
ours
rely
on
integration
tests,
but
what
we've
tried
been
trying
to
add
is
these
are
essentially
creating
elastic
search,
queries
that
are
being
used
to
search
for
things.
I've
been
trying
to
like
add
these
assertions
to
make
sure
that
the
queries
contain
the
right
building
blocks,
based
on
like
what
you're
sending
into
the
method.
B
D
D
A
A
D
A
But
I
guess
what
what
I
feel
like
is
going
to
be
really
nice.
If
we
pull
this
particular
method
out
into
this
query
Builder,
then
we
can
do
all
of
the
testing
on
what's
essentially
like
a
Json
object
like
a
hash
that
it
has
the
right
things
and
we
don't
have
to
go
through
like
oh,
we
made
a
search
and
you
have
to
stand
elastic
up,
and
that
is
a
very
expensive.
D
Ci,
that
is
the
goal
of
pulling
this
out
and
like
you,
should
actually
architect
that
way
like
what
piece
of
logic
can
I
pull
out,
that
it's
interface
is
pure.
It's
pure
business
logic
and
its
interface
are
playing
data
structures.
Then
I
can
unit
test
that
and
then
I
rely
on.
The
mineral
of
the
pyramid
is
everything
wired
together
with
maybe
some
real
stuff
and
the
top
of
the
pyramid
like
when
I
post,
the
UI
to
okay,
the
other
stuff.
A
Maybe
I've
been
approaching
these
refractors
like
a
lot
I.
Guess
it's
just
starting
at
like
the
base
level,
which
is
pulling
things
into
methods,
so
at
least
I
can
read
the
method.
That's
like.
First,
we
do
this
step.
Then
we
do
this
step,
but
it
still
is
not
fixing
the
testability
of
it
still
not
easy.
D
C
C
A
C
D
A
A
D
A
A
D
A
A
D
We're
unraveling
the
sweaters,
so
you
you
can
hold
that
method
into
a
module.
If
you
want
like
a
helper
module
and
then
it
could
be
used
or
you
could
there's
other
things,
you
could
do
that
are
more
weird.
You
could
like
inject
it,
but
or
you
could
inline
the
logic.
How
complicated
is
the
logic
for
that.
B
A
B
D
A
D
Again,
I'm
not
suggesting
this,
but
like
the
nice
thing
about
you
having
your
the
way,
we're
going
with
the
remote
development
with
all
class
methods
for
the
domain
logic,
like
you
don't
have
this
inheritance
problem,
you
could
just
like
call
that
over
there
yeah
right,
like
you,
could
call
back
to
it
or
or
move
it
around.
No.
D
In
this
case,
what
you
can
do
to
solve
it
in
you
know
an
instance.
You
know
an
owl
Paradigm.
You
could
take
that
method.
That's
used,
as
well
as
the
method
on
the
superclass,
throw
them
in
a
module
somewhere
like
introduce
a
new
module
like
the
class
helper
and
then
Mitch
them
in
where
they're
needed
and.
C
D
C
D
D
B
D
A
D
A
C
C
A
D
A
D
Inclination
is
like
I'm,
I'm
kind
of
over
the
whole.
The
rails.
Paradigm
of
everything
is
a
mixing
and
there's
just
like
multiple
inheritance,
and
you
don't
know
where
anything
comes
from
and
that's
why
I'm
more
like
concrete
things
and
like
as
you
can
see
on
our
own
development,
like
Ashley,
makes
them
class
methods.
So
there's
no
State
whatsoever
and
everything's
explicit,
but
I
was
just
saying
that
yeah.
B
D
D
C
B
B
C
Just
yeah,
you
don't
don't
have
the
block,
you
don't
need
the
book
like
that
again.
My
idea
is
just
do
a
first
step
to
make
all
the
tests
work
and
then
we
see
all
the
problems
that
this
really
creates,
which
is
the
circle
of
dependency
and
all
the
dependencies
that
that
builder
needs,
and
that
will
be
very
explicit
and
then
we
can
extract
those
as
well
and
unreal
all
the
things
that
we
need
tomorrow,
I.
D
D
A
D
Going
in,
like
Casio
was
saying
like
use
that
same
approach,
and
then
it's
like
it's
explicit,
like
all
of
these
places,
where
you
have
this
couple
logic,
and
then
you
can
decide
what
to
do
for
each
of
those
they
can
turn
into
their
own
class.
That's
used.
They
can
turn
into
a
visual
whatever.
However,
you
want
to
move
that.
B
A
D
C
B
A
Okay,
I'm
just
gonna
leave
this
note
for
myself
Yep.
This
is
probably
something
that
I
might
bring
back
to
a
future
pairing
session
since
I
know,
we'll
only
have
an
hour
and
I
originally
was
like.
They
might
be
able
to
finish
this
because
I'm
very
positive
thinking
today
and
that's
not
gonna
happen.
I.
A
B
A
Think
what
the
the
next
step
for
this
is
to
make
sure
that
all
of
these
are
either
self-contained
and
I
feel
like
these
methods
that
are
I,
think
they're
public
right
now,
but
I
want
to
make
them
private.
These
I'm
really
okay,
with
moving
along
because
they're
really
just
made
for
use
in
this
method
and
they're
just
around
essentially
whether
some
background
migrations
have
finished
so.
A
A
C
A
C
C
Section,
maybe
what
we
should
do,
it's
like
just
check
what
is
including
that
that
module
the
git
class
proxy
module,
which
is
weird
because
it's
called
git
class,
but
it's
a
module
what's
including
that,
and
why
do
we
have
things
that
are
including
that
module?
But
it's
also
depending
on
methods
that
I
Rarity
inherited.
That
word,
it's
very
hard
for
me,
sorry
inherited
from
the
application
correct
proxy.
So
we
have
those
two
dependencies
and
one
dependence
depend
on
the
other.
A
D
D
A
C
A
A
D
D
If
you
pull
that
logic
up
to
the
higher
level
in
the
hierarchy,
like
you're
you're,
saying
that
everything
that
inherits
from
that
need,
that
gets
that
mixed
in
which
isn't
good
yeah
alternate
approaches
like
everything
that
is
at
that
top
level
of
the
hierarchy.
That
maybe
doesn't
need
to
be
there
or
is
used
from
other
places,
fold
that
out
to
modules
and
only
mix
it
in
when
it's
needed.
So
then
you're
not
pulling
that
logic
up
in
the.
A
C
A
D
A
I
think
I've
in
in
some
of
the
reviews,
I've
done
in
these
classes.
I
actually
made
comment
that,
like
in
terms
of
making
things
dry,
it
sometimes
makes
it
more
hard
to
read
in
the
case
of
like
this
particular
area
of
the
code
and
I
asked
like
maybe
it's
okay,
that
we
leave
things
duplicated
for
now,
because
otherwise
trying
to
trace
where
this
thing
lives.
You
end
up.
It's
very
difficult,
I.
D
A
D
Think
the
last
takeaway
here
is
like,
even
though
we
went
to
the
wrong
abstraction,
like
we've
refactoring
helps
you
understand
the
dependencies
like
you
wouldn't
have
realized
these.
If
you
hadn't
started
on
the
path
refractory,
so
it's
fine
that
it
was
throwaway.
You
understand
it
better.
Now,
yeah.