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
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)
1 Like
Awesome. Thanks a lot Thomas ! Will make the changes now.
Thanks a lot
1 Like
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.
Sure! You’re welcome. It’s not perfect but a good step in the right direction I’d say
2 Likes