4
\$\begingroup\$

The following code grabs the HTML-encoded, multi-line content of a text field. It then splits the content on line breaks, and finally displays it with commas as separators.

I would like to have this code reviewed to see if there is a simpler way to grab the string, split and display it.

Note - I hated the idea of writing 2 collection iterations, but I found if a user in the CMS entered one line of real data, and then a couple of empty line breaks, it would throw the array length off and display a comma on a single item e.g. output should look like this :

  • Correct: Allowed Uses: example 1 | Allowed Uses: example 1, example 2
  • Incorrect: Allowed Uses: example 1, | Allowed Uses: example 1, example 2,
  var retailBreaks = umbraco.library.ReplaceLineBreaks(Model.landAllowedUses);
  string[] retailBreaksSplit = retailBreaks.Split(new string[] { "<br/>" }, StringSplitOptions.RemoveEmptyEntries);
  int breaksLength = 0;
  List<String> repeatableProperty = new List<String>();
  foreach (string s in retailBreaksSplit)
  {
    if (!string.IsNullOrWhiteSpace(s))
       {
         repeatableProperty.Add(s);
       }
  }
 breaksLength = repeatableProperty.Count();

 <div>
    <strong>Allowed Uses:</strong>

    @foreach (string s in repeatableProperty)
    {
     <span class="comma-list">@s.Trim()@((breaksLength > 1 && repeatableProperty.Last<string>() != s) ? ", " : string.Empty)  </span>
    }
</div>
\$\endgroup\$
2
\$\begingroup\$

Just do a string replace to swap the html breaks for commas, then trim the end to remove any extra commas:

retailBreaks.Replace("<br/>",",").TrimEnd(',')
\$\endgroup\$
  • 1
    \$\begingroup\$ Love the simplicity! \$\endgroup\$ – user3267755 Sep 17 '15 at 16:09
2
\$\begingroup\$

You already have got a nice and simple solution from @GavinCoates so I only would like to add a couple of comments about your code for your future coding experience.

  • You should consequently use either full types for your variables or the var keyword. In your code the var would greatly improve the readabilty as all types are obvious here and don't need to declared twice:

    var retailBreaks = umbraco.library.ReplaceLineBreaks(Model.landAllowedUses);
    var retailBreaksSplit = retailBreaks.Split(new string[] { "<br/>" }, StringSplitOptions.RemoveEmptyEntries);
    var breaksLength = 0;
    var repeatableProperty = new List<String>();
    
  • The same applies to the foreach-loops:

    foreach (var s in retailBreaksSplit)
    
  • You started using good variable names but didn't continue in your loops. This would look and read mutch better:

    foreach (var retailBreak in retailBreaksSplit)
    
  • If you have collections or lists etc. you should name the variables using plural form to indicate it's more then one element. This would be a better name for the list:

    var repeatableProperties = new List<String>();
    
  • The final thing I would change is the last loop. I would create a helper variable for the text instead of putting it inside the span. It's been long time since I last coded with ASP.NET so please forgive me if I got the syntax wrong.

    @foreach (var repeatableProperty in repeatableProperties)
    {
         var text = @((breaksLength > 1 && repeatableProperty.Last<string>() != s) ? ", " : string.Empty);
         <span class="comma-list">@s.Trim()@text </span>
    }
    
\$\endgroup\$

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.