Code Smells & Refactoring

Code Smells & Refactoring

7a0e72a6f55811246bb5d9a946fd2e49?s=128

Radoslav Stankov

March 30, 2013
Tweet

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