Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add eenv_appname:get_/1/2 [ENHANCEMENT] #3

Open
yhafri opened this issue Oct 3, 2018 · 3 comments
Open

Add eenv_appname:get_/1/2 [ENHANCEMENT] #3

yhafri opened this issue Oct 3, 2018 · 3 comments

Comments

@yhafri
Copy link

yhafri commented Oct 3, 2018

Would it be possible to also generate get_/1,get_/2 functions like these:

% eenv_appname.beam
-module(eenv_appname).

-export([get/1]).
-export([get_/1, get_/2]).

get(ip) -> {ok, "127.0.0.1"};
get(_)  -> undefined.

%% extra API
get_(ip) -> "127.0.0.1";
get_(_)  -> undefined.

get_(Key, Default) ->
  case get_(Key) of
     undefined -> Default;
     Value -> Value
  end.

The idea with get_/1 is to return the value immediately, and avoid matching against the tuple {ok, Value} in the caller.

Second, get_/2 will return a default value instead of getting undefined.

As you may have noticed, I've added an underscore _ suffix to these new calls to not clash with the original get/1.

@yhafri yhafri changed the title Add eenv_appname:get/2 [ENHANCEMENT] Add eenv_appname:get_/1/2 [ENHANCEMENT] Oct 3, 2018
@yhafri
Copy link
Author

yhafri commented Oct 3, 2018

New eenv_router.erl module for consistency.

% eenv_router.beam
-module(eenv_router).

-export([get/2]).
-export([get_/2, get_/3]).

get(appname_1, Key) -> eenv_appname_1:get(Key);
get(appname_2, Key) -> eenv_appname_2:get(Key);
get(_, _) -> unload.

%% extra API
get_(appname_1, Key) -> eenv_appname_1:get_(Key);
get_(appname_2, Key) -> eenv_appname_2:get_(Key);
get_(_, _) -> unload.

get_(appname_1, Key, Default) -> eenv_appname_1:get_(Key, Default);
get_(appname_2, Key, Default) -> eenv_appname_2:get_(Key, Default);
get_(_, _,_) -> unload.

@yhafri
Copy link
Author

yhafri commented Oct 3, 2018

A better idea IMHO. The whole purpose of erl-env is to speed up application:get_env, right?
So why not go one level down, and produce this instead:

% eenv_appname.beam
-module(eenv_appname).

-export([get/1]).

-export([ip/0, port/0]).
-export([get_/1,get_/2]).

get(ip) -> {ok, "127.0.0.1"};
get(port) -> {ok, 8080};
get(_)  -> undefined.

%% extra API

ip() -> "127.0.0.1".
port() -> 8080.

get_(ip) -> "127.0.0.1";
get_(port) -> 8080;
get_(_) -> undefined.

get_(ip, _) -> "127.0.0.1";
get_(port, _) -> 8080;
get_(_, Default) -> Default.

@zhongwencool
Copy link
Owner

In my option:

  1. It would not increase speed with cutting down the {ok, Value} to Value(The difference is too small to be measured). But this will cause one confusion when it return undefined, this mean the key is missing or value is 'undefined' ?

ip() -> "127.0.0.1".
port() -> 8080.

This assuming that the Module find function is more efficient than function clause match?
I only know the function clause match is binary tree search. It's fast.
I don't know how module find function. maybe you can prove this by benchmark.

Inconvenient: Caller will make sure the key is exist. I think this will too low level down for normal user.

  1. Make the Default into function clause.

The original method just have one atom argument. It make the search as simple as possible.
And It would not hit the default branch and only hit the match clause at most of time in production environment.

The new method increase one argument, and no use most of time, Not sure the compile can optimize
this function clause.

Change to new method is very simple, you can do it and add a small benchmark in eenv_benchmark.erl to see.

PR is welcome~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants