Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

###GenerateMasterCode

GenerateMasterCode

###GenerateMasterCodes

GenerateMasterCodes

###GenerateMasterCode

###GenerateMasterCodes

GenerateMasterCode

GenerateMasterCodes

added 95 characters in body
Source Link
Heslacher
  • 50.9k
  • 5
  • 83
  • 177

If you don't use that method outside of the class you should consider to make it private.

If you don't use that method outside of the class you should consider to make it private.

Source Link
Heslacher
  • 50.9k
  • 5
  • 83
  • 177

###GenerateMasterCode

I would break the GenerateMasterCode() method into two methods and would declare the Random as a static field tof the class.

From the Random documentation:

The default seed value is derived from the system clock and has finite resolution. As a result, different Random objects that are created in close succession by a call to the default constructor will have identical default seed values and, therefore, will produce identical sets of random numbers. This problem can be avoided by using a single Random object to generate all random numbers.

Instead of using a char[] to generate the random characters I would use a StringBuilder.

public static string GenerateMasterCode(string startString, int mastercodeLength)
{
 if (startString.Length == masterCodeLength) { return startString; }
 if (startString.Length > masterCodeLength) { return startString.Substring(0, masterCodeLength); }
 return startString + GenerateRandomCharacters(masterCodeLength - startString.Length);
}
private static Random rd = new Random();
private static string GenerateRandomCharacters(int length)
{
 string numericChars = "X123456789";
 int length = numericChars.Length;
 StringBuilder builder = new StringBuilder(length);
 for (int i=0; i < length; i++)
 {
 builder.Append(numericChars[rd.Next(length)]);
 }
 return builder.ToString();
}

###GenerateMasterCodes

Right now you query in the while loop all of the MasterCodes although you are only interested in the Campain related MasterCodes. So you should query the MasterCodes which belongs to the Campain before you start with the for loop.

Assigning false to Used can be omitted, because that is the default value of bool.

You can use the var type for the List<MasterCode> as well.

public static void GenerateMasterCodes(GenerateMasterCode model)
{
 using (ApplicationDbContext _context = new ApplicationDbContext())
 {
 Campaign campaign = _context.Campaigns.Find(model.CampaignId);
 var campainRelatedMasterCodes = _context.MasterCodes.Where(m.CampaignCode == campaign.CampaignCode);
 var masterCodes = new List<MasterCode>();
 for (var i = 1; i <= model.Count; i++)
 {
 var masterCode = new MasterCode
 {
 CampaignId = campaign.CampaignId,
 CampaignCode = campaign.CampaignCode,
 SubmittedOn = DateTime.Now
 };
 // Ensure that no code already exists for that campaign.
 var code = GenerateMasterCode(model.StartString, model.CodeLength);
 while (campainRelatedMasterCodes.Any(m => m.Code == code))
 {
 code = GenerateMasterCode(model.StartString, model.CodeLength);
 }
 masterCode.Code = code;
 masterCodes.Add(masterCode);
 }
 try
 {
 _context.MasterCodes.AddRange(masterCodes);
 _context.BulkSaveChanges();
 }
 catch (Exception ex)
 {
 ...
 }
 }
}
lang-cs

AltStyle によって変換されたページ (->オリジナル) /