Looking for a code review on code that use Plug

Hey!

I have a few comments on web.ex as a whole:

• web.ex:27 cond can simply be an if/else expression

• web.ex:50-55 should be possible to split this method using pattern matching (e.g. def valid_github_payload?(%{"issue" => _issue} = body_params, [{"x-github-event", "issue_comment", ...}])),

  You can then have one function definition per case (last one matches on any body_params and simply returns false). This way the function is easier to digest and it’s more obvious what the different scenarios are.

  The function definition might get a bit long if you’re gonna pattern match on the complete request headers, so perhaps an idea is to leave out the {"content-type", "application/json"} part.

I agree about perhaps refactoring the valid_github_payload? function cause it indeed does two things now. Just looking at that function name alone I think it makes sense it either returns true or false (not a tuple).

The request handler could then (when true) invoke handle_github_payload, which sends the private message, or does nothing. Again pattern matching can be used.

Finally, a few subjective remarks:

• web.ex:15 remove space after _ . Also I like this style better:

adapter_port = port(System.get_env("PORT")))
{:ok, _} = Plug.Adapters.Cowboy.http(PhubMe.Web, [], adapter_port)

• web.ex:20 and web.ex:22 would both have them in the same style instead of only one via the , do: shorthand

def port(nil), do: 8080
def port(value), do: String.to_integer(value)

… Actually, I don’t think you need the port method at all, this would do the job as well:

port = (System.get_env("PORT") || "8080") |> String.to_integer
{:ok, _} = Plug.Adapters.Cowboy.http(PhubMe.Web, [], port)

Awesome. Thanks a lot Thomas ! Will make the changes now.

Thanks a lot

I made the changes : Fix first github call when creating webhook by benoittgt · Pull Request #16 · benoittgt/PhubMe · GitHub

It’s much better now. I see a little bit of duplication between valid_github_payload? and handle_github_payload. But the method are readable.

Sure! You’re welcome. It’s not perfect but a good step in the right direction I’d say