Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Example for Shotgun Surgery #24

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
santakadev wants to merge 9 commits into main
base: main
Choose a base branch
Loading
from step-shotgun-surgery-example-base

Conversation

@santakadev
Copy link
Contributor

@santakadev santakadev commented Apr 8, 2021
edited
Loading

This is an example for illustrating Shotgun Surgery Code Smell, in which we compute the duration of different course Step types.

The logic for determining the duration of a Step is coupled to the step type:

  • Duration(videoStep) = VideoDuration * 1.1
  • Duration(quizStep) = QuestionCount * estimatedTimePerQuestion * 1.5

The 1.1 and 1.5 are factors that might represent:

  • Video (1.1): increases the duration due to video pauses.
  • Quiz (1.5): increases the duration due to final review of the quiz.

In this example, instead of making the behaviour cohesive to the data, we have intentionally break the cohesion by spreading the behaviour in many classes and unnecessary abstractions.

Showing Shotgun Surgery Smell

In our current context, our platform only support a few types of Steps, and we know that we will add new ones in the near future, so the lack of cohesion will be a change preventer.

To show Shotgun Surgery smell, we can try to add a new CodeExerciseStep type with a new duration logic.

For that, we can start adding a new test in GetStepDurationTest that validates the behaviour of the duration for the new type.

Then, if we follow the same patterns for adding the production code, the implementation becomes crazy:

  • Add STEP_TYPE_CODE_EXERCISE constant in StepEnums class.
  • Add STEP_DURATION_MULTIPLIER_CODE_EXERCISE constant in StepEnums class.
  • Create StepDurationCalculatorCodeExercise class.
  • Add the new calculator in StepDurationCalculatorFactory.
  • Add the logic for the new type in DurationMultiplier class.
  • Add STEP_DURATION_MULTIPLIER_CODE_EXERCISE to the STEP_TYPES array in StepEnums class.

Solving Shotgun Surgery Smell

If we take a look to the different abstractions, we will realize that all of them are coupled to the Step type. So we can perform some refactorings (Move method, Move constant, Inline class, Parallel change) for gaining cohesion and reduce accidental complexity.

The main focus of the refactorings should be:

  • Get rid of DurationMultiplier
  • Get rid of StepDurationCalculator
  • Get rid of StepEnums

@santakadev santakadev self-assigned this Apr 8, 2021
@santakadev santakadev force-pushed the step-shotgun-surgery-example-base branch from 0b1331f to 51d21ef Compare April 8, 2021 15:02
@santakadev santakadev changed the title (削除) Step shotgun surgery example base (削除ここまで) (追記) Example for Shotgun Surgery illustration (追記ここまで) Apr 8, 2021
@santakadev santakadev force-pushed the step-shotgun-surgery-example-base branch from 51d21ef to 241ea85 Compare April 8, 2021 17:00
@santakadev santakadev changed the title (削除) Example for Shotgun Surgery illustration (削除ここまで) (追記) Example for Shotgun Surgery (追記ここまで) Apr 9, 2021
@santakadev santakadev marked this pull request as ready for review April 9, 2021 12:05
@santakadev santakadev force-pushed the step-shotgun-surgery-example-base branch from 241ea85 to 4069df1 Compare April 16, 2021 15:12
@santakadev santakadev force-pushed the step-shotgun-surgery-example-base branch from e1c2a19 to e5f3274 Compare April 19, 2021 08:52
@santakadev santakadev force-pushed the step-shotgun-surgery-example-base branch from e5f3274 to a235f20 Compare April 19, 2021 08:52
Copy link
Member

@JavierCane JavierCane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to merge and commit the review suggestions afterwards in main if you prefer so 😊

@@ -0,0 +1,126 @@
# PHP Bootstrap (base / project skeleton)
Copy link
Member

@JavierCane JavierCane May 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 😬

@@ -0,0 +1,65 @@
{
"name": "codelytv/php-bootstrap",
Copy link
Member

@JavierCane JavierCane May 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 😬


declare(strict_types=1);

namespace CodelyTv\StepShotgunSurgery\Application;
Copy link
Member

@JavierCane JavierCane May 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be awesome to talk about the problem domain in the example title instead of revealing the illustrated code smell.

Why:

  • Avoid giving hints on how to approach the solution
  • Avoid collision with other examples for this very same code smell
  • Avoid having to decide which code smell include in the example name in cases where multiple of them are illustrated
  • Be able to be consisten about the example "branding' between languages. That is, not having to include the language in its folder name

Alternative suggestion: course_steps_duration_calculator


public const QUIZ_QUESTION_DURATION = 5;

# Important: don't forget to add here the type!!
Copy link
Member

@JavierCane JavierCane May 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎩

@JavierCane JavierCane added the enhancement New feature or request label May 3, 2021
@rgomezcasas rgomezcasas removed their request for review August 16, 2021 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JavierCane JavierCane JavierCane approved these changes

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /