Skip to content
This repository has been archived by the owner on Jul 24, 2020. It is now read-only.

refactor complex non-method code in Reservation #346

Closed
ebmaher opened this issue Jun 12, 2013 · 5 comments
Closed

refactor complex non-method code in Reservation #346

ebmaher opened this issue Jun 12, 2013 · 5 comments
Milestone

Comments

@ebmaher
Copy link
Contributor

ebmaher commented Jun 12, 2013

CodeClimate gives the non-method code in Reservations a complexity rating of 150... yikes

@dgoerger
Copy link
Contributor

dgoerger commented Jul 7, 2014

_Update:_ CodeClimate currently gives the non-methods areas of the Reservations model a complexity score of 152.

I'm not entirely sure what there is to refactor outside of the methods. The scopes are a bit long, yes, but most (if not all) of them are entirely necessary (we should grep for these #635). We can look into rewriting them and deleting unused scopes, but.. everything else is commented to explain why it's there and what it's doing. We can't exactly run the app without validations.

@mnquintana @shippy @squidgetx @orenyk ?

@squidgetx
Copy link
Contributor

  • What is the difference between attr_accessor and attr_accessible? (section of code outside of scopes)
  • Scopes
  • Most validations will be removed after Review validations code #573 is finished

That's about all I can see. If someone investigates the first point than we can probably just close this

@shippy
Copy link
Contributor

shippy commented Jul 8, 2014

What is the difference between attr_accessor and attr_accessible?

attr_accessor is a Ruby construct; attr_accessible is an ActiveRecord extension that presumes a corresponding database column. (I had the same question earlier.)

Scopes

I was thinking about this and I actually think that, semantically, scopes might belong in a helper, or a helper-like separation.

On a relevant note, there's an argument for ditching them altogether and substituting them with arel calls. The upside would be that the scopes would be actually defined as methods and thus be subject to the standard lookup. But that might just be useless equivocation at this point, plus that post is from 2010.

@orenyk
Copy link
Contributor

orenyk commented Jul 8, 2014

The post you liked to basically says in the update at the end that it's a matter of personal preference. Also, don't forget, that CodeClimate is looking at the master branch, so a lot of the recent changes to the Reservation model aren't included. I think the changes that are in the works (scopes as in #635, validations as #573) will simplify this model as much as possible, but it is a complex (and important) piece of the puzzle :-)

@squidgetx
Copy link
Contributor

merge into #635 and mostly resolved in #704

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

No branches or pull requests

5 participants