2
\$\begingroup\$

I was tasked to write process that initially seemed like a straight forward thing. "Fetch data from database, create a cache object, and assign this data to it. Use existing class to model this after." That's it in a nutshell. The 'existing' class did not do any caching, just an async call across layers to the database. The purpose the async method is prevent the app from waiting for this data to load. I should mention this is custom app that run as a plug-in to the MS Excel (Don't ask!), and so it's a WPF app. As such, caching of larger data is done with a use of Dictionaries rather than using System.Runtime.Caching library. Originally I used a dictionary like this: public async Task<Dictionary<projectId, ProjectFeatureIndicatorMetadata>> GetProjectFeatureIndicatorsStatusAsync(Guid projectId) but the tech lead decided not to use a Dictionary due unnecessary memory usage.

So what I'm looking to gain from this post is constructive comments to see if this is overkill, can it be simplified, or improved, etc. Bottom line is, code works I just want to see if it can be improved. If you decide to down vote it, I would ask to leave comment as to why.

Doing to start with the data layer. So here the ReaderAsync reads the data set and assigns it to the ProjectFeatureIndicatorMetadata class:

public async Task<ProjectFeatureIndicatorMetadata> GetProjectFeatureIndicatorsStatusAsync(Guid projectId)
{
    ProjectFeatureIndicatorMetadata result = null;

    await GetSpHelper().ExecuteReaderAsync(
       cancellationToken: CancellationToken.None,
       spName: "ITV.usp_ProjectFeatureIndicators_GetByProjectId",
       parmsDg: parms => {
       parms.AddWithValue("@ProjectId:", projectId);
       },
       methodDg: async (reader, ct) =>
       {
          while (await reader.ReadAsync(ct))
          {
              result = new ProjectFeatureIndicatorMetadata(
                  Convert.ToBoolean(reader["SprcIncludesInd"]),
                  Convert.ToBoolean(reader["SprcVariablesInd"]),
                  Convert.ToBoolean(reader["SprcFlowInd"]),
                  Convert.ToBoolean(reader["SprcGenerationInd"]));
          }
          return true;
       });
   return result;
}

Then I have the this code in one of the business layers. I only included pertinent code:

public class ItvCacheFeatureFlagManager : IitvCacheFeatureFlagManager
{
   public static ProjectFeatureIndicatorMetadata _featureIndicatorFlagCache = 
             new ProjectFeatureIndicatorMetadata(true,true,true,true);

   public virtual async Task<ProjectFeatureIndicatorMetadata> GetProjectFeatureIndicatorMetadata(Guid projectId)
    {
        using (new WriteLock(_lock))
        {
            if (_featureIndicatorFlagCache != null)
            {
                return _featureIndicatorFlagCache;
            }
            else
            {
                var dbresult = await GetProjectFeatureIndicatorsStatusAsync(projectId);                    
                return dbresult;
            }
        }
    }
}

The type ProjectFeatureIndicatorMetadata is like model class and this is where I was not certain if I even need this:

public class ProjectFeatureIndicatorMetadata
{
    public ProjectFeatureIndicatorMetadata(
        bool SprcIncludesInd,
        bool sprcVariablesInd,
        bool sprcFlowInd,
        bool sprcGenerationInd)
    {
        SprcIncludesInd = sprcIncludesInd;
        SprcVariablesInd = sprcVariablesInd;
        SprcFlowInd = sprcFlowInd;
        SprcGenerationInd = sprcGenerationInd;
    }       

    public bool SprcIncludesInd { get; set; }
    public bool SprcVariablesInd { get; set; }
    public bool SprcFlowInd { get; set; }
    public bool SprcGenerationInd { get; set; }
}

