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 have a little piece of code that is a little bit unoptimized and need some help with optimization in this case.

The procedure is to search in a database using "Conditions/Condicao" and create a grid using tables in HTML with content. In debug, the following part is taking a long time to complete with 300 results or more.

for (x = 0; x < Campos.Length; x++)
{
    string concat = "";
    for (i = 0; i < 2; i++)
    {
        if (concat != "")
        {
            concat += "," + oDrc[y].ItemArray[1].ToString().Trim().Replace("'", "");
        }
        else
        {
            concat += oDrc[y].ItemArray[0].ToString().Trim().Replace("'", "");
        }
    }
    if (!pkunica)
    {
        concat = "";
        for (i = 0; i < Pks.Length; i++)
        {
            if (concat != "")
            {
                concat += "," + oDrc[y].ItemArray[i].ToString().Trim().Replace("'", "");
            }
            else
            {
                concat += oDrc[y].ItemArray[i].ToString().Trim().Replace("'", "");
            }
        }
    }

I've uploaded the full code to Pastebin. And about some words, they're in PT-BR.

share|improve this question
5  
Welcome to Code Review! Please include a description of what your code does. Titles should also describe what your code does. Everyone is here to optimize or clean up their code in some way or another. –  RubberDuck 12 hours ago

4 Answers 4

Use string.Empty instead of "" to indicate an empty string. It conveys your intent with ambiguity (i.e.: nobody will think you forgot to enter something in the string).


When concatenating in a loop, use a StringBuilder.


Use meaningful names. I don't know what concat and oDrc actually mean. Nor pkunica, but that might be a language-issue (I'd encourage you to stick to English-only).

share|improve this answer
    
Yeah, mistake in the else. I don't know what I was thinking. –  Jeroen Vannevel 12 hours ago

Your first loop is unnecessary, you add more code, and more complexity, by making it a loop, and remove nothing. A loop where the loop control variable is unused should be suspect.

You are repeating yourself, and in the process making your code harder to read.

if (concat != "")
        {
            concat += "," + oDrc[y].ItemArray[i].ToString().Trim().Replace("'", "");
        }
        else
        {
            concat += oDrc[y].ItemArray[i].ToString().Trim().Replace("'", "");
        }

Should be:

if (!string.IsEmptyOrNull(concat))
{
    concat += ","; // except using a stringbuilder
}
concat += oDrc[y].ItemArray[i].ToString().Trim().Replace("'", "");

It is then clear that the action is the same.

Always try to do as much as possible in the common code, and not duplicate code between branches.

share|improve this answer

When concatenating inside of loops, you should definitely use a StringBuilder.

Also, your program flow is all over the place. You're executing code and then erasing the results based on an if statement. Why execute that code at all then?

Also, your code snippet doesn't fully work. What is y here? Another loop index?

Finally, oDrc[y].ItemArray[0].ToString().Trim().Replace("'", "") is very duplicated.

StringBuilder concat = new StringBuilder();

if (pkunica)
{
        concat.Append(GetReadableString(oDrc,y,0));
        concat.Append(GetReadableString(oDrc,y,1));
}
else
{
    if(Pks.Length > 0)
    {
        concat.Append(GetReadableString(oDrc,y,0));
    }
    for (i = 1; i < Pks.Length; i++)
    {
        concat.Append("," + GetReadableString(oDrc,y,i));
    }
}
string result = concat.ToString();


private string GetReadableString(ODrc oDrc, int oDrcIndex, int itemArrayIndex) //Using a guess here because I have no idea what type any of that is
{
    return oDrc[oDrcIndex].ItemArray[itemArrayIndex].ToString().Trim().Replace("'", "")
}
share|improve this answer
    
Stringbuilder in C# doesn't have a AppendString() method. Did you mean AppendLine() ? –  Heslacher 11 hours ago
    
Nope, just .Append() I think. Clearly my brain is not with it. But then it's 5PM where I am :) –  Nick Udell 10 hours ago
    
Thanks, i'll read more about the Stringbuilder. And hm, i have uploaded the full code in pastebin, can you check it out? :) –  Eduardo Calazans Júnior 9 hours ago

(Given how your code looks and the fact you are talking about database searches, I will assume oDrc[y] is a DataRow object)

If you are using .NET 4, you can use LINQ and string.Join:

Using the following helper methods:

private static string GetRowString(DataRow row, int count)
{
    var rowItems = row.ItemArray.Take(count);
    return string.Join(",", rowItems.Select(GetReadableString));
}

private static string GetReadableString(object item)
{
    return item.ToString().Trim().Replace("'", string.Empty);
}

your new method body becomes:

var count = pkunica ? 2 : Pks.Length;
var concat = GetRowString(oDrc[y], count);
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.