Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm using ConcurrentDictionary to hold the records.. I'm reading files from the local system in parallel, to speed up the process.

Sample 1:

static ConcurrentDictionary<Int64, Byte[]> concurrentDictionary = new ConcurrentDictionary<Int64, Byte[]>();

private void ReadFilesFromDirectory()
{
  string[] docNames = Directory.GetFiles(CompleteLettersDirectory);
  Parallel.ForEach(docNames, new ParallelOptions { MaxDegreeOfParallelism = 8 }, file =>
    {
      try
      {
          int64 letterId = Convert.ToInt64(file.Split('_')[1]);
          Byte[] CompleteLetterContent;
          CompleteLetterContent = ConvertToPdfWithAspose(System.IO.File.ReadAllBytes(file), letterId);
          concurrentDictionary.GetOrAdd(letterId, CompleteLetterContent);
      }
      catch (Exception ex)
      {
          //Logger.WriteFile(letterId.ToString() + Environment.NewLine, ex);
      }
    }
}

Sample 2: In this sample I want to keep multiple values in dictionary as such I created a letter class which has multiple properties. As Letter class is not threadsafe, will it create any issues ?

static ConcurrentDictionary<Int64, Letter> concurrentDictionary = new ConcurrentDictionary<Int64, Letter>();

private void ReadFilesFromDirectory()
{
  string[] docNames = Directory.GetFiles(CompleteLettersDirectory);
  Parallel.ForEach(docNames, new ParallelOptions { MaxDegreeOfParallelism = 8 }, file =>
    {
      try
      {
          int64 letterId = Convert.ToInt64(file.Split('_')[1]);
          Byte[] LetterContent;
          Byte[] CompleteLetter;

          Letter letter = new Letter();
          letter.LetterId = letterId;

      if(file.Contains("CL"))
      {
    letter.CompleteLetter = ConvertToPdfWithAspose(System.IO.File.ReadAllBytes(file), letterId);
      }
      else
      {
    letter.LetterContent = ConvertToHTML(System.IO.File.ReadAllBytes(file), letterId);
      }
    concurrentDictionary.GetOrAdd(letterId, letter);
          }
      catch (Exception ex)
  {
          //Logger.WriteFile(letterId.ToString() + Environment.NewLine, ex);
      }
    }
}

public class Letter
  {
      public Int64 LetterId { get; set; }
      public Byte[] LetterContent { get; set; }
      public Byte[] CompleteLetter { get; set; }
  }

Can anyone advise if code samples are threadsafe. Also, should string docNames which holds documents from the directory and is passed to Parallel.ForEach be also threadsafe or not? Or any other recommendation.

share|improve this question

1 Answer 1

The term "threadsafe" usually describes an object's behavior when its member functions are invoked from different threads. That means that the Letter class is too simple to be described as threadsafe or not threadsafe - it's just a dumb class with some public members. There isn't a problem writing to different fields, and even writing to the same field can only go so wrong - one value gets lost, but what did you expect? The way you combine the Letter and the ConcurrentDictionary is backwards, though. You need to do something like:

var letter = concurrentDictionary.GetOrAdd(letterId, new Letter());
if(file.Contains("CL"))
{
    letter.CompleteLetter = ConvertToPdfWithAspose(System.IO.File.ReadAllBytes(file), letterId);
}
else
{
    letter.LetterContent = ConvertToHTML(System.IO.File.ReadAllBytes(file), letterId);
}

In your code, if the key exists the new letter you made (and stored data in!) is dropped on the floor.

Use of docNames is fine.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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