Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

To use static classes instead of structs for storing template GUIDs #458

Open
sergey-krivchenko opened this issue Dec 11, 2018 · 1 comment

Comments

@sergey-krivchenko
Copy link

commented Dec 11, 2018

Hi. Is there a certain reason why structs were chosen to hold GUIDs of Sitecore templates and fields instead of static classes?
There are a few considerations I thought of:

  1. It is possible to instantiate a struct, which can be confusing to allow. With static classes, the compiler will not allow to do that.

  2. Microsoft recommends using static classes to define constants. Currently, the Helix documentation shows that the conventions define to choose only structs to clearly signal the Templates type’s unique function as a constants holder only. I believe it is, indeed, very important to indicate that, though a static class can fit here even better. In Microsoft's programming guide on how to define constants they actually suggest to use static classes. For even further description of it's purpose, class name qualifier can be made very self-descriptive, for example "TemplateConstants".

Furthermore, it can also be controversial that structures are used to hold constants only, according to this discussion on SO. In some other programming languages it could be true (like C/C++), but in C#, structures can have properties, methods and even implement interfaces.

  1. memory usage, size and copying aspects - a general recommendation for a structure instance size is to be less than 16 bytes, mostly because of copying aspects that can negatively impact the performance. Even though it is very unlikely that someone would decide to assign  the Templates structure to a variable (or to pass it to a method), still it does not seem to be prudent not to adhere to Microsoft's recommendation  to avoid defining a structure when it has an instance size more than 16 bytes. In our case, the structure contains only reference types and field-by-field copy is made (only references, not the objects itself). Considering that one object reference is 8 bytes in x64, usually, this recommendation is not met.

  2. I've asked about this on #helix-habitat on Sitecore Community Slack and there was common agreement among the community members that static classes are more suitable for such purpose . And I received recommendation to open an issue for that.

Do you think that indeed it will be better to use static classes to hold template GUIDs, instead of structs? Should I create a pull request for this?

Thank you.

@derekcorreia

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

Thank you for the commentary Sergey. I would agree that -- if I were creating a solution like Habitat from scratch and needed to store ID references to fields/templates -- I would likely use static classes too. I can't speak to if there is a specific reason it was implemented this way or not -- I'll leave the discussion open for others to comment.

Where I do see for certain that this can be improved is in the linked documentation on the Helix docsite -- the docs should reference static classes (or even ORM solutions) as optionally viable ways to achieve the same end goal. The docs read today as if structs are the only option -- which they are certainly not. I've tagged it so we can get around to changing that in the next doc rev.

I'd be fine with instantiating a PR, but I'd suggest waiting to see if anyone else has any thoughts/objections first that I'm just not aware of.

Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.