Extensible logging - DatabaseLogger

Recently I wrote a logging API that features an ILogger interface. I wanted to extend my library with a DatabaseLogger implementation, and I had to modify the API a little bit for this to work, but I think it was worth it.

Each ILogger instance now has its own ILogMessageFormatter. I no longer needed all loggers to be formatted identically, and I can now have a DebugLogger that writes different-looking entries than those written with a FileLogger, or even with another DebugLogger instance.

Here's the new ILogger interface:


ILogger class module (interface)

Option Explicit

Public Sub Log(ByVal level As LogLevel, ByVal message As String)
End Sub

Public Property Get Name() As String
End Property

Public Property Get MinLevel() As LogLevel
End Property

Public Property Get Formatter() As ILogMessageFormatter
End Property

I didn't change the ILogMessageFormatter interface, but the DatabaseLogMessageFormatter implementation ignores the parameters and uses the FormatMessage method to supply the DatabaseLogger with the SQL command string:


DatabaseLogMessageFormatter class module

Like all public classes in the library, the formatter has a default instance. But unlike the DefaultLogMessageFormatter implementation, this one doesn't expose the default instance, and requires the client to call the Create factory method to get an instance.

```
Option Explicit

Private Type TDatabaseLogMessageFormatter
SqlCommandText As String
End Type

Private this As TDatabaseLogMessageFormatter
Implements ILogMessageFormatter

Public Function Create(ByVal sql As String) As DatabaseLogMessageFormatter

Dim result As New DatabaseLogMessageFormatter
result.SqlCommandText = sql

Set Create = result

End Function

Friend Property Get SqlCommandText() As String
SqlCommandText = this.SqlCommandText
End Property

Friend Property Let SqlCommandText(ByVal value As String)
ValidateSQL value
this.SqlCommandText = value
End Prope

DatabaseLogMessageFormatter


I didn't change the ILogMessageFormatter interface, but the
DatabaseLogMessageFormatter implementation ignores the parameters and
uses the FormatMessage method to supply the DatabaseLogger with the
SQL command string:

Okay wow. I don't think that ignoring parameters sent into ILogMessageFormatter_FormatMessage is a very good idea. It will only lead to confusion for the maintainer. If you're forcing someone to pass parameters into a method, you should be using those parameters. I'll admit that I don't immediately see a way to correct it, but I seriously recommend thinking on it.

The next one comes only from having reviewed other code of yours.

' ?> @level
' ?> @logger
' ?> @message

In your unit testing framework, you use the @ sign to indicate "special" comments. Be careful peppering your code with it where it does not designate a special comment. It won't cause you an issue now, but could become a maintenance nightmare if you decide to create other extensions that use it as an indicator for other things.

Private Sub ValidateSQL(ByVal sql As String)

    'require an INSERT INTO command:
    If Not Framework.Strings.StartsWith("INSERT INTO ", sql, False) Then _
        OnInvalidSqlError

• Don't use a line continuation to avoid an End If. Consider it to be like omitting braces in c#.

• What if you create a stored procedure to handle the logging? It will never validate.

Private Sub OnInvalidSqlError()
    Err.Raise vbObjectError + 1192, "SqlCommandText", "Command must be 'INSERT INTO', with 3 parameters."
End Sub

• Magic Number.

• I find it nice to expose error numbers to client code.

A public constant would correct both of those issues. (An Enum would currently be overkill IMO. It's easy enough to change if you add another error number later.)

I would also consider raising the built in Runtime Error 5; Invalid Argument or Procedure Call instead of creating a custom error number.

DatabaseLogger

• I don't like the variable name result in the Create function. It's nitpicky, but result is fairly meaningless. I think dbLogger would be better.

-
This is a wise use of a line continuation.

Dim result As Boolean
result = this.SqlCmd.QuickExecuteNonQuery(this.Formatter.FormatMessage(level, this.Name, message), _
                                          sqlLevel, sqlLogger, sqlMessage)

-
but it would be unnecessary if you could figure a way to not ignore the parameters for Formatter.FormatMessage.

TestLogger

Dim connString As String
connString = "Provider=SQLOLEDB.1;Data Source=;Initial Catalog=CodeReviewSandbox;Integrated Security=SSPI;Persist Security Info=True;"

I know it's an example implementation, but people tend to write examples as they would any other code...

I wouldn't store connections in the code this way. There are a number of ways you could store connection strings, but pick one and use it. You shouldn't have to change code to change a database connection. Personally, I like using registry keys, but an *.ini file could also do the job fine. I tend to not muck with xml configuration files when dealing with VBA.

StackExchange Code Review Q#64122, answer score: 6