Skip to main content
Commonmark migration
Source Link

Overall looks very good, clean and well done. I only see minor tiny little things...

###Nitpick - "Initialise" vs "Initialize"

Nitpick - "Initialise" vs "Initialize"

This is just a minor nitpick, but it's kind of jumping at me:

private void Initialise()
{
    HttpRuntime.Cache.Add(_uniqueId, _uniqueId, new CacheDependency(null, _httpCacheKeys), DateTime.MaxValue, Cache.NoSlidingExpiration, CacheItemPriority.NotRemovable, Callback);
    InitializationComplete();
}

I find it weird that a method called Initialise() is calling one that's called InitializationComplete() - I think for consistency it would be better named Initialize().

Also the MSDN for InitializationComplete() says it's called from the constructor of derived classes to indicate that initialization is finished. - you are calling it from within the constructor, and I'm not familiar with using the ChangeMonitor class but it might be clearer to have that call explicitly show up in the constructor (similar to how InitializeComponents() remains in a form's constructor no matter what):

public HttpCacheChangeMonitor(string[] httpCacheKeys)
{
    _httpCacheKeys = httpCacheKeys;
    Initialize();
    InitializationComplete();
}

But then again it's no biggie, the code is very clear and easy to follow.


###Temporal Coupling

Temporal Coupling

That said I like constructors that do very little, and this is what you've got here, but the Initialise method seems of very little use, and has temporal coupling with the setting of _httpCacheKeys - I mean, if your constructor looked like this:

public HttpCacheChangeMonitor(string[] httpCacheKeys)
{
    Initialise();
    _httpCacheKeys = httpCacheKeys;
}

I'd expect it to blow up. One way to eliminate the temporal coupling, would be to take the array as a parameter:

private void Initialize(string[] httpCacheKeys)
{
    HttpRuntime.Cache.Add(_uniqueId, _uniqueId, new CacheDependency(null, httpCacheKeys), DateTime.MaxValue, Cache.NoSlidingExpiration, CacheItemPriority.NotRemovable, Callback);
    InitializationComplete();
}

...and then the order of operations doesn't matter anymore:

public HttpCacheChangeMonitor(string[] httpCacheKeys)
{
    Initialize(httpCacheKeys);
    _httpCacheKeys = httpCacheKeys;
}

(haven't looked at the test code)

Overall looks very good, clean and well done. I only see minor tiny little things...

###Nitpick - "Initialise" vs "Initialize"

This is just a minor nitpick, but it's kind of jumping at me:

private void Initialise()
{
    HttpRuntime.Cache.Add(_uniqueId, _uniqueId, new CacheDependency(null, _httpCacheKeys), DateTime.MaxValue, Cache.NoSlidingExpiration, CacheItemPriority.NotRemovable, Callback);
    InitializationComplete();
}

I find it weird that a method called Initialise() is calling one that's called InitializationComplete() - I think for consistency it would be better named Initialize().

Also the MSDN for InitializationComplete() says it's called from the constructor of derived classes to indicate that initialization is finished. - you are calling it from within the constructor, and I'm not familiar with using the ChangeMonitor class but it might be clearer to have that call explicitly show up in the constructor (similar to how InitializeComponents() remains in a form's constructor no matter what):

public HttpCacheChangeMonitor(string[] httpCacheKeys)
{
    _httpCacheKeys = httpCacheKeys;
    Initialize();
    InitializationComplete();
}

But then again it's no biggie, the code is very clear and easy to follow.


###Temporal Coupling

That said I like constructors that do very little, and this is what you've got here, but the Initialise method seems of very little use, and has temporal coupling with the setting of _httpCacheKeys - I mean, if your constructor looked like this:

public HttpCacheChangeMonitor(string[] httpCacheKeys)
{
    Initialise();
    _httpCacheKeys = httpCacheKeys;
}

I'd expect it to blow up. One way to eliminate the temporal coupling, would be to take the array as a parameter:

private void Initialize(string[] httpCacheKeys)
{
    HttpRuntime.Cache.Add(_uniqueId, _uniqueId, new CacheDependency(null, httpCacheKeys), DateTime.MaxValue, Cache.NoSlidingExpiration, CacheItemPriority.NotRemovable, Callback);
    InitializationComplete();
}

...and then the order of operations doesn't matter anymore:

public HttpCacheChangeMonitor(string[] httpCacheKeys)
{
    Initialize(httpCacheKeys);
    _httpCacheKeys = httpCacheKeys;
}

(haven't looked at the test code)

Overall looks very good, clean and well done. I only see minor tiny little things...

Nitpick - "Initialise" vs "Initialize"

This is just a minor nitpick, but it's kind of jumping at me:

private void Initialise()
{
    HttpRuntime.Cache.Add(_uniqueId, _uniqueId, new CacheDependency(null, _httpCacheKeys), DateTime.MaxValue, Cache.NoSlidingExpiration, CacheItemPriority.NotRemovable, Callback);
    InitializationComplete();
}

I find it weird that a method called Initialise() is calling one that's called InitializationComplete() - I think for consistency it would be better named Initialize().

Also the MSDN for InitializationComplete() says it's called from the constructor of derived classes to indicate that initialization is finished. - you are calling it from within the constructor, and I'm not familiar with using the ChangeMonitor class but it might be clearer to have that call explicitly show up in the constructor (similar to how InitializeComponents() remains in a form's constructor no matter what):

public HttpCacheChangeMonitor(string[] httpCacheKeys)
{
    _httpCacheKeys = httpCacheKeys;
    Initialize();
    InitializationComplete();
}

But then again it's no biggie, the code is very clear and easy to follow.


Temporal Coupling

That said I like constructors that do very little, and this is what you've got here, but the Initialise method seems of very little use, and has temporal coupling with the setting of _httpCacheKeys - I mean, if your constructor looked like this:

public HttpCacheChangeMonitor(string[] httpCacheKeys)
{
    Initialise();
    _httpCacheKeys = httpCacheKeys;
}

I'd expect it to blow up. One way to eliminate the temporal coupling, would be to take the array as a parameter:

private void Initialize(string[] httpCacheKeys)
{
    HttpRuntime.Cache.Add(_uniqueId, _uniqueId, new CacheDependency(null, httpCacheKeys), DateTime.MaxValue, Cache.NoSlidingExpiration, CacheItemPriority.NotRemovable, Callback);
    InitializationComplete();
}

...and then the order of operations doesn't matter anymore:

public HttpCacheChangeMonitor(string[] httpCacheKeys)
{
    Initialize(httpCacheKeys);
    _httpCacheKeys = httpCacheKeys;
}

(haven't looked at the test code)

Source Link
Mathieu Guindon
  • 75.6k
  • 18
  • 195
  • 469

Overall looks very good, clean and well done. I only see minor tiny little things...

###Nitpick - "Initialise" vs "Initialize"

This is just a minor nitpick, but it's kind of jumping at me:

private void Initialise()
{
    HttpRuntime.Cache.Add(_uniqueId, _uniqueId, new CacheDependency(null, _httpCacheKeys), DateTime.MaxValue, Cache.NoSlidingExpiration, CacheItemPriority.NotRemovable, Callback);
    InitializationComplete();
}

I find it weird that a method called Initialise() is calling one that's called InitializationComplete() - I think for consistency it would be better named Initialize().

Also the MSDN for InitializationComplete() says it's called from the constructor of derived classes to indicate that initialization is finished. - you are calling it from within the constructor, and I'm not familiar with using the ChangeMonitor class but it might be clearer to have that call explicitly show up in the constructor (similar to how InitializeComponents() remains in a form's constructor no matter what):

public HttpCacheChangeMonitor(string[] httpCacheKeys)
{
    _httpCacheKeys = httpCacheKeys;
    Initialize();
    InitializationComplete();
}

But then again it's no biggie, the code is very clear and easy to follow.


###Temporal Coupling

That said I like constructors that do very little, and this is what you've got here, but the Initialise method seems of very little use, and has temporal coupling with the setting of _httpCacheKeys - I mean, if your constructor looked like this:

public HttpCacheChangeMonitor(string[] httpCacheKeys)
{
    Initialise();
    _httpCacheKeys = httpCacheKeys;
}

I'd expect it to blow up. One way to eliminate the temporal coupling, would be to take the array as a parameter:

private void Initialize(string[] httpCacheKeys)
{
    HttpRuntime.Cache.Add(_uniqueId, _uniqueId, new CacheDependency(null, httpCacheKeys), DateTime.MaxValue, Cache.NoSlidingExpiration, CacheItemPriority.NotRemovable, Callback);
    InitializationComplete();
}

...and then the order of operations doesn't matter anymore:

public HttpCacheChangeMonitor(string[] httpCacheKeys)
{
    Initialize(httpCacheKeys);
    _httpCacheKeys = httpCacheKeys;
}

(haven't looked at the test code)