0
\$\begingroup\$

I wrote a simple logger for a server I'm writing. I'm wondering if anyone has any suggestions to improve it.

doom_logger.erl

-module(doom_logger).
-author('Andrew McRobb <andrewmcrobb@gmail.com>').

-include("include/log_types.hrl").

-export([start/1, start/0, terminate/1, log/4, log/3]).


-record(logger_state, {
        file_pointer = null,
        write_to_stdout = true,
        write_to_file = true,
        file_path = "doom.log",
        output_level = ?LOG_TYPE_DEBUG
    }).

% start/0 Simple shortcut when no arguments are provided.
start()->
    start([]).

% start/1 Initalizes the logger with key/value tuples. Simply ignores key's that aren't valid.
% Please use #logger_state as reference of key names.
start(Arguments)->
    WriteToStdOutState = update_logger_state(#logger_state{}, Arguments, write_to_stdout),
    WriteToFileState = update_logger_state(WriteToStdOutState, Arguments, write_to_file),
    FilePathState = update_logger_state(WriteToFileState, Arguments, file_path),
    OutputLevelState = update_logger_state(FilePathState, Arguments, output_level),

    if OutputLevelState#logger_state.write_to_file ->
        case file:open(OutputLevelState#logger_state.file_path, [append, {encoding, utf8}, {delayed_write, 1, 1}]) of
            {ok, Device} ->
                {ok, OutputLevelState#logger_state{file_pointer=Device}};
            Error ->
                Error
        end;
    true->
        {ok, OutputLevelState}
    end.


% terminate/1 terminates our file pointer of our log file.
terminate(#logger_state{file_pointer=null})->
    ok;
terminate(#logger_state{file_pointer=File})->
    file:close(File).

% log/3 writes to the console or output file. If output_level is set higher than the type of log,
% the function simply returns `ok`.
log(#logger_state{output_level=OutputLevel}, Type, _, _) when Type < OutputLevel -> ok;
log(State, Type, Message, FormatList) when ?IS_LOG_TYPE(Type) ->

    % Format our output log message for our log file or console.
    FormatedMessage = binary_to_list(erlang:iolist_to_binary(io_lib:format(Message, FormatList))),
    OutputMessage = lists:concat(["[", convert_type_int_to_string(Type), "] ", get_local_time_string(), " ", FormatedMessage, "\n"]),

    write_to_file(State, OutputMessage),
    write_to_console(State, Type, OutputMessage).

% log/2 writes to the console or output file. If output_level is set higher than the type of log,
% the function simply returns `ok`. This function simply doesn't support string formating for custom variables.
log(State, Type, Message)-> ?MODULE:log(State, Type, Message, []).

% write_to_file/2 Writes to our log file when 'file_pointer' is valid and 'write_to_file' is true.
write_to_file(#logger_state{file_pointer=null}, _)->
    ok;
write_to_file(#logger_state{write_to_file=false}, _)->
    ok;
write_to_file(#logger_state{file_pointer=FilePointer}, Message)->
    file:write(FilePointer, Message),
    ok.

% write_to_console/3 Writes to our stdout/stderr when 'write_to_stdout' is true.
write_to_console(#logger_state{write_to_stdout=false}, _, _) ->
    ok;
write_to_console(_, Type, Message) ->
    case Type of
        ?LOG_TYPE_WARNING ->
            io:format(standard_error, Message, []);
        ?LOG_TYPE_ERROR ->
            io:format(standard_error, Message, []);
        _->
            io:fwrite(Message)
    end,
    ok.

% convert_type_int_to_string/1 Converts our enum int to a string for log messages.
convert_type_int_to_string(Type) ->
    case Type of
        ?LOG_TYPE_DEBUG->
            "DEBUG";
        ?LOG_TYPE_MESSAGE->
            "MESSAGE";
        ?LOG_TYPE_WARNING->
            "WARNING";
        ?LOG_TYPE_ERROR->
            "ERROR";
        _->
            "UNKOWN"
    end.

% update_logger_state/3 Updates our #logger_state variable into a new state with the provided key/value tuples list and key (atom) we want.
update_logger_state(State, Arguments, Key) ->
    case proplists:get_value(Key, Arguments) of
        undefined ->
            State;
        Value ->
            case Key of
                write_to_stdout->
                    if erlang:is_boolean(Value) ->
                        State#logger_state{write_to_stdout=Value};
                    true->
                        State
                    end;
                write_to_file->
                    if erlang:is_boolean(Value) ->
                        State#logger_state{write_to_file=Value};
                    true->
                        State
                    end;
                file_path->
                    if erlang:is_list(Value) ->
                        State#logger_state{file_path=Value};
                    true->
                        State
                    end;
                output_level->
                    if erlang:is_integer(Value) ->
                        State#logger_state{output_level=Value};
                    true->
                        State
                    end;
                _->
                    State
            end
    end.

% get_local_time_string/0 Returns formated string of "Y-M-D HH:MM:SS"
get_local_time_string() ->
    {{Year, Month, Day}, {Hours, Minutes, Seconds}} = calendar:local_time(),
    lists:concat([Year, "-", Month, "-", Day, " ", Hours, ":", Minutes, ":", Seconds]).

log_types.hrl

-define(LOG_TYPE_DEBUG, 1).
-define(LOG_TYPE_MESSAGE, 2).
-define(LOG_TYPE_WARNING, 3).
-define(LOG_TYPE_ERROR, 4).

-define(IS_LOG_TYPE(V), (
        V =:= ?LOG_TYPE_DEBUG orelse
        V =:= ?LOG_TYPE_MESSAGE orelse
        V =:= ?LOG_TYPE_WARNING orelse
        V =:= ?LOG_TYPE_ERROR
    )).

Example

start()->
     {ok, L} = doom_logger:start([{write_to_file, false}, {output_level, ?LOG_TYPE_MESSAGE}]),
     doom_logger:log(L, ?LOG_TYPE_DEBUG, "I won't print output level set too high!"),
     doom_logger:log(L, ?LOG_TYPE_MESSAGE, "I sure will ~p!", ["print"]),
     doom_logger:terminate().

My only real concern is how I'm building the #logger_state with update_logger_state/3. I don't know if there really is a better way.

\$\endgroup\$
  • \$\begingroup\$ yours example not work, because not exist function terminate/0. \$\endgroup\$ – user110702 Dec 31 '16 at 18:25
1
\$\begingroup\$

I starts with several comments:

  1. Need handle case when in terminate function file_pointer equal to null:

    terminate(#logger_state{file_pointer = null}) ->
      ok;
    
  2. Redundant lines of code: _-> TypeString = "UNKOWN" and case with

    warning ->
        io:format(standard_error, OutputMessage, []);
    error ->
        io:format(standard_error, OutputMessage, []);
    
  3. Need export only the external(user) functions. For debug purpose should use directive compile(export_all).

Also your code looks bulky, so I plan to supplement this answer if no one will write it before.

Edit:

The most important thing when reading the code should be traceable logic. Let's write your task again.

  1. What should do the function start/1? Create the logger or return error. Write it:

    start(Arguments) ->
      create_logger(Arguments).
    
  2. The arguments need to be processed? Yes. Improve with by principle - one task - one function:

    start(Arguments) ->
      create_logger(parse(Arguments)).
    
  3. Writing function parse/1. How present the arguments? You used proplists but better use Maps. Do transform arguments to maps by using functions from maps module.

    parse(Arg) ->
      maps:filter(fun(K, _) -> options(K) end, maps:from_list(Arg)).
    
    options(write_to_stdout) -> true;
    options(write_to_file) -> true;
    options(file_path) -> true;
    options(output_level) -> true;
    options(_) -> false. 
    
  4. Next - creating logger. That's unite by part of function create and function update from your code. You can split this onto two functions but I not see in it necessity.

    create_logger(Map) ->
      Is_ToFile = maps:get(write_to_file, Map, true),
      FilePath = maps:get(file_path, Map, "doom.log"),
      File = case Is_ToFile of
               true ->
                 case file:open(FilePath, [append, {encoding, utf8}, {delayed_write, 1, 1}]) of
                   {ok, D} -> D;
                   Err -> Err
                 end;
               false -> null
             end,
      case File of
        {error, Error} ->
          Error;
        _ ->
          {ok,
            #logger_state
            {
              file_pointer = File,
              write_to_stdout = maps:get(write_to_stdout, Map, true),
              write_to_file = Is_ToFile,
              file_path = FilePath,
              output_level = maps:get(output_level, Map, ?LOG_TYPE_DEBUG)
            }
          }
      end.
    
  5. Log function. You have a lot of excess, we just need to compose a message and display it. Utility functions:

    write_if(true, console, Message) ->
      io:fwrite(Message);
    write_if(true, File, Message) ->
      file:write(File, Message);
    write_if(false, _, _) ->
      ok.
    
    get_type(?LOG_TYPE_DEBUG) ->
      "DEBUG";
    get_type(?LOG_TYPE_MESSAGE) ->
      "MESSAGE";
    get_type(?LOG_TYPE_WARNING) ->
      "WARNING";
    get_type(?LOG_TYPE_ERROR) ->
      "ERROR".
    

    and main:

    log(#logger_state{output_level = OutputLevel}, Type, _, _) when Type < OutputLevel -> ok;
    log(#logger_state{
      write_to_stdout = WriteToConsole,
      write_to_file = WriteToFile,
      file_pointer = FilePointer},
        Type, Message, FormatList) when ?IS_LOG_TYPE(Type) ->
    
      OutputMessage = lists:concat(
        ["[", get_type(Type), "] ",
          get_local_time_string(), " ",
          io_lib:format(Message, FormatList), "\n"]),
    
      write_if(WriteToConsole, console, OutputMessage),
      write_if(WriteToFile, FilePointer, OutputMessage).
    
\$\endgroup\$
  • \$\begingroup\$ I totally agree about the bulkiness! So by all means take a swing at whatever else you have in mind. I'm totally new to Erlang. So anything to make things simpler/better sound good to me. \$\endgroup\$ – Ajm113 Jan 1 '17 at 4:07
  • \$\begingroup\$ Okay updated the code and removed all the 'private' functions from export, added pattern matching functions for file/stdout writes for a little cleaner code, and also fixed the issue with the stdout case using atoms instead of MACROS. \$\endgroup\$ – Ajm113 Jan 1 '17 at 21:21
  • \$\begingroup\$ @Ajm113, I updated the answer. \$\endgroup\$ – user110702 Jan 1 '17 at 23:18
  • \$\begingroup\$ Wow, thank you so much! This is going to make the rest of the project's code base looking/working so much better! Thanks you so much! Definitely need to start thinking 'functional' or pattern matching in this case with functions. ;) \$\endgroup\$ – Ajm113 Jan 4 '17 at 13:25
  • \$\begingroup\$ @Ajm113, good. You can ask question about Erlang on SO with tag erlang or in Erlang chat. Btw, you can accept answer. \$\endgroup\$ – user110702 Jan 4 '17 at 20:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.