Piotr Szotkowski about "Ruby smells"
-
Upload
pivorak-meetup -
Category
Software
-
view
176 -
download
1
Transcript of Piotr Szotkowski about "Ruby smells"
Ruby Smells
Pivorak Edition
code example caveatsinstance state denoted via @instance_variables
assume those instance variables areset in constructors | dependency injected
(constructors skipped for simplicity)
(yes, I would use getters in actual code)
(yes, some examples are very badly refactored)
c l a s s M e e t u p d e f w e a t h e r _ a s s e s s m e n t i f @ w e a t h e r _ s o u r c e . w e a t h e r _ a t ( @ c i t y ) . s k i e s = = : c l e a r ' T o o s u n n y ' e l s i f @ w e a t h e r _ s o u r c e . w e a t h e r _ a t ( @ c i t y ) . t e m p > 2 0 ' T o o h o t ' e l s e ' P e r f e c t f o r c o d i n g ' e n d e n d e n d
duplicate method callthe same call made more than once
refactoring: factor out a single call
caveat: readability
c l a s s M e e t u p d e f w e a t h e r _ a s s e s s m e n t w e a t h e r = @ w e a t h e r _ s o u r c e . w e a t h e r _ a t ( @ c i t y ) i f w e a t h e r . s k i e s = = : c l e a r ' T o o s u n n y ' e l s i f w e a t h e r . t e m p > 2 0 ' T o o h o t ' e l s e ' P e r f e c t f o r c o d i n g ' e n d e n d e n d
m o d u l e A r t D e c o m p c l a s s S e p s d e f s e p a r a t e s ? ( r o w , c o l ) m a t r i x [ r o w ] a n d m a t r i x [ r o w ] [ c o l ] . n o n z e r o ? e n d e n d e n d
m o d u l e A r t D e c o m p c l a s s A r c h S i z e r d e f m i n _ q u a r t e r s c a s e w h e n i . z e r o ? , o . z e r o ? t h e n 0 w h e n i < = 5 t h e n ( o / 2 . 0 ) . c e i l e l s e [ ( i / 5 . 0 ) . c e i l , ( o / 2 . 0 ) . c e i l ] . m a x e n d e n d e n d e n d
c l a s s M e e t u p d e f d i r e c t i o n s ( s o u r c e _ l a t , s o u r c e _ l o n ) @ n a v _ s o u r c e . d i r e c t i o n s ( f r o m : [ s o u r c e _ l a t , s o u r c e _ l o n ] , t o : [ @ l a t , @ l o n ] ) e n d d e f d i s t a n c e ( s o u r c e _ l a t , s o u r c e _ l o n ) @ n a v _ s o u r c e . d i s t a n c e ( f r o m : [ s o u r c e _ l a t , s o u r c e _ l o n ] , t o : [ @ l a t , @ l o n ] ) e n d d e f n e e d s _ p a s s p o r t ? ( s o u r c e _ l a t , s o u r c e _ l o n ) @ n a v _ s o u r c e . b o r d e r s ? ( f r o m : [ s o u r c e _ l a t , s o u r c e _ l o n ] , t o : [ @ l a t , @ l o n ] ) e n d e n d
data clumpthe same set of variables passed in many contexts
refactoring: create a composite (value?) object
caveat: make sure the clumped variables are related
L o c a t i o n = S t r u c t . n e w ( : l a t , : l o n ) c l a s s M e e t u p d e f d i r e c t i o n s ( s o u r c e _ l o c a t i o n ) @ n a v _ s o u r c e . d i r e c t i o n s ( f r o m : s o u r c e _ l o c a t i o n , t o : @ l o c a t i o n ) e n d d e f d i s t a n c e ( s o u r c e _ l o c a t i o n ) @ n a v _ s o u r c e . d i s t a n c e ( f r o m : s o u r c e _ l o c a t i o n , t o : @ l o c a t i o n ) e n d d e f n e e d s _ p a s s p o r t ? ( s o u r c e _ l o c a t i o n ) @ n a v _ s o u r c e . b o r d e r s ? ( f r o m : s o u r c e _ l o c a t i o n , t o : @ l o c a t i o n ) e n d e n d
c l a s s M e e t u p d e f d i r e c t i o n s ( s o u r c e _ l o c a t i o n ) i f @ l o c a t i o n @ n a v _ s o u r c e . d i r e c t i o n s ( f r o m : s o u r c e _ l o c a t i o n , t o : @ l o c a t i o n ) e n d e n d d e f d i s t a n c e ( s o u r c e _ l o c a t i o n ) i f @ l o c a t i o n @ n a v _ s o u r c e . d i s t a n c e ( f r o m : s o u r c e _ l o c a t i o n , t o : @ l o c a t i o n ) e n d e n d d e f n e e d s _ p a s s p o r t ? ( s o u r c e _ l o c a t i o n ) i f @ l o c a t i o n @ n a v _ s o u r c e . b o r d e r s ? ( f r o m : s o u r c e _ l o c a t i o n , t o : @ l o c a t i o n ) e n d e n d e n d
repeated conditionalthe same condition is checked in many places
refactoring: factor out LocatedMeetup
refactoring: make NavSource work with NullLocation
caveat: sometimes readability trumps purer design
c l a s s M e e t u p d e f i n i t i a l i z e ( n a m e = n i l ) @ n a m e = n a m e e n d d e f a c r o n y m ? ! ! @ n a m e . m a t c h ( / \ A ( \ p { U p p e r } ) + \ z / ) u n l e s s @ n a m e . n i l ? e n d e n d
nil checkusually means there’s a missing type in the system
refactoring: find the missing type (a null object?)
caveat: make sure you don’t sacrifice simplicity
c l a s s U n n a m e d M e e t u p d e f a c r o n y m ? f a l s e # o r i s i t ? : ) e n d e n d c l a s s M e e t u p d e f s e l f . n e w ( n a m e = n i l ) n a m e ? s u p e r : U n n a m e d M e e t u p . n e w e n d d e f i n i t i a l i z e ( n a m e ) @ n a m e = n a m e e n d d e f a c r o n y m ? ! ! @ n a m e . m a t c h ( / \ A ( \ p { U p p e r } ) + \ z / ) e n d e n d
c l a s s M e e t u p d e f r u b y ? ( s t r i c t = t r u e ) i f s t r i c t @ l a n g u a g e s = [ ' R u b y ' ] e l s e @ l a n g u a g e s . i n c l u d e ? ( ' R u b y ' ) e n d e n d e n d
boolean parameterthe caller directly controls calee’s code execution
refactoring: split into dedicated methods (classes?)
caveat: consider caller usability
c l a s s M e e t u p d e f s u r e l y _ r u b y ? @ l a n g u a g e s = [ ' R u b y ' ] e n d d e f p o s s i b l y _ r u b y ? @ l a n g u a g e s . i n c l u d e ? ( ' R u b y ' ) e n d e n d
c l a s s M e e t u p d e f i n t e r e s t i n g ? ( a t t e n d e e ) c a s e a t t e n d e e w h e n ' p y t h o n i s t a ' t h e n d y n a m i c ? w h e n ' r u b y i s t ' t h e n l a i d _ b a c k ? w h e n ' r u s t a c e a n ' t h e n s y s t e m ? e n d e n d e n d
control parameterthe caller knows which path the code will take
refactoring: split into dedicated methods
caveat: consider caller usability (more likely)
c l a s s M e e t u p d e f i n t e r e s t i n g _ t o _ p y t h o n i s t a s ? d y n a m i c ? e n d d e f i n t e r e s t i n g _ t o _ r u b y i s t s ? l a i d _ b a c k ? e n d d e f i n t e r e s t i n g _ t o _ r u s t a c e a n s ? s y s t e m ? e n d e n d
c l a s s M e e t u p d e f s u c c e s s f u l ? ( a t t e n d e e s ) e x c i t e d , i n d i f f e r e n t = a t t e n d e e s . p a r t i t i o n ( & : e x c i t e d ? ) e x c i t e d . c o u n t > i n d i f f e r e n t . c o u n t e n d e n d
utility functiona method that does not depend on instance state
can be moved anywhere in the system
refactoring: move it to where it belongs
caveat: sometimes it’s just a (private?) helper
c l a s s M e e t u p d e f s u c c e s s f u l ? e x c i t e d , i n d i f f e r e n t = @ a t t e n d e e s . p a r t i t i o n ( & : e x c i t e d ? ) e x c i t e d . c o u n t > i n d i f f e r e n t . c o u n t e n d e n d
c l a s s M e e t u p d e f s u c c e s s f u l ? @ a t t e n d e e s . e x c i t e d ? e n d e n d
c l a s s A t t e n d e e s < A r r a y d e f e x c i t e d ? e x c i t e d , i n d i f f e r e n t = p a r t i t i o n ( & : e x c i t e d ? ) e x c i t e d . c o u n t > i n d i f f e r e n t . c o u n t e n d e n d
r e q u i r e ' f o r w a r d a b l e ' c l a s s A t t e n d e e s e x t e n d F o r w a r d a b l e d e f i n i t i a l i z e ( c o l l e c t i o n ) @ c o l l e c t i o n = c o l l e c t i o n e n d d e f e x c i t e d ? e x c i t e d , i n d i f f e r e n t = p a r t i t i o n ( & : e x c i t e d ? ) e x c i t e d . c o u n t > i n d i f f e r e n t . c o u n t e n d d e l e g a t e p a r t i t i o n : : @ c o l l e c t i o n e n d
c l a s s M e e t u p d e f s u c c e s s f u l ? ( a t t e n d e e s ) e x c i t e d , i n d i f f e r e n t = a t t e n d e e s . p a r t i t i o n d o | a t t e n d e e | a t t e n d e e . e x c i t e d ? | | @ l a n g u a g e = = ' R u b y ' & & a t t e n d e e . r u b y i s t ? e n d e x c i t e d . c o u n t > i n d i f f e r e n t . c o u n t e n d e n d
feature envypartially refers to instance, but mostly to arguments
refactoring: move code to one of the arguments
caveat: make sure the knowledge is local to theproblem
c l a s s M e e t u p d e f s u c c e s s f u l ? @ a t t e n d e e s . e x c i t e d ? ( l a n g u a g e : @ l a n g u a g e ) e n d e n d
c l a s s A t t e n d e e s d e f e x c i t e d ? ( l a n g u a g e : n i l ) e x c i t e d , i n d i f f e r e n t = p a r t i t i o n { | a t t e n d e e | a t t e n d e e . e x c i t e d ? ( l a n g u a g e : l a n g u a g e ) } e x c i t e d . c o u n t > i n d i f f e r e n t . c o u n t e n d e n d
c l a s s A t t e n d e e d e f e x c i t e d ? ( l a n g u a g e : n i l ) @ e x c i t e d | | @ r u b y i s t & & l a n g u a g e = = ' R u b y ' e n d e n d
c l a s s M e e t u p d e f i n i t i a l i z e ( c i t y : , n a m e : ) @ c i t y = c i t y @ n a m e = n a m e e n d a t t r _ a c c e s s o r : c i t y , : n a m e e n d
attributesetters: bad
getters: it depends
refactoring: tell, don’t ask
refactoring: immutability via #update (or #with)
caveat: an overkill in simplest (riiight…) projects
c l a s s M e e t u p d e f i n i t i a l i z e ( c i t y : , n a m e : ) @ c i t y = c i t y @ n a m e = n a m e e n d a t t r _ r e a d e r : c i t y , : n a m e d e f u p d a t e ( c i t y : @ c i t y , n a m e : @ n a m e ) s e l f . c l a s s . n e w ( c i t y : c i t y , n a m e : n a m e ) e n d e n d
real world examples
(433 lines)
(759 lines)
(1595 lines)
Matrix::EigenvalueDecomposition#hessenberg_to_real_schur
TkItemConfigMethod#__itemconfiginfo_core
RDoc::Markdown#_Code
so… how can we find codesmells in our Ruby apps?
by hand
using Reek
very opinionated
static code analyser
for finding smells
RuboCop for architecture
Reek usage$ g e m i n s t a l l r e e k $ r e e k - - n o - w i k i - l i n k s m e e t u p . r b m e e t u p . r b - - 1 0 w a r n i n g s : [ 4 0 , 4 2 ] : D u p l i c a t e M e t h o d C a l l : M e e t u p # w e a t h e r _ a s s e s s m e n t c a l l s @ w e a t h e r _ s o u r c e . w e a t h e r _ a t ( @ c i t y ) 2 t i m e s [ 2 1 , 2 7 , 3 3 ] : D a t a C l u m p : M e e t u p t a k e s p a r a m e t e r s [ s o u r c e _ l a t , s o u r c e _ l o n ] t o 3 m e t h o d s [ 2 2 , 2 8 , 3 4 ] : R e p e a t e d C o n d i t i o n a l : M e e t u p t e s t s @ l a t & & @ l o n a t l e a s t 3 t i m e s [ 5 7 ] : N i l C h e c k : M e e t u p # a c r o n y m ? p e r f o r m s a n i l - c h e c k [ 6 ] : C o n t r o l P a r a m e t e r : M e e t u p # i n t e r e s t i n g ? i s c o n t r o l l e d b y a r g u m e n t a t t e n d e e [ 1 3 ] : B o o l e a n P a r a m e t e r : M e e t u p # r u b y ? h a s b o o l e a n p a r a m e t e r ' s t r i c t ' [ 1 4 ] : C o n t r o l P a r a m e t e r : M e e t u p # r u b y ? i s c o n t r o l l e d b y a r g u m e n t s t r i c t [ 5 1 , 5 1 ] : F e a t u r e E n v y : M e e t u p # s u c c e s s f u l ? r e f e r s t o a t t e n d e e m o r e t h a n s e l f ( m a y b e m o v e i t t o a n o t h e r c l a s s ? ) [ 2 ] : A t t r i b u t e : M e e t u p # c i t y i s a w r i t a b l e a t t r i b u t e [ 1 ] : I r r e s p o n s i b l e M o d u l e : M e e t u p h a s n o d e s c r i p t i v e c o m m e n t
configuration: .reekD a t a C l u m p : m a x _ c o p i e s : 2 m i n _ c l u m p _ s i z e : 2 L o n g P a r a m e t e r L i s t : m a x _ p a r a m s : 3 o v e r r i d e s : i n i t i a l i z e : m a x _ p a r a m s : 5 T o o M a n y I n s t a n c e V a r i a b l e s : m a x _ i n s t a n c e _ v a r i a b l e s : 4 T o o M a n y S t a t e m e n t s : e x c l u d e : - i n i t i a l i z e m a x _ s t a t e m e n t s : 5
excludes: from Class#method to /meth/
configuration: code comments # : r e e k : U t i l i t y F u n c t i o n d e f s u c c e s s f u l ? ( a t t e n d e e s ) e x c i t e d , i n d i f f e r e n t = a t t e n d e e s . p a r t i t i o n ( & : e x c i t e d ? ) e x c i t e d . c o u n t > i n d i f f e r e n t . c o u n t e n d
# : r e e k : D u p l i c a t e M e t h o d C a l l : { m a x _ c a l l s : 2 } d e f w e a t h e r _ a s s e s s m e n t i f @ w e a t h e r _ s o u r c e . w e a t h e r _ a t ( @ c i t y ) . s k i e s = = : c l e a r ' T o o s u n n y ' e l s i f @ w e a t h e r _ s o u r c e . w e a t h e r _ a t ( @ c i t y ) . t e m p > 2 0 ' T o o h o t ' e l s e ' P e r f e c t f o r c o d i n g ' e n d e n d
perdirectory configuration
caveat emptormaking Reek happy with my codetaught me the most about OOP
in the last year
but
always be critical of such tools
Piotr Solnica, Clean Code Cowboy
Piotr Solnica, Clean Code Cowboy
world domination planstry out
maybe add it to ?
don’t be overwhelmed by the potential smellsreek --todo might be a good start!
please let us know what doesn’t work
have fun withrefactoring | enlightened whitelisting
Reek
Code Climate
thanks!Piotr Szotkowski
@chastell
talks.chastell.net