Andrew Lee GreyKn

  • Joined on
@GreyKn

Hi team, Good job on this project. How was working with external APIs? Interesting and sometimes hairpullingly frustrating, right? Think about the …

@GreyKn

Ignore public/assets so you don't check in precompiled assets into git.

@GreyKn

Oh, I see. You use valid to trigger populating the errors array. :+1:

@GreyKn

Should valid? be an expectation?

@GreyKn

Indent the html inside erb statements. It'll make it easier to read later. <% post.each %> <div> <% if post.class %> <div>Hi mom! </div> <% end %> …

@GreyKn

Indent is off for this closing div

@GreyKn

Try to only use unless when there's a single conditional.

@GreyKn

Why use a text value "nil" instead of an actual nil? Databases generally support nullable types.

@GreyKn

Good use of ternary and the return value of .save

@GreyKn

Using .reverse can lead to odd results unless you're running them against collections of primitives (numbers, strings). For hashes, it'd probably b…

@GreyKn

unless..else can lead to booleans being read a little oddly. Considering using if..else instead.

@GreyKn

The numbers in your post.link reference are a little magical. It'd be worth defining descriptive vars so future developers will know the how and wh…

@GreyKn

Ruby has case statements which can help you with this: matches = case params[:provider] when "instagram" our_user.instagram_users.where(ig_user_id:…

@GreyKn

See comment about scoping in sessions_controller

@GreyKn

One question, if you use User.find and it doesn't find a user, are you allowed to call .username on the returned object?

@GreyKn

Good use of ternary operation :+1:

@GreyKn

While this works for ruby, it's a potential scoping risk to define a variable for the first time inside an block. Normally, it's worth defining the…

@GreyKn

Is the second assignment to ig_account necessary?

@GreyKn

unless can be easy to misread when combined with operators, so for situations like this, use: @user = User.find(sessions[:user_id]) if session[:use…

@GreyKn

You could write this on one line to match your other auth method: def require_login redirect_to root_path unless session[:user_id] end

@GreyKn

For something like this, consider naming your method def instagram_client to match the instance var.

@GreyKn

I see that @twit_init was memoized, you could probably do the same with @instagram_client

@GreyKn

If you're trying to catch a negation, you can do this: expect(response).not_to render_template(:show)

@GreyKn

Always assume that a record won't be found and code a path around it. ActionControllers have a neat helper called rescue_from you can use to handle…

@GreyKn

There are a couple ways of doing it. I don't think you covered modules so I won't go into that here, but you could have written all the methods gen…

@GreyKn

The :each says "before each it, do this thing". It never deletes it anything explicitly. Usually what happens is that you assign a variable, which…

@GreyKn

Hey Alice, Really good job on this project. Super clean formatting and your tests are great. The only things I would add is that you could have gen…

@GreyKn

I noticed you're repeating your views for all your types. It's possible to have used a single shared set of views if you got a little creative with…

@GreyKn

Good use of a constant to hold a static referenced value

@GreyKn

Good use of loops to write 3 tests in one.