Refactor 'new' action in AccountsController for better readability and efficiency

[AfolabiOlaoluwa]

• Used '.presence' instead of '.blank?' for handling both nil and empty string in 'params[:type]'
• Removed redundant condition for 'params[:type].blank?' by using 'type' directly
• Combined 'Account.new' calls into a single line

The distinction between .blank? and .presence lies in how they handle nil and empty strings.

.blank?: This method returns true if the object is nil, empty, or consists only of whitespace characters. However, it does not handle nil directly; instead, it relies on the truthiness of the object.

.presence: This method is particularly useful when you want to obtain a non-nil value. It returns nil if the object is blank (empty or consists only of whitespace characters), and it returns the object itself if it is present.

In the context of the action code, using .presence allows handling both nil and an empty string in a concise manner. If params[:type] is nil, it will be assigned to type as nil, and if it's an empty string, it will be assigned to type as nil as well. This makes the subsequent check (type.blank? || ...) more straightforward, as it covers both cases.

For more details, refer to the link https://api.rubyonrails.org/classes/Object.html#method-i-presence

[josefarias]

Nice thinking! I think we can take it a step further.

Consider the following:

maybe(dev)> params = {}
maybe(dev)> params[:type] = "Foo"
maybe(dev)> "Account::#{params[:type]}".presence_in(Account.accountable_types).constantize
#=> (irb):23:in `<main>': undefined method `constantize' for nil (NoMethodError)
#=> "Account::#{params[:type]}".presence_in(Account.accountable_types).constantize
maybe(dev)> params[:type] = nil
maybe(dev)> "Account::#{params[:type]}".presence_in(Account.accountable_types).constantize
#=> (irb):27:in `<main>': undefined method `constantize' for nil (NoMethodError)
#=> "Account::#{params[:type]}".presence_in(Account.accountable_types).constantize
maybe(dev)> params[:type] = "Credit"
maybe(dev)> "Account::#{params[:type]}".presence_in(Account.accountable_types).constantize
=> Account::Credit(id: uuid, created_at: datetime, updated_at: datetime)

We could use that to refactor as follows:

class AccountsController < ApplicationController
  def new
    @account = Account.new accountable_type: account_type_class
  end

  private

  def account_type_class
    "Account::#{params[:type]}".presence_in(Account.accountable_types).constantize
  rescue NameError
    Account
  end
end

We're presumably okay with defaulting to Account per

maybe/app/controllers/accounts_controller.rb

Line 40 in 48ade39

Account # Default to Account if type is not provided or invalid

But if not, we can just raise the NameError — fine to throw an exception if we got this far without a valid type param. I'll let @Shpigford confirm.

[josefarias]

Actually, per

maybe/app/models/account.rb

Line 4 in 48ade39

delegated_type :accountable, types: %w[ Account::Credit Account::Depository Account::Investment Account::Loan Account::OtherAsset Account::OtherLiability Account::Property Account::Vehicle], dependent: :destroy

I don't think rescuing and returning Account makes sense. I think we should just raise.

[robzolkos]

We just need to be careful as the new action renders a different partial depending on the presence of a valid type param. I think I'll a quick test to make that doesn't break with refactors. (But agree that this could be made cleaner)

[AfolabiOlaoluwa]

Well, it's a question of where the .constantize fails. If it won't fail, then if..else statement or a tenary operator is fine.

[AfolabiOlaoluwa]

Perhaps I need to look at this as a controller of its own rather than refactoring action by action. I also think a possible solution is to use || instead of raise even if .constantize happens to return a nit and fail.

[josefarias]

How is the type param obtained? Sorry I haven't looked myself. It shouldn't be open to freetext input. If we get to this point with an invalid type then someone's attempting something shady and it's fine to raise an exception. Arguably, even better to raise and capture in Sentry so we're aware of which attack vectors are being attempted.

[robzolkos]

Via links rendered when the type parameter is blank

[AfolabiOlaoluwa]

I have changes based on what I think is better instead of rescue NameError. You might want to check.

[josefarias]

I have changes based on what I think is better instead of rescue NameError. You might want to check.

Ah yeah, I don't think we should rescue NameError either, per #273 (comment)

Can take a look after work! Thanks for putting this together

[robzolkos]

I would advise against using modifier clauses like this. Standard Rails is to not use them in controller actions

For example here - a user will at first see that it returns :not_found. Its not until the end of the line they realise its conditional.

I know this is subjective and both ways work and it saves a couple of lines, but for readability and consistency with other controller actions we should defer to the more verbose if...else...

[AfolabiOlaoluwa]

I agree it is subjective. And I buy your point regarding consistencies with other controllers, which is rather also subjective in my opinion.

Perhaps we can decide what things should look like but I don't think that "rhyme" is totally given in a large codebase mostly with if..else conditional statements or tennary operators or modifier clause. That uniformity is not given that we'll have only if..else statements accross the board. It is not achievable!

On the other hand, if I take it a bit further to avert concerns in your example and pass in some ..else statement, this is what I think:

def new
  @account = build_account

  if @account.blank?
    head :not_found
  end
end

def create
  @account = build_and_associate_account

  unless @account.save
    render "new", status: :unprocessable_entity
  else
    redirect_to accounts_path, notice: "New account created successfully"
  end
end

Afterall, it's Ruby under the hood with dashes of Rails additions. So we don't have to follow Rails standards in such a situation. I really just want to clean these controllers one after the other and set some precedence we follow in this repo. Let me know what you think using the @account, but I certainly take preference in what I provided and I'll accept it in any repository since it works.

[robzolkos]

Isn't this

unless @account.save
  render "new", status: :unprocessable_entity
else
  redirect_to accounts_path, notice: "New account created successfully"
end

just flipping the standard

 if @account.save
  redirect_to accounts_path, notice: "New account created successfully"
else
  render "new", status: :unprocessable_entity
end

I'm not seeing the benefits 🤷‍♂️

[AfolabiOlaoluwa]

Absolutely not seeing the benefits too. Just trying to regularize the uniformity in your opinion.

[robzolkos]

See comment above re modifier clauses.

[AfolabiOlaoluwa]

Yes, I responded.

[robzolkos]

The other benefits of the if...else style is that it is clearer that the primary function of the action is to save the new account. By placing the render clause first it reads left to right like the rendering of "new" if the primary function (which it only true in the exceptional case).

Furthermore it is prone to AbstractController::DoubleRenderError exceptions (as would be in this particular case if the @account did not save).

[robzolkos]

This is a nice cleanup. I left a comment re my views on modifier clauses.

[josefarias]

So this PR has grown in scope since the last time I commented on it.

The last time I saw it, the refactoring was pretty close to what I would've done. So I thought it'd be good to comment and suggest what I felt was final polish.

Now the scope has grown and it makes multiple stylistic choices I wouldn't make. I don't think expounding on my thinking would be productive here as I don't own the codebase and there's no defined code style yet.

Debated whether I should comment at all but since I was asked for a review I didn't want to leave y'all hanging. It's good work, just too far removed from what I would recommend and I don't want to bikeshed here in the PR. Happy to pontificate elsewhere, though! Hit me up! 😄

I'll let the maintainers take it from here.

[Shpigford]

My thoughts are a similar version of what @josefarias mentioned. Specifically, a lot of discourse over a relatively small amount of code change.

At this stage of the product, my general feeling is it's not worth spending this much time/energy on code that very well could completely get ripped out or rewritten over the next few weeks.

The focus at the moment should be primarily on adding new functionality and less on refactoring.

Going to close this for now.

If there are specific bits of this that have a substantial impact on features, then feel free to submit those smaller code changes.