Simple logger for a server

Question

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 <[email protected]>').

-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.

Answer 1

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).