What do I have to do in this case: List<Sobject> or Sobject?

Question

I am beginner in Salesforce and I have to verify if the field Order.ShippingCountry is filled using the OrderId.

I would like to reuse this method below to fetch the Order.ShippingCountry. But I was wondering if it is a good practice to use Order as a return. Ideally, do I have to create another method to return a List<Order> ?

public class OrderSelector {

    public static Order getOrderAndOrderItemsById(Id orderId){

            return [SELECT Id,OwnerId,EffectiveDate,ShippingCountry,
                    AccountId,Account.Name
                    (SELECT Id, Quantity, UnitPrice, 
                     Product2.ProductCode,Product2.Name,Product2.Family
                     FROM OrderItems)
                    FROM Order
                    WHERE Id = :orderId];
        }}

Using the current version of the method above, my method will be like it:

public class ValidateOrder {

    public class ValidateOrderException extends Exception{}

    private static void validateShippingCountryByOrderId(Id orderId) {

          Order varOrder = OrderSelector.getOrderAndOrderItemsById(orderId);

          if(varOrder.ShippingCountry.isEmpty()){
            throw new ValidateOrderException(Label.InvalidateDeleteDocument);
          }
    }

}

Is my code following best practices? Or would it be better to apply List<Order> varOder ? Also, as a good practice, would it be better to create another method in OrderSelector class to fetch only the Order.ShippingCountry and not all these fields?

Answer 1

In my experience, selector methods should, in general:

• accept collections as input and return collections as output. This avoids null checks.

• not be static as it facilitates mocking patterns such as ApexMocks which as you evolve your experience, you may come to favor

  public class OrderSelector {
  
    public Order[] getOrderAndOrderItemsById(Set<Id> orderIds){
        orderIds.isEmpty()
         ? new List<Order>()
         : return [SELECT Id,OwnerId,EffectiveDate,ShippingCountry,
                AccountId,Account.Name
                (SELECT Id, Quantity, UnitPrice, 
                 Product2.ProductCode,Product2.Name,Product2.Family
                 FROM OrderItems)
                FROM Order
                WHERE Id IN :orderIds];
    }}
  

If you follow the Lightning Enterprise Architecture patterns, the method name would be changed to

selectByOrderIds(Set<Id> orderIds) or
selectById(Set<Id> orderIds) // less is more  

as the method is part of an OrdersSelector, so by definition, you don't need to repeat Order in the method name.

You could also name it:

selectWithOrderItemsById(Set<Id> orderIds)

if you want to be clear that the return includes OrderItems

Answer 2

It depends. As written, if the user doesn't have access to the record, or the record Id is invalid, you'll get a "No rows for assignment". You might want more explicit error handling in this event.

public static Order getOrderAndOrderItemsById(Id orderId){
  try {
    return [SELECT Id,OwnerId,EffectiveDate,ShippingCountry,AccountId,Account.Name
             (SELECT Id, Quantity, UnitPrice, Product2.ProductCode,Product2.Name,Product2.Family
                 FROM OrderItems)
                FROM Order
                WHERE Id = :orderId];
    } catch(ListException e) {
      throw new CustomExceptions.InvalidIdOrInsufficientAccess(recordId);
    }
}

That aside, I would generally prefer accepting a list of Id values, and returning a list of Order records, because you'll need it more likely than not, and you'll avoid accidentally writing SOQL inside a for loop later down the line:

for(Order record: Trigger.new) {
  Order queriedRecord = OrderSelector.getOrderAndOrderItemsById(record.Id);
  // etc...
}