3

I am attempting to encapsulate several features about a user in a single class. Although the main use for this class would be to initialize it once and never have to call set any variables again, I would like this class to be scalable to be called many times. Although by putting this into a class I feel like I am either over-complicating it or breaking OOP standards.

At the moment I am currently using these current sorts of calls throughout VBA code:

username = GetUsername()

OR

IF userHasSecurity(GetUsername(), "WhatUserHasAccessTo") = true THEN

What I am hoping to do it replace it (respectively) with:

  Set UserID = New Username
  UserID.GetUserName

OR

IF UserID.HasModuleAccess("WhatUserHasAccessTo") = true THEN

In addition to this change, I have a function that returns Active Directory information and also verify if usernames input exist in Active Directory

The Class Diagram is as such

Private UserName As String
Private FirstName As String
Private MiddleInitial As String
Private LastName As String

Private Sub class_Initialize()
Private Function findCurrentUsername() As String
Private Function doesUsernameExist(usernameToCheck As String) As Boolean  
Public Property Get GetUserName() As String
Public Property Let setUsername(newUsername As String)
Public Property Get getFirstName() As String
Private Property Let setFirstName(newFirstName As String)
Public Property Get getLastName() As String
Private Property Let setLastName(newLastName As String)
Public Property Get getMiddleInitial() As String
Private Property Let setMiddleInitial(newMiddleInitial As String)
Private Function findNameDetails()
Public Property Get getFullName() As String
Public Function HasModuleAccess(moduleName As String, Optional appName As String) As Boolean
Public Function getUserActiveDirectoryGroups() As DAO.Recordset

Is this more an opinion based execution(standalone functions vs. class) thing or is there something to gain? Does this break OOP standards and am I using classes incorrectly?

Elias
  • 193
  • 1
  • 6
  • 8
    I think you may have some terminology problems. A *username* in most nomenclatures, refers to a string that is unique to an individual or a group. As such it is represented as a string of characters. A `User` would have an attribute of `username` as well as `userId`, `fullName`, etc. – BobDalgleish Aug 06 '14 at 16:41
  • 1
    @BobDalgleish Fair point. In retrospect this should be a `User` class with `Username`, etc. As you said. – Elias Aug 06 '14 at 16:42
  • 4
    In addition, a User, UserName or UserID would not look up itself. That assumes knowledge of the underlying data store, knowledge which User, UserName and UserID should not require. – Robert Harvey Aug 06 '14 at 16:42
  • @RobertHarvey So, although the `User` class is currently able to switch to other users and information about that user you are suggesting that it should never have to lookup that information? How would you know the `Username` or `UserID` without looking it up first? – Elias Aug 06 '14 at 16:47
  • Something else, like a Repository, looks up the UserID and hands you a User object. – Robert Harvey Aug 06 '14 at 16:50
  • @RobertHarvey At the moment, when the class is initialized it initializes with the current user information, doing what you are saying. Although the issue being that you need to know the username beforehand to use it for anyone other than the logged in user. Is that your issue? Sorry for being a bit confused about exactly what you mean. – Elias Aug 06 '14 at 16:57
  • 2
    He's saying it's wrong for the `User` class to be responsible for retrieving user data from the database (and he's right). This is unnecessary coupling. `User`s should be plain old values, preferably immutable. – Doval Aug 06 '14 at 17:48
  • @Doval How could a User Class that depends on `Active Directory` contain plain values in the code and be immutable? I don't see how you could retrieve active up-to-date information without coupling the two? Can you give any examples to why doing something like this is wrong? – Elias Aug 06 '14 at 18:37
  • 2
    @Elias Create a class/function that hits up Active Directory and creates instances of `User` based on the info it retrieves. This makes the Active Directory code depend on `User` but not vice-versa. The reason is wrong is that the coupling is unnecessary; every time there's a change in where the user data is stored you'll have to modify the `User` class, even though the code using `User` is interested in the data, not where it came from. – Doval Aug 06 '14 at 18:44
  • @Elias, you would either want to have several different UserFactories that do know how to get the data to create a user, or your user itself could take in some interface object that knows how. Personally, I think the factory pattern is the better choice and more semanticly correct. – RubberDuck Dec 12 '14 at 06:54

1 Answers1

3

*before sitting down to writing code etc. - search, research available resources, functions - basically don't reinvent the wheel...

There is an Environ() function in VBA that pretty much does what you are describing and trying to imitate by using your own wrapper class.. ask yourself is it worth it? do you really need it that much?

Rule of thumb

Stick with good encapsulation

you said:

Although the main use for this class would be to initialize it once and never have to call set any variables again,

Don't assume things that may break good encapsulation - don't assume you will never have to do X and Y with your class - there are different techniques to protect objects state through properties.

As already pointed out in the comment section - it's important that your class is rather named User and not Username.

It's a user. If it's meant to be reusable meaning you can reuse it in many different projects - not have many instances of it then it shouldn't be depended on any other, unrelated, piece of your application - specially the active directory etc. If VBA supported parametarised constructors that could have been a different story based on some crucial circumstances but it does not so no argument is needed.

read this: Properties vs public variables in class modules

Hope you get the idea because as far as your User class goes you don't really need some of the properties you currently declared as properties...

I mean, look - you are not storing User's age -- you don't really need a Let property to check whether the age passed is not negative or not greater than some huge number... You are only storing some strings like firstName, lastName, middleInitial etc.. if you don't need to protect the access to change those then they should ideally be just public variables - acting sort of like read/write properties - without the ability to verify the data being assigned to them - but does it matter? They're Strings...

One good candidate for a property could be an Enumerable property UserAccessLevel...

First you would probably want to define a public Enum like

Public Enum UserAccessLevel
    ualReadOnly
    ualWriteOnly
    ualReadWrite
End Enum

then you could have a property like

private currentUserAccessLevel as UserAccessLevel

' cool stuff

Public Property Get ModuleAccess() as UserAccessLevel
    ModuleAccess = currentUserAccessLevel
End Property

Public Property Let ModuleAccess(value as UserAccessLevel)
    currentUserAccessLevel = value
End Property

That would be cool with all the intelli-sense tips for your enums etc...

Now, I have noticed this:

Public Function getUserActiveDirectoryGroups() As DAO.Recordset

And I have mixed feelings because

  • VBA doesn't support class polymorphism - so you can't derive a CustomUser from User - I know, that sucks... but you can create a new class and expose a User as a property of the class etc. Too broad to discuss here though...

  • getUserActiveDirectoryGroups() may not necessarily be something that any other project may want to use. I mean it's cool to have it there as long as it's not dependant on any other external and unrelated to the current User class members.

Say that the getUserActiveDirectoryGroups() needs to know things that current User class doesn't have access / doesn't know than it would probably be a better idea not to include that with the user class.

I could talk about this for another hour or two but I really hope at this point you feel a little bit more enlightened :)


Stick with good encapsulation.

from here

  • Understand the object-oriented principle of Encapsulation.
  • Learn the available modifiers for type members.
  • Protect object state through properties.
  • Control access to methods.
Community
  • 1