Update: I refactored some of the code and included some of the startup code as it was suggested below.

 //Staring class:
 public class ClientUserInterface : IClientUserInterface
 {
    [NotNull]
    private ProjectData Data
    {
       [Pure, DebuggerStepThrough]
       get { return ProjectDataProvider.ProjectData; }
    }

    private void OpenPublisher(ProjectIdentifier project)
    {
       Task.Run(async () => await Data.GetProjectFeatureIndicatorMetadata(project));
    }
  }

  public class ProjectData : DependencyObject, INotifyPropertyChanged, IDisposable
  {
     // the green squidgy under _featureManger says "Field 'ProjectData._featureManger' 
     // is never assigned to, and will have its default value null
     private readonly IitvCacheFeatureFlagManager _featureManger;

     public async Task<ProjectFeatureIndicatorMetadata>
        GetProjectFeatureIndicatorMetadata(ProjectIdentifier projectId)
     {
        return await _featureManger.GetProjectFeatureIndicatorMetadata(projectId);
     }
  }

  public interface IitvCacheFeatureFlagManager
  {
     Task<ProjectFeatureIndicatorMetadata> 
         GetProjectFeatureIndicatorMetadata(Guid projectId);
  }

  public class ItvCacheFeatureFlagManager : IitvCacheFeatureFlagManager
  {
     private readonly ItvCacheFeatureFlagProvider _provider;
     private readonly ReaderWriterLockSlim _lock = 
          new ReaderWriterLockSlim();

     public static ProjectFeatureIndicatorMetadata _featureIndicatorFlagCache = 
         new ProjectFeatureIndicatorMetadata();

     public ItvCacheFeatureFlagManager( SqlConnectionFactory connectionFactory )
     {
        _provider = new ItvCacheFeatureFlagProvider(connectionFactory);
     }

     public virtual async Task<ProjectFeatureIndicatorMetadata>
         GetProjectFeatureIndicatorMetadata(Guid projectId)
     {
         using (new WriteLock(_lock))
         {
            if (_featureIndicatorFlagCache != null)
            {
               return _featureIndicatorFlagCache;
            }
            else
            {
              var dbresult = await GetProjectFeatureIndicatorsStatusAsync(projectId);                    
              return dbresult;
            }
        }
     }

     public virtual async Task<ProjectFeatureIndicatorMetadata> 
        GetProjectFeatureIndicatorsStatusAsync(Guid projectId)
     {
       var dbresult = await _provider.GetProjectFeatureIndicatorsStatusAsync(projectId);            
       return dbresult;
     }
 }

 public class ItvCacheFeatureFlagProvider : IitvCacheFeatureFlagProvider
 {
    private SqlConnectionFactory ConnectionFactory { get; }

    private SqlSpHelper GetSpHelper() => new SqlSpHelper(ConnectionFactory);

    public ItvCacheFeatureFlagProvider(SqlConnectionFactory connectionFactory)
    {
       ConnectionFactory = connectionFactory;
    }

    public async Task<ProjectFeatureIndicatorMetadata>
       GetProjectFeatureIndicatorsStatusAsync(Guid projectId)
    {
       ProjectFeatureIndicatorMetadata result = null;

       await GetSpHelper().ExecuteReaderAsync(
       cancellationToken: CancellationToken.None,
       spName: "ITV.usp_ProjectFeatureIndicators_GetByProjectId",
       parmsDg: parms => {
             parms.AddWithValue("@ProjectId:", projectId);
       },
       methodDg: async (reader, ct) =>
       {
          while (await reader.ReadAsync(ct))
          {
             result.SprcIncludesInd = Convert.ToBoolean(reader["SprcIncludesInd"]);
             result.SprcVariablesInd = Convert.ToBoolean(reader["SprcVariablesInd"]);
             result.SprcGenerationInd = Convert.ToBoolean(reader["SprcGenerationInd"]);
             result.SprcFlowInd = Convert.ToBoolean(reader["SprcFlowInd"]);
          }  
          return true;
       });
       return result;
    }
 }

public class ProjectFeatureIndicatorMetadata
{
    public ProjectFeatureIndicatorMetadata()
    {
    }       

