HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Am I using Lazy initialization properly?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
properlyinitializationlazyusing

Problem

The following code is intended to implement a LinFu DynamicProxy interceptor to lazy load given virtual properties from an Umbraco datastore.

My concerns:

  • Thread safety, Have I covered all the bases, Am I storing variables correctly?



  • Duplication - Getting the child type, am I safe to store a variable via my Lazy to use later?



  • Efficiency, If I have everything else correct, am I being wasteful?



The code:

```
///
/// The content interceptor for intercepting virtual content properties.
///
internal class ContentInterceptor : IInvokeWrapper
{
///
/// The reader writer lock slim.
///
private static readonly ReaderWriterLockSlim ReaderWriterLockSlim
= new ReaderWriterLockSlim();

///
/// The lazy content.
///
private static readonly Lazy LazyParent = new Lazy(
() =>
{
IContent content = contentService.GetById(id);
IContent parent = content.Parent();
return parent;
});

///
/// The lazy list content.
///
private static readonly Lazy> LazyChildren =
new Lazy>(
() =>
{
IContent content = contentService.GetById(id);
Type childType = returnType.GetGenericArguments().Single();

AliasAttribute aliasAttribute = childType.FirstAttribute();
string alias = Conventions
.GetPropertyTypeAlias(aliasAttribute, childType.Name);

IEnumerable umbracoChildren = contentService
.GetChildrenByName(content.Id, alias);
return umbracoChildren;
});

///
/// The id.
///
private static int id;

///
/// The return type.
///
private static Type returnType;

///
/// The content service.
///
private static IContentService contentService;

///
/// The target to intercept.
///
private readonly obj

Solution

I'm not familiar with Umbraco, and I could not find any documentation on LinFu, I do, however, have some remarks on your code:

Comments - your code is more than 40% comments, and un-useful comments at that - a comment that tells me that id is /// The id is a waste of space and reduces readability of your code - I actually needed to copy your code locally and remove all the comments to read it.

Static vs. instance scopes - you chose to put some of the state in the instance, and some in the class, but you manage both from the class. This means that if you create two instances of your class, their state will be unexpected, as they will change each-other's state. If you this class is meant to be a Singleton, make it a Singleton.

Use locks properly - you chose to use ReaderWriterLockSlim as your lock object. That's OK, though I don't see why you would need a reader-writer lock.

The problem is that you misuse it - the way you use it is as if it was just a regular object. Again, a simple lock object is fine, but declaring it as a fancy lock type, you confuse the code reader to look for (and assume) its use.

Context

StackExchange Code Review Q#42413, answer score: 7

Revisions (0)

No revisions yet.