Conditional Code Smells

And not like roses. If/then/else is one of the first things a new programmer learns. But, like any tool, it can be overused. Too much conditional logic makes your methods hard to understand and reason about.

Conditionals can often indicate that we are missing important abstractions in our code. You may be familiar with using polymorphism to eliminate conditionals, but there are many other ways. This talk goes through a few of those ways and focuses on rewriting a piece of complicated, conditional logic to use no conditionals at all.

Materials mentioned:

Sandi Metz: All the Little Things - https://www.youtube.com/watch?v=8bZh5LMaSmE

Sandi Metz: Practical Object Oriented Design in Ruby - http://www.poodr.com/

Katrina Owen: Therapeutic Refactoring - https://www.youtube.com/watch?v=J4dlF0kcThQ

Avdi Grimm: Confident Ruby - http://www.confidentruby.com/­

Avdi Grimm: Confident Code (the talk that preceded the book) - https://www.youtube.com/watch?v=T8J0j2xJFgQ

Transcript

1. CONDITIONAL CODE Smells ERIC ROBERTS | @EROBERTS
2. WHAT IS CONDITIONAL LOGIC?
3. Conditional statements are features of a programming language which perform different computations or actions depending on whether a programmer-specified boolean condition evaluates to true or false. Apart from the case of branch predication, this is always achieved by selectively altering the control flow based on some condition. — Wikipedia
4. if thing puts 'hello' else puts 'goodbye' end
5. thing ? puts 'hello' : puts 'goodbye'
6. puts 'hello' if thing puts 'goodbye' unless thing
7. case thing when Hello puts 'hello' when Goodbye puts 'goodbye' else puts 'uh oh..' end
8. SO, WHY'S THIS BAD?
9. GOOD QUESTION
10. IT'S NOT
11. BUT IT CAN BE
12. WHAT ARE CODE SMELLS?
13. Code smells are not technically incorrect and do not prevent the program from functioning.
14. Instead, they indicate weaknesses in design that may be slowing down development or increasing the risk of bugs in the future. — Wikipedia
15. LET'S TAKE A LOOK
16. if thing puts 'hello' else puts 'goodbye' end
17. if thing puts 'hello' elsif other_thing puts 'auf wiedersehen' else puts 'goodbye' end
18. if thing puts 'hello' elsif other_thing puts 'auf wiedersehen' elsif another_thing puts 'bonjour' else puts 'goodbye' end
19. if thing puts 'hello' elsif other_thing if going puts 'auf wiedersehen' else puts 'hallo' end elsif another_thing puts 'bonjour' else puts 'goodbye' end
20. Well, this is just getting out of hand, isn't it?
21. def pricing_range_fold_and_save(new_range) pos = 0 pricings = self.pricings other_range = Pricing.new if pricings.length == 0 pricings.push(new_range) else pricings.each_with_index do |e, i| if i == 0 && new_range.start_date < e.start_date pricings.unshift(new_range) if new_range.end_date <= e.end_date e.start_date = new_range.end_date + 1.day break else while pricings[1].present? && new_range.end_date >= pricings[1].end_date pricings[1].destroy pricings.delete pricings[1] end if pricings[1].present? pricings[1].start_date = new_range.end_date + 1.day end break end elsif new_range.start_date <= e.end_date && new_range.start_date >= e.start_date if new_range.end_date < e.end_date other_range = e.dup other_range.start_date = new_range.end_date + 1.day if new_range.start_date == e.start_date e.destroy pricings.delete e i -= 1 else e.end_date = new_range.start_date - 1.day end pricings.insert(i+1, new_range) pos = i+1 pricings.insert(i+2, other_range) break else if new_range.start_date == e.start_date e.destroy pricings.delete e i -= 1 else e.end_date = new_range.start_date - 1.day end
22. pricings.insert(i+1, new_range) pos = i+1 while pricings[i+2].present? && new_range.end_date >= pricings[i+2].end_date pricings[i+2].destroy pricings.delete pricings[i+2] end if pricings[i+2].present? pricings[i+2].start_date = new_range.end_date + 1.day end break end elsif i == pricings.size-1 pricings[i].end_date = new_range.start_date-1.day pricings.push(new_range) break end end end pricings.each_with_index do |pricing, i| if i != pricings.size-1 && pricing.price ==pricings[i+1].price pricing.end_date = pricings[i+1].end_date end if i != 0 && pricing.end_date == pricings[i-1].end_date pricing.destroy pricings.delete pricing end if pricing.end_date < pricing.start_date pricing.destroy pricings.delete pricing end end pricings.each do |pricing| if pricing != pricings[pos] pricing.currency = pricings[pos].currency pricing.save end end pricings[pos].save return pricings end
23. REMEMBER CHOOSE YOUR OWN ADVENTURE BOOKS?
24. Conditional code can be like reading a choose your own adventure book from front to back.
25. This next bit is from Avdi Grimm's excellent book, Confident Ruby
44. SO, WE ALL KNOW WHAT'S GOING ON RIGHT?
45. OK, SO HOW DO I FIX IT?
46. COMMON INTERFACES
47. class Mailing belongs_to :owner, polymorphic: true def company end end
48. class Company has_many :campaigns has_many :mailings, as: :owner end class Campaign belongs_to :company has_many :mailings, as: :owner end
49. class Mailing belongs_to :owner, polymorphic: true def company if owner.is_a? Company owner else owner.company end end end
50. class Mailing belongs_to :owner, polymorphic: true def company owner.company end end
51. class Mailing belongs_to :owner, polymorphic: true delegate :company, to: :owner end
52. class Company has_many :campaigns has_many :mailings, as: :owner def company self end end
53. company = Company.new mailing = Mailing.new(owner: company) mailing.company campaign = Campaign.new(company: company) mailing = Mailing.new(owner: campaign) mailing.company
54. USE INHERITANCE Sandi Metz, ALL THE LITTLE THINGS
55. class GildedRose attr_reader :name, :quality, :days_remaining def initialize(name, quality, days_remaining) @name, @quality, @days_remaining = name, quality, days_remaining en def tick [...] end end
56. def tick days_remaining -= 1 if name == 'Aged Brie' quality -= 1 elsif name == 'Sulfuras, Hand of Ragnaros' quality += 1 end end
57. class GildedRose attr_reader :name, :quality, :days_remaining def initialize(name, quality, days_remaining) @name, @quality, @days_remaining = name, quality, days_remaining en def tick days_remaining -= 1 end end
58. class AgedBrie < GildedRose def tick super quality -= 1 end end
59. class Sulfuras < GildedRose def tick super quality += 1 end end
60. class GildedRose [...] def self.klass_for(name) { 'Aged Brie' => AgedBrie, 'Sulfuras, Hand of Ragnaros' => Sulfuras } end end
61. thing = GildedRose.klass_for('Aged Brie') thing.tick thing = GildedRose.klass_for('Sulfuras, Hand of Ragnaros') thing.tick
62. USE SPECIAL CASE OBJECTS
63. if current_user "Hello, #{current_user.name}" else "Hello, Guest" end
64. def user current_user || GuestUser.new end class GuestUser def name "Guest" end end
65. "Hello, #{user.name}"
66. USE HASH#FETCH Avdi Grimm, CONFIDENT RUBY
67. def create_user(options) login = options[:login] unless login raise ArgumentError, 'login must be supplied' end end
68. def create_user(options) login = options.fetch(:login) end
69. def create_user(options) login = options.fetch(:login, 'default_login') end
70. things = { foo: { bar: :baz } }
71. if things[:foo] things[:foo][:bar] end
72. things.fetch(:foo, { bar: nil })[:bar]
73. USE ENUMERABLE
74. This one's going to take a little longer...
75. def pricing_range_fold_and_save(new_range) pos = 0 pricings = self.pricings other_range = Pricing.new if pricings.length == 0 pricings.push(new_range) else pricings.each_with_index do |e, i| if i == 0 && new_range.start_date < e.start_date pricings.unshift(new_range) if new_range.end_date <= e.end_date e.start_date = new_range.end_date + 1.day break else while pricings[1].present? && new_range.end_date >= pricings[1].end_date pricings[1].destroy pricings.delete pricings[1] end if pricings[1].present? pricings[1].start_date = new_range.end_date + 1.day end break end elsif new_range.start_date <= e.end_date && new_range.start_date >= e.start_date if new_range.end_date < e.end_date other_range = e.dup other_range.start_date = new_range.end_date + 1.day if new_range.start_date == e.start_date e.destroy pricings.delete e i -= 1 else e.end_date = new_range.start_date - 1.day end pricings.insert(i+1, new_range) pos = i+1 pricings.insert(i+2, other_range) break else if new_range.start_date == e.start_date e.destroy pricings.delete e i -= 1 else e.end_date = new_range.start_date - 1.day end
76. ADDING A NEW PRICING SCREWED UP THE REST OF MY YEAR
77. Time to start reading...
78. def pricing_range_fold_and_save(new_range) pos = 0 pricings = self.pricings other_range = Pricing.new if pricings.length == 0 pricings.push(new_range) else
79. pricings.each_with_index do |e, i| if i == 0 && new_range.start_date < e.start_date pricings.unshift(new_range) if new_range.end_date <= e.end_date e.start_date = new_range.end_date + 1.day break else while pricings[1].present? && new_range.end_date >= pricings[1].end_date pricings[1].destroy pricings.delete pricings[1] end if pricings[1].present? pricings[1].start_date = new_range.end_date + 1.day end break end
80. OK, enough of that. What do the tests say?
81. require 'spec_helper' describe Property do pending "add some examples to (or delete) #{__FILE__}" end
82. Alrighty then...
83. SO, WHAT IS THIS SUPPOSED TO DO?
86. MAGIC
87. 1 - If the new price completely covers an old price, delete the old price.
88. 2 - Adjust overlapping pricings.
89. 3 - Split prices that fully contain the new price.
90. 4 - Expand the range to meet the new price.
91. 5 - Combine pricings where the pricings match.
92. class PricingRange attr_reader :property def initialize(property) @property = property end def add(pricing) delete_complete_overlaps(pricing) adjust_overlaps(pricing) split_containing(pricing) expand_to_meet(pricing) coalesce_pricings end end
93. class PricingRange [...] def add(pricing) delete_complete_overlaps(pricing) [...] end end
94. DELETE COMPLETE OVERLAPS
95. def delete_complete_overlaps(new) pricings.each do |old| if new.start_date <= old.start_date && new.end_date >= old.end_date pricings.delete old end end end
96. def delete_complete_overlaps(new) to_delete = pricings.select do |old| new.start_date <= old.start_date && new.end_date >= old.end_date end pricings.delete to_delete end
97. new.start_date <= old.start_date && new.end_date >= old.end_date
98. class PricingRange [...] def contains?(pricing1, pricing2) pricing1.start_date <= pricing2.start_date && pricing1.end_date >= pricing2.end_date end end
99. class Pricing [...] def contains?(other) self.start_date <= other.start_date && self.end_date >= other.end_date end end
100. def delete_complete_overlaps(new) to_delete = pricings.select { |old| new.contains? old } pricings.delete to_delete end
101. def delete_complete_overlaps(new) pricings.each do |old| if new.start_date <= old.start_date && new.end_date >= old.end_date pricings.delete old end end end
102. def delete_complete_overlaps(new) to_delete = pricings.select { |old| new.contains? old } pricings.delete to_delete end
103. ADJUST OVERLAPS
104. class PricingRange [...] def adjust_overlaps(new) adjust_overlaps_of_start(new) adjust_overlaps_of_end(new) end end
105. OVERLAPPING START
106. def adjust_overlaps_of_start(new) pricings.each do |old| if new.start_date <= old.start_date && new.end_date >= old.start_date old.start_date = new.end_date + 1.day old.save! end end end
107. def adjust_overlaps_of_start(new) to_adjust = pricings.select do |old| new.start_date <= old.start_date && new.end_date >= old.start_date end to_adjust.each do |pricing| pricing.start_date = new.start_date + 1.day pricing.save! end end
108. class Pricing [...] def overlaps_start_of?(other) self.start_date <= other.start_date && self.end_date >= other.start_date end end
109. def adjust_overlaps_of_start(new) to_adjust = pricings.select { |old| new.overlaps_start_of? old } to_adjust.each do |pricing| pricing.start_date = new.start_date + 1.day pricing.save! end end
110. def adjust_overlaps_of_start(new) overlapped = pricings.find { |old| new.overlaps_start_of? old } overlapped.start_date = new.start_date + 1.day overlapped.save! end
111. def adjust_overlaps_of_start(new) overlapped = pricings.find { |old| new.overlaps_start_of? old } return unless overlapped overlapped.start_date = new.start_date + 1.day overlapped.save! end
112. def adjust_overlaps_of_start(new) to_adjust = pricings.select { |old| new.overlaps_start_of? old } to_adjust.each do |pricing| pricing.start_date = new.start_date + 1.day pricing.save! end end
113. OVERLAPPING END
114. def adjust_overlaps_of_end(new) pricings.each do |old| if new.start_date <= old.end_date && new.end_date >= old.end_date old.end_date = new.start_date - 1.day old.save! end end end
115. def adjust_overlaps_of_end(new) to_adjust = pricings.select { |old| new.overlaps_end_of? old } to_adjust.each do |pricing| pricing.end_date = new.start_date - 1.day pricing.save! end end
116. class PricingRange [...] def add(pricing) delete_complete_overlaps(pricing) adjust_overlaps(pricing) split_containing(pricing) [...] end end
117. SPLIT CONTAINING
118. def split_containing(new) pricings.each do |old| if old.start_date <= new.start_date && old.end_date >= new.end_date old_end = old.end_date old.end_date = new.start_date - 1.day old.save! other_side = old.dup other_side.start_date = new.end_date + 1.day other_side.end_date = old_end pricings << other_side end end end
119. def split_containing(new) to_split = pricings.select { |old| old.contains? new } to_split.each do |left_side| right_side_end = left_side.end_date left_side.end_date = new.start_date - 1.day left_side.save! pricings << build_right_side(left_side, new.end_date + 1.day, right_side_end) end end
120. def build_right_side(left_side, middle, start_date, end_date) right_side = left_side.dup right_side.start_date = start_date right_side.end_date = end_date right_side end
121. class PricingRange [...] def add(pricing) delete_complete_overlaps(pricing) adjust_overlaps(pricing) split_containing(pricing) expand_to_meet(pricing) [...] end end
122. EXPAND TO MEET
123. def expand_to_meet(new) expand_to_meet_before(new) expand_to_meet_after(new) end
124. def expand_to_meet_before(new) pricings.each do |old| if old == pricings.order('start_date ASC').first && old.start_date > new.end_date old.start_date = new.end_date + 1.day old.save! end end end
125. def expand_to_meet_before(new) first_pricing_after_new = pricings.select do |old| old == pricings.order('start_date ASC').first && old.start_date > new.end_date end first_pricing_after_new.each do |old| old.start_date = new.end_date + 1.day old.save! end end
126. def first_pricing pricings.order('start_date ASC').first end
127. class Pricing [...] def is_after?(other) self.start_date > other.end_date end end
128. def expand_to_meet_before(new) first_pricing_after_new = pricings.select do |old| old == first_pricing && old.is_after?(new) end first_pricing_after_new.each do |old| old.start_date = new.end_date + 1.day old.save! end end
129. def expand_to_meet_after(new) [...] end
130. I think you probably get it...
131. class PricingRange [...] def add(pricing) delete_complete_overlaps(pricing) adjust_overlaps(pricing) split_containing(pricing) expand_to_meet(pricing) coalesce_pricings end end
132. COALESCE PRICINGS
133. def coalesce_pricings pricings.each do |pricing| previous = pricings.where(end_date: pricing.start_date - 1.day).first if previous && pricing.price == previous.price pricing.start_date = previous.start_date previous.destroy pricing.save! end end end
134. def coalesce_pricings delete_before = pricings.select do |pricing| previous = pricings.where(end_date: pricing.start_date - 1.day).first previous && pricing.price == previous.price end delete_before.each do |pricing| previous = pricings.where(end_date: pricing.start_date - 1.day).first pricing.start_date = previous.start_date previous.destroy pricing.save! end end
135. def previous_pricing_for(pricing) pricings.where(end_date: pricing.start_date - 1.day).first end
136. def coalesce_pricings delete_before = pricings.select do |pricing| previous = previous_pricing_for(pricing) previous && pricing.price == previous.price end delete_before.each do |pricing| previous = previous_pricing_for(pricing) pricing.start_date = previous.start_date previous.destroy pricing.save! end end
137. class PricingRange [...] def add(pricing) delete_complete_overlaps(pricing) adjust_overlaps(pricing) split_containing(pricing) expand_to_meet(pricing) coalesce_pricings end end
138. class PricingRange [...] def add(pricing) delete_complete_overlaps(pricing) adjust_overlaps(pricing) split_containing(pricing) expand_to_meet(pricing) pricings << pricing coalesce_pricings pricings end end
139. DONE!
140. WRAP-UP
141. WRITE TESTS
142. Ugly, conditional code, is made more beautiful by tests.
143. DON'T EVER USE UNLESS/ELSE
144. unless thing do_something else do_something_else end
145. IF YOU READ THAT AND IT MADE SENSE, YOU ARE A ROBOT.
146. WE'RE ALL STILL GOING TO USE CONDITIONALS.
147. IT'S OK.
148. WHAT'S IMPORTANT IS THAT YOU FEEL BAD ABOUT IT.
149. That's all, folks!
150. Oh, and you should probably follow me on Twitter @eroberts
151. THINGS YOU SHOULD CHECK OUT FOR MORE INFORMATION
152. ▸ Confident Ruby by Avdi Grimm ▸ All the Little Things by Sandi Metz ▸ Practical Object Oriented Design in Ruby by Sandi Metz ▸ Therapeutic Refactoring by Katrina Owen