ARCHITETURAL QUESTION

  • Thread starter Thread starter Bruno Rodrigues
  • Start date Start date
B

Bruno Rodrigues

Hi all

I'm using the following code standart in class
libraries. I will not split this function in more layers,
but I want to know if there are some architetural
improvement to do in my code. E.g: Database connection;
Try/Catch use; DataTable as a set of rows; etc...
Any suggestions are welcome.

Thanks!
Bruno Rodrigues

==========================================================

using System;
using System.Data;
using System.Data.SqlClient;

namespace CodeDesign
{
/// <summary>
/// Test class for code design.
/// </summary>
public class Test
{
SqlConnection conn = new SqlConnection
("Persist Security Info=False;Integrated
Security=False;database=Test;server=Computer;Connect
Timeout=30;User ID=sa;Password=sa;");

private void openDataBase()
{
try
{
conn.Open();
}
catch(Exception e)
{
throw new Exception("An
error ocurred while opening the database.\n" + e.Message,
e);
}
}

private void closeDataBase()
{
try
{
conn.Close();
}
catch(Exception e)
{
throw new Exception("An
error ocurred while closing the database.\n" + e.Message,
e);
}
}

/// <summary>
/// Retrieves a specific record.
/// </summary>
/// <param name="id">Record ID.</param>
/// <returns>DataRow with record's
data.</returns>
public DataRow Return(int id)
{
openDataBase();

string sql = "SELECT * FROM Tests
WHERE PK = " + id.ToString();
SqlDataAdapter adapter = new
SqlDataAdapter(sql, conn);
DataTable table = new DataTable();

try
{
adapter.Fill(table);
return table.Rows[0];
}
catch(Exception e)
{
throw new Exception("An
error ocurred while retrieving the records.\n" +
e.Message, e);
}
finally
{
closeDataBase();
}
}


/// <summary>
/// Retrieves all records.
/// </summary>
/// <returns>DataTable with all
records.</returns>
public DataTable ReturnAll()
{
string sql = "SELECT * FROM
Tests";
SqlDataAdapter adapter = new
SqlDataAdapter(sql, conn);
DataTable table = new DataTable();

try
{
adapter.Fill(table);
return table;
}
catch(Exception e)
{
throw new Exception("An
error ocurred while retrieving all records.\n" +
e.Message, e);
}
finally
{
closeDataBase();
}
}

/// <summary>
/// Returns the number of records.
/// </summary>
/// <returns>Number of records.</returns>
public int Count()
{
string sql = "SELECT Count(PK) AS
Total FROM Tests";
SqlCommand command = new
SqlCommand(sql, conn);

try
{
return Convert.ToInt32
(command.ExecuteScalar());
}
catch(Exception e)
{
throw new Exception("An
error ocurred while counting records.\n" + e.Message, e);
}
finally
{
closeDataBase();
}
}

/// <summary>
/// Delete a record.
/// </summary>
/// <param name="id">Record ID.</param>
public void Delete(int id)
{
string sql = "DELETE FROM Tests
WHERE PK = " + id.ToString();
SqlCommand command = new
SqlCommand(sql, conn);

try
{
command.ExecuteNonQuery();
}
catch(Exception e)
{
throw new Exception("An
error ocurred while deleting a record.\n" + e.Message, e);
}
finally
{
closeDataBase();
}
}
}
}
 
see http://www.microsoft.com/resources/practices/completelist.asp

--
-------------------------
"Manish Agarwal"- <[email protected]>

Bruno Rodrigues said:
Hi all

I'm using the following code standart in class
libraries. I will not split this function in more layers,
but I want to know if there are some architetural
improvement to do in my code. E.g: Database connection;
Try/Catch use; DataTable as a set of rows; etc...
Any suggestions are welcome.

Thanks!
Bruno Rodrigues

==========================================================

using System;
using System.Data;
using System.Data.SqlClient;

namespace CodeDesign
{
/// <summary>
/// Test class for code design.
/// </summary>
public class Test
{
SqlConnection conn = new SqlConnection
("Persist Security Info=False;Integrated
Security=False;database=Test;server=Computer;Connect
Timeout=30;User ID=sa;Password=sa;");

private void openDataBase()
{
try
{
conn.Open();
}
catch(Exception e)
{
throw new Exception("An
error ocurred while opening the database.\n" + e.Message,
e);
}
}

private void closeDataBase()
{
try
{
conn.Close();
}
catch(Exception e)
{
throw new Exception("An
error ocurred while closing the database.\n" + e.Message,
e);
}
}

/// <summary>
/// Retrieves a specific record.
/// </summary>
/// <param name="id">Record ID.</param>
/// <returns>DataRow with record's
data.</returns>
public DataRow Return(int id)
{
openDataBase();

string sql = "SELECT * FROM Tests
WHERE PK = " + id.ToString();
SqlDataAdapter adapter = new
SqlDataAdapter(sql, conn);
DataTable table = new DataTable();

try
{
adapter.Fill(table);
return table.Rows[0];
}
catch(Exception e)
{
throw new Exception("An
error ocurred while retrieving the records.\n" +
e.Message, e);
}
finally
{
closeDataBase();
}
}


/// <summary>
/// Retrieves all records.
/// </summary>
/// <returns>DataTable with all
records.</returns>
public DataTable ReturnAll()
{
string sql = "SELECT * FROM
Tests";
SqlDataAdapter adapter = new
SqlDataAdapter(sql, conn);
DataTable table = new DataTable();

try
{
adapter.Fill(table);
return table;
}
catch(Exception e)
{
throw new Exception("An
error ocurred while retrieving all records.\n" +
e.Message, e);
}
finally
{
closeDataBase();
}
}

/// <summary>
/// Returns the number of records.
/// </summary>
/// <returns>Number of records.</returns>
public int Count()
{
string sql = "SELECT Count(PK) AS
Total FROM Tests";
SqlCommand command = new
SqlCommand(sql, conn);

try
{
return Convert.ToInt32
(command.ExecuteScalar());
}
catch(Exception e)
{
throw new Exception("An
error ocurred while counting records.\n" + e.Message, e);
}
finally
{
closeDataBase();
}
}

/// <summary>
/// Delete a record.
/// </summary>
/// <param name="id">Record ID.</param>
public void Delete(int id)
{
string sql = "DELETE FROM Tests
WHERE PK = " + id.ToString();
SqlCommand command = new
SqlCommand(sql, conn);

try
{
command.ExecuteNonQuery();
}
catch(Exception e)
{
throw new Exception("An
error ocurred while deleting a record.\n" + e.Message, e);
}
finally
{
closeDataBase();
}
}
}
}
Click to expand...
 
Well One thing is to Make all of you SQL strings constants. -
with good names. -
In fact I place any strings into Constants in all programs. - or a
database.- but the SQL should be a constant
 
Bruno,

There are a number of issues here:

- Your class does not implement IDisposable. Because you hold other
resources on the class level which implement IDisposable, your class should
do the same.

- Your swallowing of the original exception and then re-throwing of a new
exception is a very, VERY bad idea IMO. I think that you should let the
original exception propogate. It's good that you are setting the inner
exception, but using a general Exception is very bad. You should create a
new Exception class specific to the error you are getting. So if you have
an error opening the database, you should throw an OpenDatabaseException, or
something of the sort. Eric Gunnerson wrote a good article about using
type-specific exceptions vs a value on the exception itself.

- You should be using stored prcoedures for all of your queries. If you
can't use stored procedures, at the least, use parameterized queries (which
you might or might not prepare).

- Instead of using openDatabase/closeDatabase and handling that yourself,
just pass the connection object to the command. It will open and close the
connection for you. This also reduces a lot of the error code that you have
(when the exception is thrown, you will know you have a problem opening the
connection from the internal exception, and your exception will indicate
where it happened).

- Implement Count as a property, not as a method. If implementing as a
method, then change the method name to GetCount, to indicate it is
performing some operation.

Hope this helps.

--
- Nicholas Paldino [.NET/C# MVP]
- (e-mail address removed)

Bruno Rodrigues said:
Hi all

I'm using the following code standart in class
libraries. I will not split this function in more layers,
but I want to know if there are some architetural
improvement to do in my code. E.g: Database connection;
Try/Catch use; DataTable as a set of rows; etc...
Any suggestions are welcome.

Thanks!
Bruno Rodrigues

==========================================================

using System;
using System.Data;
using System.Data.SqlClient;

namespace CodeDesign
{
/// <summary>
/// Test class for code design.
/// </summary>
public class Test
{
SqlConnection conn = new SqlConnection
("Persist Security Info=False;Integrated
Security=False;database=Test;server=Computer;Connect
Timeout=30;User ID=sa;Password=sa;");

private void openDataBase()
{
try
{
conn.Open();
}
catch(Exception e)
{
throw new Exception("An
error ocurred while opening the database.\n" + e.Message,
e);
}
}

private void closeDataBase()
{
try
{
conn.Close();
}
catch(Exception e)
{
throw new Exception("An
error ocurred while closing the database.\n" + e.Message,
e);
}
}

/// <summary>
/// Retrieves a specific record.
/// </summary>
/// <param name="id">Record ID.</param>
/// <returns>DataRow with record's
data.</returns>
public DataRow Return(int id)
{
openDataBase();

string sql = "SELECT * FROM Tests
WHERE PK = " + id.ToString();
SqlDataAdapter adapter = new
SqlDataAdapter(sql, conn);
DataTable table = new DataTable();

try
{
adapter.Fill(table);
return table.Rows[0];
}
catch(Exception e)
{
throw new Exception("An
error ocurred while retrieving the records.\n" +
e.Message, e);
}
finally
{
closeDataBase();
}
}


/// <summary>
/// Retrieves all records.
/// </summary>
/// <returns>DataTable with all
records.</returns>
public DataTable ReturnAll()
{
string sql = "SELECT * FROM
Tests";
SqlDataAdapter adapter = new
SqlDataAdapter(sql, conn);
DataTable table = new DataTable();

try
{
adapter.Fill(table);
return table;
}
catch(Exception e)
{
throw new Exception("An
error ocurred while retrieving all records.\n" +
e.Message, e);
}
finally
{
closeDataBase();
}
}

/// <summary>
/// Returns the number of records.
/// </summary>
/// <returns>Number of records.</returns>
public int Count()
{
string sql = "SELECT Count(PK) AS
Total FROM Tests";
SqlCommand command = new
SqlCommand(sql, conn);

try
{
return Convert.ToInt32
(command.ExecuteScalar());
}
catch(Exception e)
{
throw new Exception("An
error ocurred while counting records.\n" + e.Message, e);
}
finally
{
closeDataBase();
}
}

/// <summary>
/// Delete a record.
/// </summary>
/// <param name="id">Record ID.</param>
public void Delete(int id)
{
string sql = "DELETE FROM Tests
WHERE PK = " + id.ToString();
SqlCommand command = new
SqlCommand(sql, conn);

try
{
command.ExecuteNonQuery();
}
catch(Exception e)
{
throw new Exception("An
error ocurred while deleting a record.\n" + e.Message, e);
}
finally
{
closeDataBase();
}
}
}
}
Click to expand...
 
Back
Top