Code Smells & Refactoring

Transcript

1. Radoslav Stankov VarnaConf 2013 20/07/2013 Code Smells & Refactoring

2. Кой съм аз? @rstankov http://rstankov.com http://github.com/rstankov

3. None
4. None
5. None
6. None
7. None

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

14. None
15. None

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
18. None

19. Code refactoring is a disciplined technique for restructuring an existing

    body of code, altering its internal structure without changing its external behavior Refactoring
20. None
21. None
22. None

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
44. None

45. class ExamQuestion < ActiveRecord::Base def correct_answer?(answer) if type == 'single'

    correct_answer == answer else correct_answers.include? answer end end end
46. None

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