Skip to main content
Code Review

Return to Answer

minor code fix + another simpler solution
Source Link
Xiaoy312
Xiaoy312
  • 3.1k
  • 17
  • 18
  • The first 67 elements are always present.
  • The last 2 elements are present if it is not null/empty
  • The elements are always in the same order.
private string ConstructSeedReference(SeedViewModel model)
{
    var reference = string.Concat(
        model.Customer.CustomerCode.TrimEnd(), 
        model.Seed.CollectionCodeId.TrimEnd(), 
        model.Seed.SpeciesId.TrimEnd(),
        model.Seed.Zone.TrimEnd(), 
        model.Seed.Elevation.TrimEnd(), 
        model.Seed.ColYear.TrimEnd(), 
        model.Seed.OrchardId);

    if (!string.IsNullOrEmpty(model.Seed.GID))
    {
        reference += model.Seed.GID;GID.TrimEnd();
    }
    
    if (!string.IsNullOrEmpty(model.Seed.Clone))
    {
        reference += model.Seed.Clone;Clone.TrimEnd();
    }
    
    return reference;
}

EDIT: Just reread your question, if your concern is only about the NullReferenceException. You can simply use null-coalescing operator ?.:

private string ConstructSeedReference(SeedViewModel model)
{
    return string.Concat(
        model.Customer.CustomerCode.TrimEnd(),
        model.Seed.CollectionCodeId.TrimEnd(),
        model.Seed.SpeciesId.TrimEnd(),
        model.Seed.Zone.TrimEnd(),
        model.Seed.Elevation.TrimEnd(),
        model.Seed.ColYear.TrimEnd(),
        model.Seed.OrchardId,
        model.Seed.GID?.TrimEnd(),
        model.Seed.Clone?.TrimEnd());
}
  • The first 6 elements are always present.
  • The last 2 elements are present if it is not null/empty
  • The elements are always in the same order.
private string ConstructSeedReference(SeedViewModel model)
{
    var reference = string.Concat(
        model.Customer.CustomerCode.TrimEnd(), 
        model.Seed.CollectionCodeId.TrimEnd(), 
        model.Seed.SpeciesId.TrimEnd(),
        model.Seed.Zone.TrimEnd(), 
        model.Seed.Elevation.TrimEnd(), 
        model.Seed.ColYear.TrimEnd(), 
        model.Seed.OrchardId);

    if (!string.IsNullOrEmpty(model.Seed.GID))
    {
        reference += model.Seed.GID;
    }
    
    if (!string.IsNullOrEmpty(model.Seed.Clone))
    {
        reference += model.Seed.Clone;
    }
    
    return reference;
}
  • The first 7 elements are always present.
  • The last 2 elements are present if it is not null/empty
  • The elements are always in the same order.
private string ConstructSeedReference(SeedViewModel model)
{
    var reference = string.Concat(
        model.Customer.CustomerCode.TrimEnd(), 
        model.Seed.CollectionCodeId.TrimEnd(), 
        model.Seed.SpeciesId.TrimEnd(),
        model.Seed.Zone.TrimEnd(), 
        model.Seed.Elevation.TrimEnd(), 
        model.Seed.ColYear.TrimEnd(), 
        model.Seed.OrchardId);

    if (!string.IsNullOrEmpty(model.Seed.GID))
    {
        reference += model.Seed.GID.TrimEnd();
    }
    
    if (!string.IsNullOrEmpty(model.Seed.Clone))
    {
        reference += model.Seed.Clone.TrimEnd();
    }
    
    return reference;
}

EDIT: Just reread your question, if your concern is only about the NullReferenceException. You can simply use null-coalescing operator ?.:

private string ConstructSeedReference(SeedViewModel model)
{
    return string.Concat(
        model.Customer.CustomerCode.TrimEnd(),
        model.Seed.CollectionCodeId.TrimEnd(),
        model.Seed.SpeciesId.TrimEnd(),
        model.Seed.Zone.TrimEnd(),
        model.Seed.Elevation.TrimEnd(),
        model.Seed.ColYear.TrimEnd(),
        model.Seed.OrchardId,
        model.Seed.GID?.TrimEnd(),
        model.Seed.Clone?.TrimEnd());
}
Source Link
Xiaoy312
Xiaoy312
  • 3.1k
  • 17
  • 18

String.Format("{0}{1}{2}{3}{4}{5}{6}", ...);

Since you are just doing concatenation, string.Concat is more appropriate here.

if (!String.IsNullOrEmpty(model.Seed.GID) && !String.IsNullOrEmpty(model.Seed.Clone))
{
    SeedRefId = String.Format("{0}{1}{2}{3}{4}{5}{6}{7}{8}",
    model.Customer.CustomerCode.TrimEnd(), model.Seed.CollectionCodeId.TrimEnd(), model.Seed.SpeciesId.TrimEnd(),
    model.Seed.Zone.TrimEnd(), model.Seed.Elevation.TrimEnd(), model.Seed.ColYear.TrimEnd(), model.Seed.OrchardId, model.Seed.GID.TrimEnd(), model.Seed.Clone.TrimEnd());
    return SeedRefId;
}

Between your ifs and else, there is a lot of repeated code. And, we can observe a pattern there:

  • The first 6 elements are always present.
  • The last 2 elements are present if it is not null/empty
  • The elements are always in the same order.

The method can be reduced to this:

private string ConstructSeedReference(SeedViewModel model)
{
    var reference = string.Concat(
        model.Customer.CustomerCode.TrimEnd(), 
        model.Seed.CollectionCodeId.TrimEnd(), 
        model.Seed.SpeciesId.TrimEnd(),
        model.Seed.Zone.TrimEnd(), 
        model.Seed.Elevation.TrimEnd(), 
        model.Seed.ColYear.TrimEnd(), 
        model.Seed.OrchardId);

    if (!string.IsNullOrEmpty(model.Seed.GID))
    {
        reference += model.Seed.GID;
    }
    
    if (!string.IsNullOrEmpty(model.Seed.Clone))
    {
        reference += model.Seed.Clone;
    }
    
    return reference;
}