Critique My SQL

J

Jonathan Wood

I'm writing a Website that contains the following code:

// Query article data
SqlDataAdapter adap = new SqlDataAdapter("Select art_name, art_date,
art_desc, auth_name," +
" '" + ResolveUrl("~/") + "Articles/' + grp_folder + '/' + cat_folder + '/'
+ art_filename + '.aspx' AS art_path" +
" FROM Articles, Authors, Categories, Groups" +
" WHERE cat_id = '" + id.ToString() + "' AND art_auth_id = auth_id AND
art_cat_id = cat_id AND cat_grp_id = grp_id" +
" ORDER BY art_name", conn);
DataSet ds = new DataSet();
adap.Fill(ds);

// Assign datasource to grid
GridView1.DataSource = ds;
GridView1.DataBind();

It's working correctly but I'm new to this and have a couple of questions.

1. Does my SQL statement seem reasonably efficient and correctly formed? Or
am I asking for performance problems with a page that renders with complex
queries?

2. How can I find out if no rows were returned?

Thanks!
 
R

RobinS

You need to put that query in a stored procedure. You are opening yourself
up to SQL Injection type hacking.

You can check ds.tables(0).Rows.Count to see if you got any results.

Robin S.
 
D

David Browne

Jonathan Wood said:
I'm writing a Website that contains the following code:

// Query article data
SqlDataAdapter adap = new SqlDataAdapter("Select art_name, art_date,
art_desc, auth_name," +
" '" + ResolveUrl("~/") + "Articles/' + grp_folder + '/' + cat_folder +
'/' + art_filename + '.aspx' AS art_path" +
" FROM Articles, Authors, Categories, Groups" +
" WHERE cat_id = '" + id.ToString() + "' AND art_auth_id = auth_id AND
art_cat_id = cat_id AND cat_grp_id = grp_id" +
" ORDER BY art_name", conn);
DataSet ds = new DataSet();
adap.Fill(ds);

// Assign datasource to grid
GridView1.DataSource = ds;
GridView1.DataBind();

It's working correctly but I'm new to this and have a couple of questions.

1. Does my SQL statement seem reasonably efficient and correctly formed?
Or am I asking for performance problems with a page that renders with
complex queries?
Click to expand...
No. Or rather, who can tell when you splice together C# and SQL like that.
Not only is it ugly and hard to code, it's slow (causes compilations) and
dangerous (risks SQL injection). Other SQL problems in your queries are
failure to use ANSI join syntax, and failure to alias tables and qualify
columns by aliases, and doing concatenation in the query (which forces you
to paste in the URL).


string SQL = @"
SELECT
a.art_name,
a.art_date,
a.art_desc,
au.auth_name,
grp.grp_folder,
cat.cat_folder,
a.art_filename,
FROM
Categories cat
join Articles a
on a.art_cat_id = cat.cat_id
join Authors au
on a.art_auth_id = au.auth_id
Groups grp
on grp.grp_id = cat.cat_grp_id
join
WHERE cat_id = @cat_id
ORDER BY art.art_name
";

SqlCommand cmd = new SqlCommand(SQL,con);
cmd.Parameters.Add(@cat_id, id);
SqlDataAdapter adap = new SqlDataAdapter(cmd);
....


Then you can add a calculated column to your DataTable for art_path or add
it in your grid.
2. How can I find out if no rows were returned?
Click to expand...

Check ds.Tables[0].Rows.Count

David
 
J

Jebblue

RobinS said:
You need to put that query in a stored procedure. You are opening yourself
up to SQL Injection type hacking.

You can check ds.tables(0).Rows.Count to see if you got any results.

Robin S.
----------------------------
Click to expand...

Please don't top post.

You could just use a parameterized query to gain performance and avoid SQL
injection.
 
O

Otis Mukinfus

I'm writing a Website that contains the following code:

// Query article data
SqlDataAdapter adap = new SqlDataAdapter("Select art_name, art_date,
art_desc, auth_name," +
" '" + ResolveUrl("~/") + "Articles/' + grp_folder + '/' + cat_folder + '/'
+ art_filename + '.aspx' AS art_path" +
" FROM Articles, Authors, Categories, Groups" +
" WHERE cat_id = '" + id.ToString() + "' AND art_auth_id = auth_id AND
art_cat_id = cat_id AND cat_grp_id = grp_id" +
" ORDER BY art_name", conn);
DataSet ds = new DataSet();
adap.Fill(ds);

