Retrieving database values through properties

I have a Settings table which I make frequent DlookUp calls to retrieve values. So I thought instead of having to write Nz(DLookup("FieldName", "TableName", "Condition"), vbNullString) everytime I needed the value, why not wrap it in a property in a standard module.

Public Property Get ActiveKey() As String
    ActiveKey = Nz(DLookup("[_ActiveKey]", "WmSettings"), vbNullString)
End Property

Public Property Let ActiveKey(ByVal pValue As String)
    CurrentDb.Execute StringFormat("UPDATE WmSettings SET [_ActiveKey]='{0}';", pValue), dbFailOnError
End Property

Then I can call it like this:

key_ = Settings.ActiveKey       'I tend to write the module name for clarity

Settings.ActiveKey = "FAD22L"

The same approach could easily apply to an objectFrame/subform scenario.

Public Property Get PageFrame() As Object
    Set PageFrame = Forms.frmMain.SubForm.Form.InnerSubForm
End Property

Public Property Get Page() As Form
    Set Page = Forms.frmMain.SubForm.Form.InnerSubForm.Form
End Property

So my question is, is this approach I've taken considered to be a good practice and does it add a considerable amount of overhead to the application?

Properties in Standard Modules

Standard modules can have Properties, but as @Rubberduck pointed out, they're not necessarily intuitive (although they do force you to qualify a call with the module name, so there's that as a redeeming quality). You could write public methods instead, but you could also create a class, and if you wanted to avoid the need to New it up, you could give that class a Predeclared attribute. A class would better reflect the stateful nature of the settings, and would give you the flexibility of abstracting the persistence of settings from the retrieval and usage of settings.

Right now, your approach looks up the value from the underlying table every time, and writes it to the table every time. That's time consuming disk access and page locking. It might be better to load the settings on demand, use/change the settings, and then save them on exit.

Variable Names

pValue is presumably short for parameterValue. Why not use the full name, or better still, something like keyName? pValue could be confused for the statistical term P-Value, or a user might infer that the p is Hungarian notation, but guess at what type that notation represents.

SQL Injection

Kudos for using the StringFormat (presumably from @Mat'sMug's CR question), but you're building a SQL statement on the fly, and a malicious user, or inadvertent user could cause an error, or worse, by providing a parameter with a single-quote in the pValue. What happens if the user supplied "', AdminMode = TRUE, AdminName = '"?

At the very least, sanitize your inputs. Better still, parameterize your queries and leave a sign up saying that Johnny DropTables isn't welcome.

LateBound PageFrame property

You're explicitly returning an Object, which forces usage of the property to be late-bound. You probably know the type of the return value, why not use it?

If you know the return-type will always be an instance of InnserSubForm, then return that type:

Public Property Get PageFrame() As Form_InnerSubForm
    Set PageFrame = Forms.frmMain.SubForm.Form.InnerSubForm
End Property

If the subform is set dynamically, and you only know the return type will be a Form, then return that base type:

Public Property Get PageFrame() As Access.Form
    Set PageFrame = Forms.frmMain.SubForm.Form.InnerSubForm
End Property

Clarifying the Form type

Access has built-in Form objects, but also allows the use of VBA UserForms. It might be helpful to qualify the type as Access.Form:

Public Property Get Page() As Access.Form

Be careful with properties matching built-in control names

Page is the name of a built-in type, but you're using it to return a Form. That might be confusing for users of the property.

Accessing the Forms object

The Forms object extends the names of open forms at run-time. That means you're using late-bound code and you're not getting any Intellisense. Worse still, if the form isn't open, or it's in the wrong state, the call will fail. At the very least, you should have some error handling.

Using the default instance of the form

Access creates a default predeclared instance of a form (much like VBA creates a default predeclared instance of a UserForm), but Access only does this if the form is open. However, it is also possible to create instances of the Access form (and, I'd argue, is the preferable way to deal with Forms and UserForms).

Your code is intimately tied to the default instance, and should perhaps be made capable of handling any instance of the form. See this example to see how a form with a default caption of "foo" is handled by VBA:

Dim frm As Form_frmMain
  Set frm = New Form_frmMain

  frm.Caption = "Bar"

  'Print the current instance's caption - Bar
  Debug.Print frm.Caption

  'Print the early-bound default instance's caption - Foo
  Debug.Print Form_frmMain.Caption

  'Print the late-bound default instance's caption - Foo
  Debug.Print Forms.frmmain.Caption

CurrentDb is a function used like an object

CurrentDB is a function that returns a pointer to a Database. It's a member of the Application class, but note this text from an old KB article:


The following example attempts to use the CurrentDb function to return a
pointer to the database that is currently open in Microsoft Access. Because
the code does not assign that database to an object variable, the pointer
returned by the CurrentDb function is temporary and becomes invalid after
the TableDef object is set. Consequently, any later references in your code
to the TableDef object variable will result in an error.

That is, usage of Database members against CurrentDB, like CurrentDB.Execute or, more correctly, CurrentDB().Execute can be unreliable and result in error Object Invalid or Not Set. You'll have more reliable code if you first assign the result of CurrentDB() to a local variable, and then access the Execute method from that variable.

```
D

StackExchange Code Review Q#162098, answer score: 5