Skip to content
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

refatored conferences_controller #3037

Closed
wants to merge 1 commit into from
Closed

refatored conferences_controller #3037

wants to merge 1 commit into from

Conversation

carlosdanielpohlod
Copy link

Checklist

  • [ x ] I have read the Contribution & Best practices Guide.
  • [x ] My branch is up-to-date with the upstream master branch.
  • [x ] The tests pass locally with my changes.
  • I have added tests that prove my fix is effective or that my feature works(if appropriate).
  • I have added necessary documentation (if appropriate).

Short description of what this resolves/which issues does this fix?:

Changes proposed in this pull request:
Moved some resources of conferences_controller to new services to improve the code legebility

related issue: #3036

@image_url = @service.conference_image_url(request)

if splashpage.include_cfp
cfp_variables

Choose a reason for hiding this comment

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

What do you think about naming this like "assign_cfp_variables"?

end

if splashpage.include_program
program_variables

Choose a reason for hiding this comment

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

Maybe "assign_program_variables"?

@@ -0,0 +1,57 @@
class ConferencesCalendarService

Choose a reason for hiding this comment

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

What do you think about make this service stateless and delegate the responsibility of returning a full/not-full to itself? Suggestion:

# Create or Build (not enough context)
class CreateConferenceCalendarService
  def self.call(calendar, conference, is_full_calendar)
    new(calendar, conference, is_full_calendar).call
  end
  
  def initialize(calendar, conference, is_full_calendar)
    @calendar = calendar
    @conference = conference
    @is_full_calendar = is_full_calendar
  end
  private_class_method :new
  
  def call
    if full?
      ...
    else
      ...
    end
  end
  
  private
  
  def full?
    @is_full_calendar
  end
end

@hennevogel
Copy link
Member

@carlosdanielpohlod ping

@carlosdanielpohlod carlosdanielpohlod closed this by deleting the head repository Nov 8, 2022
@carlosdanielpohlod
Copy link
Author

Sorry, I will open another PRs to apply these changes.

The calendar refactor is in this other one:
#3101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants