Code Smells & Refactoring

Video: https://www.youtube.com/watch?v=MHMWJkGS7sY

Transcript

1. Radoslav Stankov VarnaConf 2013 20/07/2013 Code Smells & Refactoring
2. Кой съм аз? @rstankov http://rstankov.com http://github.com/rstankov
8. Know your tools
9. DRY don’t repeat yourself
10. SLAP Single Level of Abstraction Principle
11. Law of Demeter
12. Out side of the box
13. Code smells
16. Code smell is any symptom in the source code of a program that possibly indicates a deeper problem. Code smells
17. Code smells are usually not bugs—they are not technically incorrect and don't currently prevent the program from functioning. Instead, they indicate weaknesses in design that may be slowing down development or increasing the risk of bugs or failures in the future. Code smells
19. Code refactoring is a disciplined technique for restructuring an existing body of code, altering its internal structure without changing its external behavior Refactoring
23. Exam ExamQuestion ExamEntry ExamAnswer
24. class ExamEntry < ActiveRecord::Base def result correct_answers = [] for answer in answers question = answer.question if (question.type == 'single' && question.correct_answer == answer.text) || (question.type == 'multi' && question.correct_answers.include?(answer.text)) correct_answers << answer end end wrong_answers = [] for answer in answers question = answer.question unless (question.type == 'single' && question.correct_answer == answer.text) || (question.type == 'multi' && question.correct_answers.include?(answer.text)) wrong_answers << answer end end score = 0 for answer in correct_answers score += answer.question.points end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end end
25. Long Method
26. A method, function, or procedure that has grown too large. Long Method
27. describe ExamEntry do describe "#result" do it "works ( I hope :P )" do exam = create :exam single_1 = create :single_question, exam: exam, points: 1 single_2 = create :single_question, exam: exam, points: 2 multi_1 = create :multi_question, exam: exam, points: 1 multi_2 = create :multi_question, exam: exam, points: 2 entry = create(:exam_entry, answers: [ create(:answer, question: single_1, text: single_1.correct_answer), create(:answer, question: single_2, text: '-wrong-answer-'), create(:answer, question: multi_1, text: multi_1.correct_answers.first), create(:answer, question: multi_2, text: '-wrong-answer-') ]) result = entry.result result.keys.should eq [:correct_answers, :wrong_answers, :score] result[:correct_answers].map(&:question).should eq [single_1, multi_1] result[:wrong_answers].map(&:question).should eq [single_2, multi_2] result[:score].should eq single_1.points + multi_1.points end end end
28. describe ExamEntry do describe "#result" do it "works ( I hope :P )" do exam = create :exam single_1 = create :single_question, exam: exam, points: 1 single_2 = create :single_question, exam: exam, points: 2 multi_1 = create :multi_question, exam: exam, points: 1 multi_2 = create :multi_question, exam: exam, points: 2 entry = create(:exam_entry, answers: [ create(:answer, question: single_1, text: single_1.correct_answer), create(:answer, question: single_2, text: '-wrong-answer-'), create(:answer, question: multi_1, text: multi_1.correct_answers.first), create(:answer, question: multi_2, text: '-wrong-answer-') ]) result = entry.result result.keys.should eq [:correct_answers, :wrong_answers, :score] result[:correct_answers].map(&:question).should eq [single_1, multi_1] result[:wrong_answers].map(&:question).should eq [single_2, multi_2] result[:score].should eq single_1.points + multi_1.points end end end
29. describe ExamEntry do describe "#result" do it "works ( I hope :P )" do exam = create :exam single_1 = create :single_question, exam: exam, points: 1 single_2 = create :single_question, exam: exam, points: 2 multi_1 = create :multi_question, exam: exam, points: 1 multi_2 = create :multi_question, exam: exam, points: 2 entry = create(:exam_entry, answers: [ create(:answer, question: single_1, text: single_1.correct_answer), create(:answer, question: single_2, text: '-wrong-answer-'), create(:answer, question: multi_1, text: multi_1.correct_answers.first), create(:answer, question: multi_2, text: '-wrong-answer-') ]) result = entry.result result.keys.should eq [:correct_answers, :wrong_answers, :score] result[:correct_answers].map(&:question).should eq [single_1, multi_1] result[:wrong_answers].map(&:question).should eq [single_2, multi_2] result[:score].should eq single_1.points + multi_1.points end end end
30. describe ExamEntry do describe "#result" do it "works ( I hope :P )" do exam = create :exam single_1 = create :single_question, exam: exam, points: 1 single_2 = create :single_question, exam: exam, points: 2 multi_1 = create :multi_question, exam: exam, points: 1 multi_2 = create :multi_question, exam: exam, points: 2 entry = create(:exam_entry, answers: [ create(:answer, question: single_1, text: single_1.correct_answer), create(:answer, question: single_2, text: '-wrong-answer-'), create(:answer, question: multi_1, text: multi_1.correct_answers.first), create(:answer, question: multi_2, text: '-wrong-answer-') ]) result = entry.result result.keys.should eq [:correct_answers, :wrong_answers, :score] result[:correct_answers].map(&:question).should eq [single_1, multi_1] result[:wrong_answers].map(&:question).should eq [single_2, multi_2] result[:score].should eq single_1.points + multi_1.points end end end
31. describe ExamEntry do describe "#result" do it "works ( I hope :P )" do exam = create :exam single_1 = create :single_question, exam: exam, points: 1 single_2 = create :single_question, exam: exam, points: 2 multi_1 = create :multi_question, exam: exam, points: 1 multi_2 = create :multi_question, exam: exam, points: 2 entry = create(:exam_entry, answers: [ create(:answer, question: single_1, text: single_1.correct_answer), create(:answer, question: single_2, text: '-wrong-answer-'), create(:answer, question: multi_1, text: multi_1.correct_answers.first), create(:answer, question: multi_2, text: '-wrong-answer-') ]) result = entry.result result.keys.should eq [:correct_answers, :wrong_answers, :score] result[:correct_answers].map(&:question).should eq [single_1, multi_1] result[:wrong_answers].map(&:question).should eq [single_2, multi_2] result[:score].should eq single_1.points + multi_1.points end end end
32. describe ExamEntry do describe "#result" do it "works ( I hope :P )" do exam = create :exam single_1 = create :single_question, exam: exam, points: 1 single_2 = create :single_question, exam: exam, points: 2 multi_1 = create :multi_question, exam: exam, points: 1 multi_2 = create :multi_question, exam: exam, points: 2 entry = create(:exam_entry, answers: [ create(:answer, question: single_1, text: single_1.correct_answer), create(:answer, question: single_2, text: '-wrong-answer-'), create(:answer, question: multi_1, text: multi_1.correct_answers.first), create(:answer, question: multi_2, text: '-wrong-answer-') ]) result = entry.result result.keys.should eq [:correct_answers, :wrong_answers, :score] result[:correct_answers].map(&:question).should eq [single_1, multi_1] result[:wrong_answers].map(&:question).should eq [single_2, multi_2] result[:score].should eq single_1.points + multi_1.points end end end
33. describe ExamEntry do describe "#result" do it "works ( I hope :P )" do exam = create :exam single_1 = create :single_question, exam: exam, points: 1 single_2 = create :single_question, exam: exam, points: 2 multi_1 = create :multi_question, exam: exam, points: 1 multi_2 = create :multi_question, exam: exam, points: 2 entry = create(:exam_entry, answers: [ create(:answer, question: single_1, text: single_1.correct_answer), create(:answer, question: single_2, text: '-wrong-answer-'), create(:answer, question: multi_1, text: multi_1.correct_answers.first), create(:answer, question: multi_2, text: '-wrong-answer-') ]) result = entry.result result.keys.should eq [:correct_answers, :wrong_answers, :score] result[:correct_answers].map(&:question).should eq [single_1, multi_1] result[:wrong_answers].map(&:question).should eq [single_2, multi_2] result[:score].should eq single_1.points + multi_1.points end end end
34. describe ExamEntry do describe "#result" do it "works ( I hope :P )" do exam = create :exam single_1 = create :single_question, exam: exam, points: 1 single_2 = create :single_question, exam: exam, points: 2 multi_1 = create :multi_question, exam: exam, points: 1 multi_2 = create :multi_question, exam: exam, points: 2 entry = create(:exam_entry, answers: [ create(:answer, question: single_1, text: single_1.correct_answer), create(:answer, question: single_2, text: '-wrong-answer-'), create(:answer, question: multi_1, text: multi_1.correct_answers.first), create(:answer, question: multi_2, text: '-wrong-answer-') ]) result = entry.result result.keys.should eq [:correct_answers, :wrong_answers, :score] result[:correct_answers].map(&:question).should eq [single_1, multi_1] result[:wrong_answers].map(&:question).should eq [single_2, multi_2] result[:score].should eq single_1.points + multi_1.points end end end
35. describe ExamEntry do describe "#result" do it "works ( I hope :P )" do exam = create :exam single_1 = create :single_question, exam: exam, points: 1 single_2 = create :single_question, exam: exam, points: 2 multi_1 = create :multi_question, exam: exam, points: 1 multi_2 = create :multi_question, exam: exam, points: 2 entry = create(:exam_entry, answers: [ create(:answer, question: single_1, text: single_1.correct_answer), create(:answer, question: single_2, text: '-wrong-answer-'), create(:answer, question: multi_1, text: multi_1.correct_answers.first), create(:answer, question: multi_2, text: '-wrong-answer-') ]) result = entry.result result.keys.should eq [:correct_answers, :wrong_answers, :score] result[:correct_answers].map(&:question).should eq [single_1, multi_1] result[:wrong_answers].map(&:question).should eq [single_2, multi_2] result[:score].should eq single_1.points + multi_1.points end end end
36. Text
37. def result correct_answers = [] for answer in answers question = answer.question if (question.type == 'single' && question.correct_answer == answer.text) || (question.type == 'multi' && question.correct_answers.include?(answer.text)) correct_answers << answer end end wrong_answers = [] for answer in answers question = answer.question unless (question.type == 'single' && question.correct_answer == answer.text) || (question.type == 'multi' && question.correct_answers.include?(answer.text)) wrong_answers << answer end end score = 0 for answer in correct_answers score += answer.question.points end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
38. def result correct_answers = [] for answer in answers question = answer.question if (question.type == 'single' && question.correct_answer == answer.text) || (question.type == 'multi' && question.correct_answers.include?(answer.text)) correct_answers << answer end end wrong_answers = [] for answer in answers question = answer.question unless (question.type == 'single' && question.correct_answer == answer.text) || (question.type == 'multi' && question.correct_answers.include?(answer.text)) wrong_answers << answer end end score = 0 for answer in correct_answers score += answer.question.points end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
39. def result correct_answers = [] for answer in answers question = answer.question if (question.type == 'single' && question.correct_answer == answer.text) || (question.type == 'multi' && question.correct_answers.include?(answer.text)) correct_answers << answer end end wrong_answers = [] for answer in answers question = answer.question unless (question.type == 'single' && question.correct_answer == answer.text) || (question.type == 'multi' && question.correct_answers.include?(answer.text)) wrong_answers << answer end end score = 0 for answer in correct_answers score += answer.question.points end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
40. Conditional complexity
41. Branches that check lots of unrelated conditions and edge cases that don't seem to capture the meaning of a block of code. Complex conditionals
42. (question.type == 'single' && question.correct_answer == answer.text) || (question.type == 'multi' && question.correct_answers.include?(answer.text))
43. describe ExamQuestion do describe "#correct_answer?" do context "single" do it "returns true if answer is correct" it "returns false if answer is wrong" end context "multi" do it "returns true if answer included in answers list" it "returns false if answer isn’t included in answers list" end end end
45. class ExamQuestion < ActiveRecord::Base def correct_answer?(answer) if type == 'single' correct_answer == answer else correct_answers.include? answer end end end
47. class ExamQuestion < ActiveRecord::Base def correct_answer?(answer) if type == 'single' correct_answer == answer else correct_answers.include? answer end end end
48. Text
49. def result correct_answers = [] for answer in answers question = answer.question if (question.type == 'single' && question.correct_answer == answer.text) || (question.t correct_answers << answer end end wrong_answers = [] for answer in answers question = answer.question unless (question.type == 'single' && question.correct_answer == answer.text) || (questi wrong_answers << answer end end score = 0 for answer in correct_answers score += answer.question.points end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
50. def result correct_answers = [] for answer in answers question = answer.question if (question.type == 'single' && question.correct_answer == answer.text) || (question.t correct_answers << answer end end wrong_answers = [] for answer in answers question = answer.question unless (question.type == 'single' && question.correct_answer == answer.text) || (questi wrong_answers << answer end end score = 0 for answer in correct_answers score += answer.question.points end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
51. def result correct_answers = [] for answer in answers question = answer.question if (question.type == 'single' && question.correct_answer == answer.text) || (question.t correct_answers << answer end end wrong_answers = [] for answer in answers question = answer.question unless (question.type == 'single' && question.correct_answer == answer.text) || (questi wrong_answers << answer end end score = 0 for answer in correct_answers score += answer.question.points end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
52. def result correct_answers = [] for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer end end wrong_answers = [] for answer in answers question = answer.question unless question.correct_answer? answer.text wrong_answers << answer end end score = 0 for answer in correct_answers score += answer.question.points end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
53. Text
54. def result correct_answers = [] for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer end end wrong_answers = [] for answer in answers question = answer.question unless question.correct_answer? answer.text wrong_answers << answer end end score = 0 for answer in correct_answers score += answer.question.points end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
55. def result correct_answers = [] for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer end end wrong_answers = [] for answer in answers question = answer.question unless question.correct_answer? answer.text wrong_answers << answer end end score = 0 for answer in correct_answers score += answer.question.points end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
56. def result correct_answers = [] for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer end end wrong_answers = [] for answer in answers question = answer.question unless question.correct_answer? answer.text wrong_answers << answer end end score = 0 for answer in correct_answers score += answer.question.points end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
57. Code duplication
58. Identical or very similar code exists in more than one location. Duplicated code
59. def result correct_answers = [] for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer end end wrong_answers = [] for answer in answers question = answer.question unless question.correct_answer? answer.text wrong_answers << answer end end score = 0 for answer in correct_answers score += answer.question.points end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
60. def result correct_answers = [] for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer end end wrong_answers = [] for answer in answers question = answer.question unless question.correct_answer? answer.text wrong_answers << answer end end score = 0 for answer in correct_answers score += answer.question.points end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
61. def result correct_answers = [] for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer end end wrong_answers = [] for answer in answers question = answer.question unless question.correct_answer? answer.text wrong_answers << answer end end score = 0 for answer in correct_answers score += answer.question.points end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
62. def result correct_answers = [] wrong_answers = [] for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer else wrong_answers << answer end end score = 0 for answer in correct_answers score += answer.question.points end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
63. Text
64. def result correct_answers = [] wrong_answers = [] for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer else wrong_answers << answer end end score = 0 for answer in correct_answers score += answer.question.points end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
65. def result correct_answers = [] wrong_answers = [] for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer else wrong_answers << answer end end score = 0 for answer in correct_answers score += answer.question.points end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
66. def result correct_answers = [] wrong_answers = [] score = 0 for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer score += question.points else wrong_answers << answer end end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
67. Text
68. def result correct_answers = [] wrong_answers = [] score = 0 for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer score += question.points else wrong_answers << answer end end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
69. def result correct_answers = [] wrong_answers = [] score = 0 for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer score += question.points else wrong_answers << answer end end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
70. def result correct_answers = [] wrong_answers = [] score = 0 for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer score += question.points else wrong_answers << answer end end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
71. Feature Envy
72. A class that uses methods of another class excessively. Feature envy
73. def result correct_answers = [] wrong_answers = [] score = 0 for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer score += question.points else wrong_answers << answer end end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
74. describe ExamAnswer do describe "#correct?" do it "returns true if answer is correct" it "returns false if answer is wrong" end end
75. Text
76. class ExamAnswer < ActiveRecord::Base def correct? question.correct_answer? text end end
77. Text
78. def result correct_answers = [] wrong_answers = [] score = 0 for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer score += question.points else wrong_answers << answer end end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
79. def result correct_answers = [] wrong_answers = [] score = 0 for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer score += question.points else wrong_answers << answer end end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
80. def result correct_answers = [] wrong_answers = [] score = 0 for answer in answers question = answer.question if question.correct_answer? answer.text correct_answers << answer score += question.points else wrong_answers << answer end end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
81. def result correct_answers = [] wrong_answers = [] score = 0 for answer in answers if answer.correct? correct_answers << answer score += answer.question.points else wrong_answers << answer end end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
82. Text
83. def result correct_answers = [] wrong_answers = [] score = 0 for answer in answers if answer.correct? correct_answers << answer score += answer.question.points else wrong_answers << answer end end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
84. def result correct_answers = [] wrong_answers = [] score = 0 for answer in answers if answer.correct? correct_answers << answer score += answer.question.points else wrong_answers << answer end end { correct_answers: correct_answers, wrong_answers: wrong_answers, score: score } end
85. Message chain
86. Long sequences of method calls to get routine data. Intermediaries are dependencies in disguise. Message chain
87. describe ExamAnswer do describe "#points" do context "correct answer" do it "returns its question points" end context "wrong answer" do it "returns 0" end end end
88. describe ExamAnswer do describe "#points" do context "correct answer" do it "returns its question points" end context "wrong answer" do it "returns 0" end end end
89. Text
90. class ExamAnswer < ActiveRecord::Base def points return 0 unless correct? question.points end end