2

In my plugin code, I use early bound entities (generated via the crmsvcutil). Within my code, I am using MemberExpression to retrieve the name of the property. For instance, if I want the full name of the user who initiated the plugin I do the following

SystemUser pluginExecutedBy = new SystemUser();
pluginExecutedBy = Common.RetrieveEntity(service
                                        , SystemUser.EntityLogicalName
                                        , new ColumnSet(new string[] {Common.GetPropertyName(() => pluginExecutedBy.FullName)})
                                        , localContext.PluginExecutionContext.InitiatingUserId).ToEntity<SystemUser>();

The code for GetPropertyName is as follows

    public static string GetPropertyName<T>(Expression<Func<T>> expression)
    {
        MemberExpression body = (MemberExpression)expression.Body;
        return body.Member.Name.ToLower();
    }

The code for RetrieveEntity is as follows

public static Entity RetrieveEntity(IOrganizationService xrmService, string entityName, ColumnSet columns, Guid entityId)
        {
            return (Entity)xrmService.Retrieve(entityName, entityId, columns);
        }

My solution architect's comments:

Instead of writing the code like above, why not write it like this (hardcoding the name of the field - or use a struct).

SystemUser pluginExecutedBy = null;
pluginExecutedBy = Common.RetrieveEntity(service
                                        , SystemUser.EntityLogicalName
                                        , new ColumnSet(new string[] {"fullname"})
                                        , localContext.PluginExecutionContext.InitiatingUserId).ToEntity<SystemUser>();

Reason:

  1. Your code unnecessarily creates an object before it requires it (as you instantiate the object with the new keyword before the RetrieveEntity in order to use it with my GetProperty method) which is bad programming practice. In my code, I have never used the new keyword, but merely casting it and casting does not create a new object. Now, I am no expert in C# or .NET, but I like to read and try out different things. So, I looked up the Microsoft.Xrm.Sdk.dll and found that ToEntity within Sdk, actually did create a new Entity using the keyword new.
  2. If the Common.Retrieve returns null, your code has unnecessarily allocated memory which will cause performance issues whereas mine would not? A managed language like C# "manages the memory" for me, does it not?

Question

  1. Is my code badly written? If so, why? If it is better - why is it? (I believe it is a lot more cleaner and even if a field name changes as long as as the early bound class file is regenerated, I do not have to re-write any code)

  2. I agree that cast does not create a new object, but does my code unnecessarily create objects?

  3. Is there a better way (a completely different third way) to write the code?

Note: I suggested using the GetPropertyName because, he was hard-coding attribute names all over his code and so in a different project which did not use early bound entities I used structs for attribute names - something like below. I did this 3 weeks into my new job with CRM 2011 but later on discovered the magic of MemberExpression. He was writing a massive cs file for each of the entity that he was using in his plugin and I told him he did not have to do any of this as he could just use my GetPropertyName method in his plugin and get all the fields required and that prompted this code review comments. Normally he does not do a code review.

public class ClientName
{
    public struct EntityNameA
    {
        public const string LogicalName = "new_EntityNameA";
        public struct Attributes
        {
            public const string Name = "new_name";
            public const string Status = "new_status";
        }
    }
}

PS: Or is the question / time spent analyzing just not worth it?

2
  • 1
    It is never not worth asking a question and trying to become better at what you're doing. Also, criticizing your maybe slightly inefficient but not wrong solution to get at the attribute name in which you invested some creative thought while approving of the use of a static class and method doesn't let me think too highly of this "architect". Apr 29, 2013 at 22:13
  • @GCATNM - You and your double negatives! ;-) It took me a good 30 seconds to read the "never not worth" - the eyes read it, the brain interpreted it as "It is never worth ..." and I was half-inclined to click on Delete. Time to go and rest as it is 30 minutes to midnight!
    – Kanini
    Apr 29, 2013 at 22:26

2 Answers 2

3

Early Bound, Late Bound, MemberExpression, bla bla bla :)

I can understand the "philosophy", but looking at your code a giant alarm popup in my head:

public static Entity RetrieveEntity(IOrganizationService xrmService, string entityName, ColumnSet columns, Guid entityId)
        {
            return (Entity)xrmService.Retrieve(entityName, entityId, columns);
        }

