Slide 1

Slide 1 text

CONDITIONAL CODE Smells ERIC ROBERTS | @EROBERTS

Slide 2

Slide 2 text

WHAT IS CONDITIONAL LOGIC?

Slide 3

Slide 3 text

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

Slide 4

Slide 4 text

if thing puts 'hello' else puts 'goodbye' end

Slide 5

Slide 5 text

thing ? puts 'hello' : puts 'goodbye'

Slide 6

Slide 6 text

puts 'hello' if thing puts 'goodbye' unless thing

Slide 7

Slide 7 text

case thing when Hello puts 'hello' when Goodbye puts 'goodbye' else puts 'uh oh..' end

Slide 8

Slide 8 text

SO, WHY'S THIS BAD?

Slide 9

Slide 9 text

GOOD QUESTION

Slide 10

Slide 10 text

IT'S NOT

Slide 11

Slide 11 text

BUT IT CAN BE

Slide 12

Slide 12 text

WHAT ARE CODE SMELLS?

Slide 13

Slide 13 text

Code smells are not technically incorrect and do not prevent the program from functioning.

Slide 14

Slide 14 text

Instead, they indicate weaknesses in design that may be slowing down development or increasing the risk of bugs in the future. — Wikipedia

Slide 15

Slide 15 text

LET'S TAKE A LOOK

Slide 16

Slide 16 text

if thing puts 'hello' else puts 'goodbye' end

Slide 17

Slide 17 text

if thing puts 'hello' elsif other_thing puts 'auf wiedersehen' else puts 'goodbye' end

Slide 18

Slide 18 text

if thing puts 'hello' elsif other_thing puts 'auf wiedersehen' elsif another_thing puts 'bonjour' else puts 'goodbye' end

Slide 19

Slide 19 text

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

Slide 20

Slide 20 text

Well, this is just getting out of hand, isn't it?

Slide 21

Slide 21 text

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

Slide 22

Slide 22 text

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

Slide 23

Slide 23 text

REMEMBER CHOOSE YOUR OWN ADVENTURE BOOKS?

Slide 24

Slide 24 text

Conditional code can be like reading a choose your own adventure book from front to back.

Slide 25

Slide 25 text

This next bit is from Avdi Grimm's excellent book, Confident Ruby

Slide 26

Slide 26 text

No content

Slide 27

Slide 27 text

No content

Slide 28

Slide 28 text

No content

Slide 29

Slide 29 text

No content

Slide 30

Slide 30 text

No content

Slide 31

Slide 31 text

No content

Slide 32

Slide 32 text

No content

Slide 33

Slide 33 text

No content

Slide 34

Slide 34 text

No content

Slide 35

Slide 35 text

No content

Slide 36

Slide 36 text

No content

Slide 37

Slide 37 text

No content

Slide 38

Slide 38 text

No content

Slide 39

Slide 39 text

No content

Slide 40

Slide 40 text

No content

Slide 41

Slide 41 text

No content

Slide 42

Slide 42 text

No content

Slide 43

Slide 43 text

No content

Slide 44

Slide 44 text

SO, WE ALL KNOW WHAT'S GOING ON RIGHT?

Slide 45

Slide 45 text

OK, SO HOW DO I FIX IT?

Slide 46

Slide 46 text

COMMON INTERFACES

Slide 47

Slide 47 text

class Mailing belongs_to :owner, polymorphic: true def company end end

Slide 48

Slide 48 text

class Company has_many :campaigns has_many :mailings, as: :owner end class Campaign belongs_to :company has_many :mailings, as: :owner end

Slide 49

Slide 49 text

class Mailing belongs_to :owner, polymorphic: true def company if owner.is_a? Company owner else owner.company end end end

Slide 50

Slide 50 text

class Mailing belongs_to :owner, polymorphic: true def company owner.company end end

Slide 51

Slide 51 text

class Mailing belongs_to :owner, polymorphic: true delegate :company, to: :owner end

Slide 52

Slide 52 text

class Company has_many :campaigns has_many :mailings, as: :owner def company self end end

Slide 53

Slide 53 text

company = Company.new mailing = Mailing.new(owner: company) mailing.company campaign = Campaign.new(company: company) mailing = Mailing.new(owner: campaign) mailing.company

Slide 54

