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

Some thoughts #25

Closed
JC5 opened this issue Oct 9, 2014 · 6 comments
Closed

Some thoughts #25

JC5 opened this issue Oct 9, 2014 · 6 comments

Comments

@JC5
Copy link
Member

JC5 commented Oct 9, 2014

Alright so here's the problem.

Sooner or later everything touches everything. Ew! There's transactions and journals and budgets and accounts and just about every single view has a different set of requirements. Some only need transactions, others only need budgets but sooner or later there's a search for the related journal or the transactions in a budget and then it's all back to square one: a single method is "including" a billion tables methods and resources.

Firefly can go about this two ways. Either it keeps everything ready just in case the current method requires it, or it does everything on the fly. Another, the current solution is to include stuff when we need it through helpers, which keeps the code fairly clean but passes of the problem to various helpers, builders, implementers and factories.

It's pretty difficult to find out what's needed because the application is growing and changing all the time. A 'service'-kind of a situation could catch some problems but in the end it's all down to App::make a billion things and just run with it. Services that "do exactly x" quickly overflow because every method needs a tiny thing done differently which ends with twenty thousand methods all doing things slightly differently.

So then. Maybe some rules and regulations to get this thing going? Throwing around the core ideas behind Firefly III all the time isn't working either. And the tests! The tests!

Grouping stuff within their respective controllers isn't working because give me five seconds before I come up with some kind of combined view that messes everything up.

Same with helpers. It's nice, moving code around, pushing it towards these helpers but sooner or later the same problem arises; it becomes a mess.

Oh and then there's the problem of combining views with JSON which often means I'm picking up the same things twice, writing code twice. It sucks.

And if I decide to make objects as inclusive as possible I'm also back to square one where showing a budget generates 50 billion queries "just in case".

It all starts so simple but it becomes a giant mess. I think I need to draw something.

@JC5
Copy link
Member Author

JC5 commented Oct 10, 2014

Alright

  1. First set (one for each ~model) is CRUD + validate. I mentioned "~model" because I'm not going to split transaction journals and transactions until I really have to. Controllers are responsible for passing on the data in such a way that it fits.

The return value for CRUDv (I just made that up) are:

  • The model OR a message bag.
  • The model(s)
  • The model OR a message bag
  • A message bag (message bagS)
  1. Then there's a set called "tools" or maybe "helpers" or I don't know. Email, preferences, validation helpers (expanding on Laravel's ruleset), Form helpers and what-not.

  2. Triggers follow the CRUD principle as to what they're triggered by. update model, create something, etc.

Every constructor contains the necessary App::make calls to create the classes / objects needed.

  1. Then, last set is "the rest". The methods to 'search for transactions in budget x with amount z between two dates'. The methods that 'format a list of accounts in an overly specific way'. They have to go.

If it's unique to one method it doesn't need to be extracted. Once it does, it becomes Helper/Controller/X or Helper/Model/X or Helper/??/X where X will probably be the model or the thing or whatever it is we're dealing with this time. Still have to fill in the ?? without being overly vague. /Helper/Helps/X is really weird.

@JC5
Copy link
Member Author

JC5 commented Oct 10, 2014

/Helper/Shared/X it is, thanks to the Classnamer! I can't believe I just used that tool.

@JC5
Copy link
Member Author

JC5 commented Oct 10, 2014

So now there's a feature freeze so I can try to a few things done. Whatever I am missing somewhere, that becomes a new issue and something to build. In the future.

  1. Slowly refactor /lib/ so it fits the description you see above.
  2. Make sure all views fit the new layout.
  3. This includes the JSON powered tables which will be refactored to be a part of their own controller (the JSON calls are now handled by an external controller called JsonController). This is a nice use case for /Helper/Shared/Json.
  4. Make sure every form can submit, submit+return, validate.
  5. Nice comments, mess detection and everything.
  6. INTEGRATION TEST every controller action and UNIT TEST every helper.

@JC5
Copy link
Member Author

JC5 commented Oct 10, 2014

With all of this work I'd almost start over. Again! But NO.

@JC5
Copy link
Member Author

JC5 commented Oct 10, 2014

Alright to make it easier the "new" library will be called "FireflyIII" so I can more easily move over and bla bla.

@JC5
Copy link
Member Author

JC5 commented Oct 10, 2014

Also remove all "helper" methods from the models.

@JC5 JC5 closed this as completed Jan 19, 2015
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

1 participant