the Retrieve throws an exception if the record is not found.

About the other things, the GetPropertyName is ok, but are always choices, for example I try to use always late bound in plugins, maybe in a project I prefer to use early bound, often there is more than one way to resolve a problem.

Happy crm coding!

4
  • Ha, I know...but that Common.RetrieveEntity function is also written by him. So, no comments :-) Also, wanted to post a complete scenario with code snippets. And, the "overlords" have decided to use early bound (no arguments, again). So, I have to use early bound, and the Common function - I can add, but not edit any of them!
    – Kanini
    Apr 29, 2013 at 22:19
  • 2
    I understand, but overthinking can lead to lose focus. You are using early bound, tomorrow a field name change, you need to rebuild your crmsvutil classes and recompile the plugin, so to have GetPropertyName<T> or to have the hardcoded field name make really so much difference inside a plugin? is the plugin so complex and re-usable for so many entities that is necessary to build a castle on top of it? :) Apr 29, 2013 at 22:27
  • In terms of the exception I don't really see the problem, almost all functions throw exceptions when improperly used. As that one expects to be given the ID of a record that exists, you are unlikely to use an ID that doesn't - where would you get it from? If it does throw an exception you may want it to bubble up, depends on the exception handling design.
    – James Wood
    Apr 30, 2013 at 11:56
  • 1
    @JamesWood it's rare but can happen (I think is possible to simulate using a synchronous plugin that delete the record before an asynchronous plugin retrieve it) but is not the point and is also a design choice as you said. What I wanted to express is the "useless feud" (in my opinion) between the OP and his software architect considering the described context (early-bound vs late-bound inside a plugin) Apr 30, 2013 at 12:33
2

Although GetPropertyName is a quite a clever solution I don't like it, and that's entirely to do with readability. To me its far easier to understand what is going on with: new ColumnSet(new string[] {"fullname"}).

But that's pretty much personal preference, but its important to remember that your not just writing code for yourself you are writing it for your team, they should be able to easily understand the work you have produced.

As a side a hardcoded string probably performs better at runtime. I usually hardcode all my values, if the entity model in CRM changes I will have to revisit to make changes in any case. There's no difference between early and late bound in that situation.

I don't understand the point of this function,

public static Entity RetrieveEntity(IOrganizationService xrmService, string entityName, ColumnSet columns, Guid entityId)
{
    return (Entity)xrmService.Retrieve(entityName, entityId, columns);
}

It doesn't do anything (apart from cast something that is already of that type).

1.Your code unnecessarily creates an object before it requires it (as you instantiate the object with the new keyword before the RetrieveEntity in order to use it with my GetProperty method) which is bad programming practice. In my code, I have never used the new keyword, but merely casting it and casting does not create a new object.

I believe this refers to; SystemUser pluginExecutedBy = new SystemUser(); I can see his/her point here, in this case new SystemUser() doesn't do much, but if the object you were instantiating did something resource intensive (load files, open DB connections) you might be doing something 'wasteful'. In this case I would be surprised if changing SystemUser pluginExecutedBy = null; actually yielded any significant performance gain.

2.If the Common.Retrieve returns null, your code has unnecessarily allocated memory which will cause performance issues

I would be surprised if that caused a performance issue, and anyway as Guido points out that function wont return null in any case.

Overall there is little about this code I strongly feel needs changing - but things can be always be better and its worth discussing (e.g. the point of code review), although it can be hard not to you shouldn't be precious about your code.

Personally I would go with hardcoded attribute names and dump the Common.RetrieveEntity function as it doesn't do anything.

pluginExecutedBy = service.Retrieve(SystemUser.EntityLogicalName, localContext.PluginExecutionContext.InitiatingUserId, new ColumnSet(new String[] {"fullname"} ));
1
  • I agree. I was re-writing the whole Common library of methods and in my code I included it because, he specifically stated that I cannot modify / delete anything. I added this comment though "The method does not do anything" :-) Given a choice, I would go with the struct instead of hardcoding as I have only one place to change. I was fiddling around with various aspects of CRM 2011 and thought - "Hey, here is a cool way of writing code".
    – Kanini
    Apr 30, 2013 at 13:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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