Code evaluation

[fasil]

Would anyone here please evaluate my code? Is there something I am doing terribly wrong?

 

using System;

using System.Collections;

using MySql;

using MySql.Data;

using MySql.Data.MySqlClient;

 

public class ProductData

{

private int _ProductId;

private string _ProductName;

private decimal _ProductPrice;

 

//Properties

 

public ProductData()

{

}

 

public int ProductId

{

get { return _ProductId;}

set { _ProductId = value;}

}

public string ProductName

{

get { return _ProductName;}

set { _ProductName = value;}

}

public decimal ProductPrice

{

get { return _ProductPrice;}

set { _ProductPrice = value;}

}

 

}

 

/// <summary>

/// Summary description for Product.

/// </summary>

public class Product

{

public Product()

{

}

 

public ArrayList GetProductSearchLst(string SoegStreng)

{

String strSql = "SELECT id as product_id, Name as product_name, Price as product_price FROM product WHERE NAME LIKE '%"+ SoegStreng + "%' ORDER BY NAME;";

DbConnector dbCon = new DbConnector();

MySqlDataReader rdr = dbCon.DbConnect(strSql);

 

ArrayList arrLstProduct = new ArrayList();

 

while (rdr.Read())

{

ProductData p = new ProductData();

p.ProductId = Convert.ToInt32(rdr["product_id"]);

p.ProductName = rdr["product_name"].ToString();

p.ProductPrice = Convert.ToDecimal(rdr["product_price"]);

arrLstProduct.Add(p);

}

dbCon.DbClose();

return arrLstProduct;

}

 

public ArrayList GetProduct(int ProductId)

{

String strSql = "SELECT id as product_id, Name as product_name, Price as product_price FROM product WHERE ID = "+ ProductId + "";

DbConnector dbCon = new DbConnector();

MySqlDataReader rdr = dbCon.DbConnect(strSql);

 

ArrayList arrLstProduct = new ArrayList();

 

ProductData p = new ProductData();

p.ProductId = Convert.ToInt32(rdr["product_id"]);

p.ProductName = rdr["product_name"].ToString();

p.ProductPrice = Convert.ToDecimal(rdr["product_price"]);

arrLstProduct.Add(p);

 

dbCon.DbClose();

return arrLstProduct;

}

}

[PlausiblyDamp]

Is there a problem with the code as it stands or are you just after (hopefully constructive) criticism of the code so far?

Not had chance to have a look to see if there are any errors but the immediate concern I would have is the use of string concatenation to build your SQL - you should look at using stored procedures or at the very least using parameterised SQL. Search these forums for more advice / examples of both techniques.

Edited April 3, 2007 by PlausiblyDamp

[fasil]

Glad for some inputs. The coce is fully functional, and yes I am familiar with SQL server Stored Procedures. But I am using MySql 4.0 and therefore I can only make use of 'inline' sql querys and concatenations

[fasil]

Maybe someone can also tell me why _variables are so popular....

[Derek Stone]

There are numerous things wrong with this block of code:

 

• The application is not layered correctly.
  
• Entity names should reflect the entity, nothing more (drop the "Data" off of "ProductData"; rename "Product" to "Products").
  
• A glaring SQL injection vulnerability is present (as mentioned by PlausiblyDamp).
  
• No validation is done at the property accessor level. You could easily set "ProductPrice" to -5, which makes little to no sense.
  
• Property names on business objects shouldn't include the name of the business object (change "ProductID" to "ID", etc.).
  
• Unnecessary unboxing is performed on the database fields.
  
• The collections are not strongly typed.
  
• The readers are never closed.
  

[fasil]

I am very glad for the information.

Actually I am quite new i c# and .net, and I am very familiar with some of the errors u have mentioned. I it wouldn't be so much for u, can u post back your version of this code. I will be really glad, if you also put in some of the above mentioned flaws as comments in your corrected version. I thought I had layered it correctly, but that is why I want to listen from the real experts in here, and get my code evaluated.

[fasil]

Actually I close my reader in my DBConnector class, maybe it should also be evaluated here.

-----------------------------------------------------------

 

using MySql;

using MySql.Data;

using MySql.Data.MySqlClient;

using System.Data;

 

 

/// <summary>

/// Summary description for dbConnector.

/// Datbaseconnector

/// </summary>

 

public class DbConnector

{

private const string strConn = "Database=ecommerce;Data Source=localhost;User Id=root;Password=skywalker";

 

private MySqlConnection _connection;

 

protected MySqlConnection SqlConnection

{

get

{

if(_connection == null)

_connection = new MySqlConnection(strConn);

return _connection;

}

}

 

 

public MySqlDataReader DbConnect(string strSql)

{

MySqlConnection SqlConnection = new MySqlConnection(strConn);

SqlConnection.Open();

 

MySqlCommand sel = new MySqlCommand(strSql, SqlConnection);

MySqlDataReader rdr = sel.ExecuteReader(CommandBehavior.CloseConnection);

return rdr;

}

 

public void DbClose()

{

SqlConnection.Close();

}

}

[michael_hk]

In addition to Derek Stone's comment, one more point is there is no error handling.

[fasil]

I am very glad for all the input. But could someone at least correct my code in such a fashion it is flawless, and put in some comments on why such code is better.

[bri189a]

There are numerous things wrong with this block of code:

 

• Unnecessary unboxing is performed on the database fields.
  

 

What were you referring to?:

 

p.ProductId = Convert.ToInt32(rdr["product_id"]);

 

I would do:

 

p.ProductId = (int) rdr["product_id"];

 

because otherwise you get a compile error, but my terminology on boxing is the best so I'm just trying to understand what you meant.

[fasil]

What were you referring to?:

 

p.ProductId = Convert.ToInt32(rdr["product_id"]);

 

I would do:

 

p.ProductId = (int) rdr["product_id"];

 

because otherwise you get a compile error, but my terminology on boxing is the best so I'm just trying to understand what you meant.

 

Then what about...

 

p.ProductName = rdr["product_name"].ToString();

p.ProductPrice = Convert.ToDecimal(rdr["product_price"]);

[bri189a]

p.ProductName = (string) rdr["product_name"];

p.ProductPrice = (string) rdr["product_price"];

 

That just how I typically do it. I'm not sure but I think .ToString() and Convert.ToWhatever() creates a new instance of the object type rather than casting the object itself. I could be totally wrong on that though...one of the guys who's more familiar with terminology and might be able to better answer that part.

[fasil]

But what about the last one which is a decimal.

 

I tried (decimal) rdr["product_price"];

There was no compilation error but it gave an error when calling the page.

[Mister E]

Well, you're going to get an error if the contents of the "product_price" field can't be cast into a decimal. If the incoming data has the chance of being malformed, you should isolate the code in its own try/catch block and handle any errors appropriately.