// Assign datasource to grid
GridView1.DataSource = ds;
GridView1.DataBind();

It's working correctly but I'm new to this and have a couple of questions.

1. Does my SQL statement seem reasonably efficient and correctly formed? Or
am I asking for performance problems with a page that renders with complex
queries?

2. How can I find out if no rows were returned?

Thanks!
Click to expand...

Both previous answers are correct regarding avoidance of SQL injection attacks.

Since you say you are new at this I recommend you try to write your source code
so it is easier to follow (do some formatting for readability). A year from now
when you, or someone else has to read it for debugging or modification purposes,
you will be happy you did.

It is not my intention to flame you. I'm just trying to be helpful.

Good luck with your project,

Otis Mukinfus

http://www.otismukinfus.com
http://www.arltex.com
http://www.tomchilders.com
http://www.n5ge.com
 
J

Jonathan Wood

Thanks for the comments.

--
Jonathan Wood
SoftCircuits Programming
http://www.softcircuits.com

David Browne said:
Jonathan Wood said:
I'm writing a Website that contains the following code:

// Query article data
SqlDataAdapter adap = new SqlDataAdapter("Select art_name, art_date,
art_desc, auth_name," +
" '" + ResolveUrl("~/") + "Articles/' + grp_folder + '/' + cat_folder +
'/' + art_filename + '.aspx' AS art_path" +
" FROM Articles, Authors, Categories, Groups" +
" WHERE cat_id = '" + id.ToString() + "' AND art_auth_id = auth_id AND
art_cat_id = cat_id AND cat_grp_id = grp_id" +
" ORDER BY art_name", conn);
DataSet ds = new DataSet();
adap.Fill(ds);

// Assign datasource to grid
GridView1.DataSource = ds;
GridView1.DataBind();

It's working correctly but I'm new to this and have a couple of
questions.

1. Does my SQL statement seem reasonably efficient and correctly formed?
Or am I asking for performance problems with a page that renders with
complex queries?
Click to expand...
No. Or rather, who can tell when you splice together C# and SQL like
that. Not only is it ugly and hard to code, it's slow (causes
compilations) and dangerous (risks SQL injection). Other SQL problems in
your queries are failure to use ANSI join syntax, and failure to alias
tables and qualify columns by aliases, and doing concatenation in the
query (which forces you to paste in the URL).


string SQL = @"
SELECT
a.art_name,
a.art_date,
a.art_desc,
au.auth_name,
grp.grp_folder,
cat.cat_folder,
a.art_filename,
FROM
Categories cat
join Articles a
on a.art_cat_id = cat.cat_id
join Authors au
on a.art_auth_id = au.auth_id
Groups grp
on grp.grp_id = cat.cat_grp_id
join
WHERE cat_id = @cat_id
ORDER BY art.art_name
";

SqlCommand cmd = new SqlCommand(SQL,con);
cmd.Parameters.Add(@cat_id, id);
SqlDataAdapter adap = new SqlDataAdapter(cmd);
...


Then you can add a calculated column to your DataTable for art_path or add
it in your grid.
2. How can I find out if no rows were returned?
Click to expand...

Check ds.Tables[0].Rows.Count

David
Click to expand...
 
J

Jonathan Wood

Otis,
Since you say you are new at this I recommend you try to write your source
code
so it is easier to follow (do some formatting for readability). A year
from now
when you, or someone else has to read it for debugging or modification
purposes,
you will be happy you did.
Click to expand...

I'm new to C#, ASP.NET, and haven't worked that much with SQL. But I've been
programming for 20 years.

Other than the complexity of the query, which would be substantially
improved if I moved it to a stored procedure, I'm not sure what your
complaint is about the formatting. (Note that additional line breaks were
inserted in the newsgroup post so it may look differently there than in my
code.)
Good luck with your project,
Click to expand...

Thanks.
 
Top