    public bool? SprcIncludesInd { get; set; }
    public bool? SprcVariablesInd { get; set; }
    public bool? SprcFlowInd { get; set; }
    public bool? SprcGenerationInd { get; set; }
}
\$\endgroup\$
  • \$\begingroup\$ I still have doubts this code works as expected. ClientUserInterface.OpenPublisher calls ProjectData.GetProjectFeatureIndicatorMetadata, this is just a chain command and doesn't do any process, that gets chained to ItvCacheFeatureFlagManager.GetProjectFeatureIndicatorMetadata which checks if _featureIndicatorFlagCache is not null, which it never is null as it set with a default value. So it will always return _featureIndicatorFlagCache and never call into GetProjectFeatureIndicatorsStatusAsync \$\endgroup\$ – CharlesNRice Feb 2 at 3:49
1
\$\begingroup\$

Does this code work? The _featureIndicatorFlagCache is set right away and is never cleared out. So the check if it's null will always be false and send back just he default and never execute GetProjectFeatureIndicatorsStatusAsync.

I assume you want to cache the call to GetProjectFeatureIndicatorsStatusAsync but you won't be able to store that in one property unless it's a dictionary as I assume the data it returns is based on projectId.

If you want an example of a simple caching you can use the Lazy<Task<>> in the dictionary. The Lazy will only execute once and return the same task back to all the callers. I would just use the ConcurrentDictionary as not have to deal with the locking myself.

public static ConcurrentDictionary<Guid, Lazy<Task<ProjectFeatureIndicatorMetadata>>> _cache =
    new ConcurrentDictionary<Guid, Lazy<Task<ProjectFeatureIndicatorMetadata>>>();

The the cache method would look something like this

public virtual Task<ProjectFeatureIndicatorMetadata> GetProjectFeatureIndicatorMetadata(
    Guid projectId)
{
    // This is optional just removes any tasks that have been faulted to re-execute 
    if (_cache.TryGetValue(projectId, out var result))
    {
        // IsFaulted will be false while task is in flight - this is what we want and don't want to block on it
        if (result.Value.IsFaulted)
        {
            // cast as dictionary to get remove that also checks the value along with the key
            var cacheDictionary = (IDictionary<Guid, Lazy<Task<ProjectFeatureIndicatorMetadata>>>) _cache;
            cacheDictionary.Remove(
                new KeyValuePair<Guid, Lazy<Task<ProjectFeatureIndicatorMetadata>>>(projectId, result));
            result = null;
        }
    }

    if (result == null)
    {
        result = _cache.GetOrAdd(projectId, id => new Lazy<Task<ProjectFeatureIndicatorMetadata>>(GetProjectFeatureIndicatorsStatusAsync(id)));
    }

    return result.Value;
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for your reply! In the start up method I call this method like this: Task.Run(async () => await FeatureFlagManager.GetProjectFeatureIndicatorMetadata(project)); and checking the cache object like this: var cacheData = ItvCacheFeatureFlagManager._featureIndicatorFlagCache; before the call is made, cacheData is null and after it shows my four variable as 'true'. GetProjectFeatureIndicatorsStatusAsync is important as this where these values come from. \$\endgroup\$ – Risho Jan 31 at 19:06
  • \$\begingroup\$ Again how can you cache to a single property if the data is required by projectId? Is there only ever one project id? \$\endgroup\$ – CharlesNRice Jan 31 at 19:45
  • \$\begingroup\$ I timed out as wrote my previous comment. The projectId is not relevant after results are fetched. It just occurred to that I'm over writing the data from the database call when I instantiated the metadata class. That's fixed now. As to your latest comment, the projectId is just a key that gets one specific row from the database. After that it's not used. So this case there only one projectId per application session. \$\endgroup\$ – Risho Jan 31 at 20:16
  • \$\begingroup\$ Please add your startup code to your question as it seems important to reviewing this code. As in your code the GetProjectFeatureIndicatorMetadata method isn't static \$\endgroup\$ – CharlesNRice Jan 31 at 21:28
  • \$\begingroup\$ I've added an update above. I've refactored some of the code and included some of the interfaces and classes in various layers. I appreciate you sticking with this! \$\endgroup\$ – Risho Feb 1 at 0:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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