views:

168

answers:

3

Hi,

I need to append an Sqlquery for each row in a datatable.I have 36 columns and based on the datatype of each column i need to append sqlquery.Can anyone suggest me the effective way to do it.Is it bad way of coding to use " + " operator to append text in between the append ?

Following is my code.

 query ="INSERT INTO MASTERPI (tag,instrumenttag)");
 query += "VALUES ('" + createTagRow["tag"].ToString() + "','" + createTagRow["instrumenttag"].ToString() + "'");

Thanks, Vix

A: 

Your code would probably be more readable if you used String.Format to build each line of the query, making use of its placeholder syntax to interpolate the data from your row object into the query text.

pmarflee
Performance-wise, that's not much better than concatenation. You're still creating a bunch of throwaway `string` instances instead of effectively using the `StringBuilder`.
Aaronaught
I'd suggest that you also create an Extension Method to handle the string concatenation, merely from a code readability perspective in conjunction with the above answer.
Ahmad
@Ahmad: What does an extension method accomplish here?? I wish I could downvote comments.
Aaronaught
Why not use StringBuilder.AppendFormat()? Its not performance wiser than string.Format(), but at least does not create throw away strings.
BeowulfOF
+5  A: 

If you're already using a StringBuilder then it's detrimental to use regular string concatenation at the same time. You're negating half of the utility of the StringBuilder class. Don't do it. Use the StringBuilder.Append method exclusive and get rid of those + statements.

I'm also thinking that all those createTagRow(...).ToString() calls are wasteful. Something in there is doing the work of serializing those elements to a string, so you're effectively doing the work twice, creating the string and then appending it. If it's possible for you to pass the StringBuilder itself to those createTagRow calls, that would also be a lot less... scary.

Actually, on a second look-over, it seems that this code is building a SQL query. Parameterize that query NOW. No excuses. That way you won't even need to worry about string.Format vs. StringBuilder vs. concatenation, the DB library will handle it all for you.

Aaronaught
+1 for parameterising the query - anything else is asking for trouble
Will
A: 

As already mentioned you should consider two possibilities:

  1. Aaronaught: Parameterize that query
  2. If you need full control over the statement string, you should take a look into .FormatWith extension and refactor your different blocks into classes, which give the needed parts back.

A little example:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;
using System.Web.UI;

namespace ConsoleApplication1
{
    class Program
    {
        static void Main(string[] args)
        {
            var columns = new CommaSeparated();

            columns.Add("tag");
            columns.Add("instrumenttag");
            columns.Add("pointsource");
            columns.Add("pointtype");
            columns.Add("dataowner");
            columns.Add("dataaccess");
            columns.Add("location1");
            columns.Add("location2");
            columns.Add("location3");
            columns.Add("location4");
            columns.Add("location5");
            columns.Add("...");


            var values = new CommaSeparated();

            values.EncloseEachElementWith = "'";
            values.Add(createTagRow["tag"].ToString());
            values.Add(createTagRow["instrumenttag"].ToString());
            values.Add(createTagRow["pointsource"].ToString());
            values.Add(createTagRow["pointtype"].ToString());
            values.Add(createTagRow["dataowner"].ToString());
            values.Add(createTagRow["dataaccess"].ToString());
            values.Add(createTagRow["location1"].ToString());
            values.Add(createTagRow["location2"].ToString());
            values.Add(createTagRow["location3"].ToString());
            values.Add(createTagRow["location4"].ToString());
            values.Add(createTagRow["location5"].ToString());
            values.Add(createTagRow["..."].ToString());

            //INSERT INTO MASTERPI ({columns}) values ({values})
            var query = "INSERT INTO MASTERPI ({columns}) values ({values})".FormatWith(new { columns, values });

            Console.WriteLine(query);
            Console.ReadKey();
        }
    }

    public class CommaSeparated : List<string>
    {
        public CommaSeparated()
            : base()
        {
            EncloseEachElementWith = String.Empty;
        }

        public override string ToString()
        {
            var elements = this.Select(element => String.Format("{0}{1}{0}", EncloseEachElementWith, element));

            return String.Join(", ", elements.ToArray());
        }

        public string EncloseEachElementWith { get; set; }
    }

    public static class StringExtensions
    {
        public static string FormatWith(this string format, object source)
        {
            return FormatWith(format, null, source);
        }

        public static string FormatWith(this string format, IFormatProvider provider, object source)
        {
            if (format == null)
                throw new ArgumentNullException("format");

            Regex r = new Regex(@"(?<start>\{)+(?<property>[\w\.\[\]]+)(?<format>:[^}]+)?(?<end>\})+",
              RegexOptions.Compiled | RegexOptions.CultureInvariant | RegexOptions.IgnoreCase);

            List<object> values = new List<object>();
            string rewrittenFormat = r.Replace(format, delegate(Match m)
            {
                Group startGroup = m.Groups["start"];
                Group propertyGroup = m.Groups["property"];
                Group formatGroup = m.Groups["format"];
                Group endGroup = m.Groups["end"];

                values.Add((propertyGroup.Value == "0")
                  ? source
                  : DataBinder.Eval(source, propertyGroup.Value));

                return new string('{', startGroup.Captures.Count) + (values.Count - 1) + formatGroup.Value
                  + new string('}', endGroup.Captures.Count);
            });

            return string.Format(provider, rewrittenFormat, values.ToArray());
        }
    }
}
Oliver
I'm trying to use string += like this :query ="INSERT INTO MASTERPI (tag,instrumenttag)"); query += "VALUES ('" + createTagRow["tag"].ToString() + "','" + createTagRow["instrumenttag"].ToString() + "'");Is it efficient ??
vix
To the question if it is efficient, it is very hard to read. So it is inefficient regarding to the maintainability. My approach is also not quite perfect, cause both lists have to be in the same order. This could be achieved by using some kind of Tuple<T1, T2>, but this can by solved by the questioner itself. It should only show another approach to solve this problem.
Oliver