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