current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 0 down vote favorite

Here is my code to create a definition text from 3 arguments. Is there any way to make it work with better performance and look nicer?

 private const string IllegalCharacters = "\\" + "\"" + "'#,.[]{}" + " ";

 public static string CreateDefText(string mainId, string     secondaryId,    string extraText)
    {
        var sb = new StringBuilder();
        sb.Append(mainId);
        if (sb.Length > 0 && !string.IsNullOrEmpty(secondaryId))
        {
            sb.Append("_");
        }
        sb.Append(secondaryId);
        sb.Append(extraText);
        if (sb.Length > 0 && (char.IsNumber(sb[0]) || '+'.Equals(sb[0]) || '-'.Equals(sb[0])))
        {
            sb.Insert(0, "N");
        }
        foreach (var illegalChar in IllegalCharacters)
        {
            sb.Replace(illegalChar.ToString(CultureInfo.InvariantCulture), string.Empty);
        }
        return sb.ToString();
    }
share|improve this question
up vote 3 down vote accepted

Code Style

Logic section

Use blank lines to separate your code into logical sections

Instead of:

var sb = new StringBuilder();
sb.Append(mainId);
if (sb.Length > 0 && !string.IsNullOrEmpty(secondaryId))
{
   sb.Append("_");
}
sb.Append(secondaryId);
sb.Append(extraText);

You can write:

var sb = new StringBuilder();   // section of object creation

sb.Append(mainId); // section of appending mainID

// section of checking for mainID and secondaryID
if (sb.Length > 0 && !string.IsNullOrEmpty(secondaryId))
{
   sb.Append("_");
}

sb.Append(secondaryId);
sb.Append(extraText);

The same for the rest part.

Long condition in if statement

Don't use long condition in if statement. It's not readable.

Instead of:

if (sb.Length > 0 && (char.IsNumber(sb[0]) || '+'.Equals(sb[0]) || '-'.Equals(sb[0])))

You can use:

isNum = char.IsNumber(sb[0]);
isPlus = sb[0].Equals('+');
isMinus = sb[0].Equals('-');

if (sb.Length > 0 && (isNum || isPlus || isMinus) 
{
   sb.Insert(0, "N");
}

We can move deeply and I want to get rid of this long if statement:

if (sb.Length > 0) 
{
   isNum = char.IsNumber(sb[0]);
   isPlus = sb[0].Equals('+');
   isMinus = sb[0].Equals('-');

   if (isNum || isPlus || isMinus) 
   {
      sb.Insert(0, "N");
   }
}

I still don't like this if construction, I'll extract this code into separate method. Before this I'll create an array of legal symbols

if (sb.Length > 0) 
{
   isNum = char.IsNumber(sb[0]);
   isLegalSymb = isLegalSymb(sb[0]);

   if (isNum || isLegalSymb) 
   {
    sb.Insert(0, "N");
   }
}

Separate method for checking if character is legal.

private static bool isLegalSymb(char symb) {
   string[] legalSymb = { "+", "-" };

   for (int i = 0; i < legalSymb.Length; i++) {
     if (symb.Equals(legalSymb[i])) {
       return true;
     }
   }

   return false;
}

Comments

Your code is not always obvious and other developers may don't understand anything without a context of this code.

In such situations you can provide more comments to make our life more easy.

It relates to your code:

foreach(var illegalChar in IllegalCharacters) {
  sb.Replace(illegalChar.ToString(CultureInfo.InvariantCulture), string.Empty);
}

I'm also confused with:

private const string IllegalCharacters = "\\" + "\"" + "'#,.[]{}" + " ";

I'll again extract this code into separate method:

replaceAllBannedCharacters(sb);

Separate method for replacing all banned symbols:

private static void replaceAllBannedCharacters(StringBuilder sb) {
  string[] bannedSymb = { "#", "'", ".", "[", "]", "{", "}", "," };

  for (int i = 0; i < bannedSymb.Length; i++) {
    sb.Replace(bannedSymb[i], string.Empty);
  }
}

Naming

mainId can be replaced with firstID

secondaryId can be replaced with secondID

share|improve this answer
    
I have only one obligation. The name of the method isLegalSymb() should be changed to something like IsIllegalStartSymbol‌​(), because we are checking the illegal symbols in this case. – Endery Jun 3 at 9:54
    
@Endery, firstly I used this method name, but it seems very difficult to read a lot of 'lll'', that's why I've changed it to another name. – martinShil Jun 3 at 10:00
    
I should confess that I have never thought like that. Interesting point. – Endery Jun 3 at 10:08
up vote 1 down vote

I found this to be rather unclear, with a surprising amount of logic in relatively few lines.

Take for instance IllegalCharacters: I kind of see the point of constructing it like that, but at the same time it feels clumsy and counter-intuitive. I'd be more inclined to construct it like this:

private static readonly char[] IllegalCharacters = { '\\', '"', '\'', '#', ',', '.', '[', ']', '{', '}', ' ' };

I'd split up CreateDefText into smaller methods; certainly the last part deserves its own method:

private static string Clean(StringBuilder sb)
{
    foreach (var illegalChar in IllegalCharacters)
    {
        sb.Replace(illegalChar.ToString(CultureInfo.InvariantCulture), string.Empty);
    }
    return sb.ToString();
}

Same for the test of the first character:

private static bool IsN(char startChar)
{
    return char.IsNumber(startChar) || '+'.Equals(startChar) || '-'.Equals(startChar);
}

So what's left are the shenanigans with the StringBuilder. Which seem over-complicated and make it hard to determine the logic. Here's my solution:

public static string CreateDefText(string mainId, string secondaryId, string extraText)
{
    var items = new List<string>();
    if (!string.IsNullOrEmpty(mainId))
    {
        items.Add(mainId);
    }
    if (!string.IsNullOrEmpty(secondaryId))
    {
        items.Add(secondaryId);
    }

    var result = string.Empty;
    if (items.Any())
    {
        result = string.Join("_", items);
    }

    if (!string.IsNullOrEmpty(result) && IsN(result[0]))
    {
        result = "N" + result;
    }

    result += extraText;

    return Clean(new StringBuilder(result));
}

Not that I'm too happy with this. Naming is still bad, and I'd prefer to move the various parts into their own methods, so it is clearer what happens.

share|improve this answer
    
Join() on an empty list gives string.Empty' so we can just use result = string.Join("_", items);` – AlanT Jun 3 at 9:24
    
I liked the suggestion to split up the method into smaller methods, I will definitely do it. But the answer from @martin-shil is covering more issues, so I will accept that one. Please do not be upset. – Endery Jun 3 at 9:39
    
@AlanT Ah yes, true. – BCdotWEB Jun 3 at 9:53

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.