Looking for a code review on code that use Plug

Questions / Help
,
#1

Hello

I wrote a very simple tool on Elixir to get notified on Slack when someone mention you in and issue on Github.

The project use Plug and I recently added a fix for a specific behavior of the github’s api but I’m not sure about what I did.

The change is small (don’t be scared about the 187 lines it’s the fixture) and simple but I would to get some feedbacks.

https://github.com/benoittgt/PhubMe/pull/16

Feel free to make any comments. I have already shared some doubts on the pull request.

Thanks

3 Likes
#2

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 _ :yum: . 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)
1 Like
#3

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

Thanks a lot

1 Like
#4

I made the changes : https://github.com/benoittgt/PhubMe/pull/16/files

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.

#5

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

2 Likes