Slide 54 text

USE INHERITANCE Sandi Metz, ALL THE LITTLE THINGS

Slide 55

Slide 55 text

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

Slide 56

Slide 56 text

def tick days_remaining -= 1 if name == 'Aged Brie' quality -= 1 elsif name == 'Sulfuras, Hand of Ragnaros' quality += 1 end end

Slide 57

Slide 57 text

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

Slide 58

Slide 58 text

class AgedBrie < GildedRose def tick super quality -= 1 end end

Slide 59

Slide 59 text

class Sulfuras < GildedRose def tick super quality += 1 end end

Slide 60

Slide 60 text

class GildedRose [...] def self.klass_for(name) { 'Aged Brie' => AgedBrie, 'Sulfuras, Hand of Ragnaros' => Sulfuras } end end

Slide 61

Slide 61 text

thing = GildedRose.klass_for('Aged Brie') thing.tick thing = GildedRose.klass_for('Sulfuras, Hand of Ragnaros') thing.tick

Slide 62

Slide 62 text

USE SPECIAL CASE OBJECTS

Slide 63

Slide 63 text

if current_user "Hello, #{current_user.name}" else "Hello, Guest" end

Slide 64

Slide 64 text

def user current_user || GuestUser.new end class GuestUser def name "Guest" end end

Slide 65

Slide 65 text

"Hello, #{user.name}"

Slide 66

Slide 66 text

USE HASH#FETCH Avdi Grimm, CONFIDENT RUBY

Slide 67

Slide 67 text

def create_user(options) login = options[:login] unless login raise ArgumentError, 'login must be supplied' end end

Slide 68

Slide 68 text

def create_user(options) login = options.fetch(:login) end

Slide 69

Slide 69 text

def create_user(options) login = options.fetch(:login, 'default_login') end

Slide 70

Slide 70 text

things = { foo: { bar: :baz } }

Slide 71

Slide 71 text

if things[:foo] things[:foo][:bar] end

Slide 72

Slide 72 text

things.fetch(:foo, { bar: nil })[:bar]

Slide 73

Slide 73 text

USE ENUMERABLE

Slide 74

Slide 74 text

This one's going to take a little longer...

Slide 75

Slide 75 text

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

Slide 76

Slide 76 text

ADDING A NEW PRICING SCREWED UP THE REST OF MY YEAR

Slide 77

Slide 77 text

Time to start reading...

Slide 78

Slide 78 text

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

Slide 79

Slide 79 text

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

Slide 80

Slide 80 text

OK, enough of that. What do the tests say?

Slide 81

Slide 81 text

require 'spec_helper' describe Property do pending "add some examples to (or delete) #{__FILE__}" end

Slide 82

Slide 82 text

Alrighty then...

Slide 83

Slide 83 text

SO, WHAT IS THIS SUPPOSED TO DO?

Slide 84

Slide 84 text

No content

Slide 85

Slide 85 text

No content

Slide 86

Slide 86 text

MAGIC

Slide 87

Slide 87 text

1 - If the new price completely covers an old price, delete the old price.

Slide 88

Slide 88 text

2 - Adjust overlapping pricings.

Slide 89

Slide 89 text

3 - Split prices that fully contain the new price.

Slide 90

Slide 90 text

4 - Expand the range to meet the new price.

Slide 91

Slide 91 text

5 - Combine pricings where the pricings match.

Slide 92

Slide 92 text

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

Slide 93

Slide 93 text

class PricingRange [...] def add(pricing) delete_complete_overlaps(pricing) [...] end end

Slide 94

Slide 94 text

DELETE COMPLETE OVERLAPS

Slide 95

Slide 95 text

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

Slide 96

Slide 96 text

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

Slide 97

Slide 97 text

new.start_date <= old.start_date && new.end_date >= old.end_date

Slide 98

Slide 98 text

class PricingRange [...] def contains?(pricing1, pricing2) pricing1.start_date <= pricing2.start_date && pricing1.end_date >= pricing2.end_date end end

Slide 99

Slide 99 text

class Pricing [...] def contains?(other) self.start_date <= other.start_date && self.end_date >= other.end_date end end

Slide 100

Slide 100 text

def delete_complete_overlaps(new) to_delete = pricings.select { |old| new.contains? old } pricings.delete to_delete end

