3

I have a lightning component controller class:

public class ComponentVisibilityController {
        @AuraEnabled
        public static Boolean checkToDisplayComponent(Id recordId) {
            return ComponentVisibilityFacade.run(new LeadComponentVisibility(recordId)); 
        }
    }

In the ComponentVisibilityFacade class:

public class ComponentVisibilityFacade {
        public static Boolean run(ComponentVisibility componentVisibility) { //LeadComponentVisibility is a Child of ComponentVisibility. 
            componentVisibility.setCurrentUser();
            .
            .
            .
            .
            .
            return componentVisibility.displayUnshared();
        }
    }

In the ComponentVisibility class I have the below method and more:

public virtual class ComponentVisibility {
        protected User currentUser = null;

        public void setCurrentUser() {
                currentUser = UserDetailsUtil.queryUserDetails(userInfo.getUserId());
                System.debug('currentUser: ' + currentUser.Name);
        }
            
        public Boolean displayUnshared() {
            .
            .
            .
        }
    }

As you can see UserDetailsUtil is static class that calls queryUserDetails method and returns the current user.

I have other Util classes (most of them just query various objects and return results) that are all static and are called in LeadComponentVisibility class.

So in summary:

  • A static class ComponentVisibilityController (I know the class can instantiated but since it only has a static method, I am not going to instantiate it, hence a static class) is calling another static class ComponentVisibilityFacade, where a new instance of LeadComponentVisibility (dynamic binding) is created which calls another static class UserDetailsUtil (via inheritance) to get current user.

Now:

  1. Should I make UserDetailsUtil an instantiable class? Or is it ok I leave it static? I mean is there a problem with multiple instances of LeadComponentVisibility class calling the static UserDetailsUtil?
  2. Is my design correct? That is, I am following good software design principles(SOLID)? Or am I missing something or can the class structures be improved? For example: UserDetailsUtil just queries user object based on the user or lead owner id I am passing and does nothing else(the S in SOLID).
  3. LeadComponentVisibility decides whether to make a component visible or not but also calls other classes and their methods because I need to decide whether a component is visible based on current users and owners roles, hence setCurrentUser method and others. Is calling these classes and their methods in LeadComponentVisibility OK or am I violating the S?
  4. Since this is Util class if today itself I end up using UserDetailsUtil.queryUserDetails(Id user) method in another class that requires another field. What should I do? Since this field is used only in this another class and not in ComponentVisibility class, I am not unnecessarily querying a field I don't need?
  5. In UserDetailsUtil.queryUserDetails(Id user), I am querying only required fields of User. But tomorrow (say a year later) if I need another field of the user for the same ComponentVisibility class, how would I do that? Yes I know I can simply modify the query. But this violates the O in SOLID. Should I query all the fields so that I don't violate the O? Again, I am not unnecessarily querying fields I don't need?
  • 1
    IMHO purely static classes are an anti-pattern. You cannot mock them out and therefore cannot truly create unit tests for your apex - they are instead more like integration tests. – Phil W Jul 8 at 21:37
  • 1
    The other issue you need to address is ensuring you minimise the number of queries you do. You therefore will want to effectively cache data, queried once when first required and re-used on subsequent invocations within your session/transaction. – Phil W Jul 8 at 21:39
  • Regarding the caching, every user will have different results of query. So we can cache all results? – nSv23 Jul 8 at 21:44
  • maybe getUserById(Id userId), getUserByFederationId(String fedId), getUserByXXX(..)` and return either User or List<User> and let caller pluck field they need. Underlying utility needs to query once and cache; or accept collections as input – cropredy Jul 8 at 21:48
  • @PhilW regarding caching do you mean a static variable in the util class to cache the result? If so I am already doing that. But that conflict with your first comment about pure static classes being anti patttern. – nSv23 Jul 9 at 6:24
4

SOLID is a great principle to live by, in theory, but Apex has many practical limitations that require you to make pragmatic decisions that will violate the "letter" of SOLID. It's more important to live by the "spirit" of the policies as much as practical. SOLID is now 20 years old, and it definitely wasn't written with Apex (or any limited CPU/memory/query) language in mind.

Since we have governor limits, it's usually more important to remember YAGNI (You Ain't Gonna Need It) and KISS (Keep It Simple, S<redacted>). If you don't need instance variables or multiple copies, make your methods static. That's not really even a problem with SOLID; it says nothing about not using static methods.

A class may call other classes in order to maintain the Single Responsibility Principle. The "S" does not state that a class must do everything itself, simply that it should do just one thing. It should control a Visualforce page, or serve a REST response, or handle calculating sharing permissions for a user. It should not do all of those things in the same class. In particular, a child class should call its parent methods as necessary to help it out, otherwise it has little business being a child of that class.

The "O" has an interesting point. If you have a method queryUserDetails, but those details may vary, you should either have multiple methods, or simply query the records when necessary. You don't need to consolidate every single instance of a query into a method for reuse. It's okay to have similar queries in different places, especially if they'll never be called in the same transaction. Don't beat yourself up over it.

In regards to your last point, have you ever wondered why there's no "SELECT * FROM OBJECT" as there is in SQL? It's because Salesforce wants you to consider each query you write carefully to avoid excessive heap usage. It's a very limited resource, and a wildcard-like query could easily hit the governor limit with just a few rows of data. If you can't aggregate your queries into a single method, just use more than one, or use it on demand.

If you don't understand SOLID, don't worry about it too much. Honestly, most of these principles will come to you with experience, if you even need them. "S" is pretty common-sense, "O" will usually "mostly" occur normally, and if you have to violate it, you'll know why, "L" is rarely violated, as you have to either intentionally do so, or be so inexperienced that you've never used subclasses more than about three times, "I" is rare to even need, but if you do, you'll find it's actually the path of least resistance, and "D" is not particularly used in most business logic, so by the time you need it, you'll probably have the experience to use it responsibly.

My only recommendation is to ditch the queryUserDetails method until you actually find a common need for it. If queryUserDetails does something else (which might be a violation of "S"), have it accept a list of users and do what it needs to do (e.g. query roles, calculate sharing, etc). Each caller can then do the query it needs without worrying about the "O" principle. Alternatively, write different versions of the method, or have it accept a list of fields that the caller wants.

As far as static methods, as long as you don't need instance variables or method overrides, feel free to make them static. That's what static is meant to be used for. SOLID doesn't actually prohibit static methods anyways (it says nothing at all about it).

| improve this answer | |
  • thank you. I do have one last question. The whole purpose of the UserDetailsUtil class is to just query some fields of the user, so I either (1) create a dynamic query with string (which can also take in a list) and pass it to the class or (2) altogether ditch the class and do the dynamic query in the ComponentVisibility class itself or (3) create a single very generic query class (for any object) that simply accepts a query string as parameter and returns the result. Is there any differences /advantages/drawbacks between (1), (2) and (3) or they all the same? – nSv23 Jul 11 at 16:22
  • 1
    @nSv23 In most cases, a query class is superfluous until you have a standardized need (e.g. the same fields from the same objects with the same filters over many classes). It is perfectly appropriate to write a query directly into the class in question without violating "S" of SOLID. You may be overengineering at this point. Premature optimization is the root of all evil. comes to mind here. If you can't reuse the query as is, it probably doesn't belong where you're trying to put it. – sfdcfox Jul 11 at 18:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.