Slide 101

Slide 101 text

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

Slide 102

Slide 102 text

def delete_complete_overlaps(new) to_delete = pricings.select { |old| new.contains? old } pricings.delete to_delete end

Slide 103

Slide 103 text

ADJUST OVERLAPS

Slide 104

Slide 104 text

class PricingRange [...] def adjust_overlaps(new) adjust_overlaps_of_start(new) adjust_overlaps_of_end(new) end end

Slide 105

Slide 105 text

OVERLAPPING START

Slide 106

Slide 106 text

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

Slide 107

Slide 107 text

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

Slide 108

Slide 108 text

class Pricing [...] def overlaps_start_of?(other) self.start_date <= other.start_date && self.end_date >= other.start_date end end

Slide 109

Slide 109 text

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

Slide 110

Slide 110 text

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

Slide 111

Slide 111 text

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

Slide 112

Slide 112 text

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

Slide 113

Slide 113 text

OVERLAPPING END

Slide 114

Slide 114 text

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

Slide 115

Slide 115 text

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

Slide 116

Slide 116 text

class PricingRange [...] def add(pricing) delete_complete_overlaps(pricing) adjust_overlaps(pricing) split_containing(pricing) [...] end end

Slide 117

Slide 117 text

SPLIT CONTAINING

Slide 118

Slide 118 text

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

Slide 119

Slide 119 text

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

Slide 120

Slide 120 text

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

Slide 121

Slide 121 text

class PricingRange [...] def add(pricing) delete_complete_overlaps(pricing) adjust_overlaps(pricing) split_containing(pricing) expand_to_meet(pricing) [...] end end

Slide 122

Slide 122 text

EXPAND TO MEET

Slide 123

Slide 123 text

def expand_to_meet(new) expand_to_meet_before(new) expand_to_meet_after(new) end

Slide 124

Slide 124 text

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

Slide 125

Slide 125 text

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

Slide 126

Slide 126 text

def first_pricing pricings.order('start_date ASC').first end

Slide 127

Slide 127 text

class Pricing [...] def is_after?(other) self.start_date > other.end_date end end

Slide 128

Slide 128 text

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

Slide 129

Slide 129 text

def expand_to_meet_after(new) [...] end

Slide 130

Slide 130 text

I think you probably get it...

Slide 131

Slide 131 text

class PricingRange [...] def add(pricing) delete_complete_overlaps(pricing) adjust_overlaps(pricing) split_containing(pricing) expand_to_meet(pricing) coalesce_pricings end end

Slide 132

Slide 132 text

COALESCE PRICINGS

Slide 133

Slide 133 text

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

Slide 134

Slide 134 text

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

Slide 135

Slide 135 text

def previous_pricing_for(pricing) pricings.where(end_date: pricing.start_date - 1.day).first end

Slide 136

Slide 136 text

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

Slide 137

Slide 137 text

class PricingRange [...] def add(pricing) delete_complete_overlaps(pricing) adjust_overlaps(pricing) split_containing(pricing) expand_to_meet(pricing) coalesce_pricings end end

Slide 138

Slide 138 text

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

Slide 139

Slide 139 text

DONE!

Slide 140

Slide 140 text

WRAP-UP

Slide 141

Slide 141 text

WRITE TESTS

Slide 142

Slide 142 text

Ugly, conditional code, is made more beautiful by tests.

Slide 143

Slide 143 text

DON'T EVER USE UNLESS/ELSE

Slide 144

Slide 144 text

unless thing do_something else do_something_else end

Slide 145

Slide 145 text

IF YOU READ THAT AND IT MADE SENSE, YOU ARE A ROBOT.

Slide 146

Slide 146 text

WE'RE ALL STILL GOING TO USE CONDITIONALS.

Slide 147

Slide 147 text

IT'S OK.

Slide 148

Slide 148 text

WHAT'S IMPORTANT IS THAT YOU FEEL BAD ABOUT IT.

Slide 149

Slide 149 text

That's all, folks!

Slide 150

Slide 150 text

Oh, and you should probably follow me on Twitter @eroberts

Slide 151

Slide 151 text

THINGS YOU SHOULD CHECK OUT FOR MORE INFORMATION

Slide 152

Slide 152 text

▸ 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