What's the right way to write my data access layer's methods?

Is this good enough ? do I need to add anything or remove anything ? like a rollback to my Sql Queries ? add catch() ? should my function accept only the property that I need or the object it self ? and how can I let the presentation layer know that the function's code was executed with no errors .. should I make it book instead of void or what ?

    public static void DeleteAllCabinFeaturesFromACruise(int CruiseID)
    {
        string commandText = "DELETE FROM Cruise_Features WHERE CruiseID = @cruiseId";
        SqlConnection connection = new SqlConnection(ConnectionString);
        SqlCommand command = new SqlCommand(commandText, connection);

        try
        {
            using (connection)
            {
                using (command)
                {
      开发者_StackOverflow              command.Parameters.AddWithValue("@cruiseId", CruiseID);
                    connection.Open();
                    command.ExecuteScalar();
                }
            }
        }

        finally { connection.Close(); }
    }

You are not using using correctly. The idea of using is to wrap some resource, that needs to be released in a safety-west, that protects it from exceptions. So, the correct way of using using (ha-ha) is the following:

using(SqlConnection connection = new SqlConnection(ConnectionString)){
{
    using(SqlCommand command = new SqlCommand(commandText, connection)){
        //your code here
    }
}

The second issue is that you are executing your query as if it should return Scalar value. It's OK, but I think it's better use just Execute: command.Execute(); And, if you want some error handling, you'd better wrap

connection.Open();
command.ExecuteScalar();

in a try ... catch block like you have. Like this:

//I would place it inside inner-most using block, but nothing wrong placing it outside
try{
    connection.open();
    command.Parameters.AddWithValue("@cruiseId", CruiseID);
     command.Execute();
}
//this catches ALL exceptions, regardless of source. Better narrow this down with
//some specific exception, like SQLException or something like that
catch (Exception e){
    return false; //or whatever you need to do
}

You can remove the try...finally block. The "using" will take care of disposing the connection for you.

EDIT: As chibacity says, you should initialise the connection in the using to ensure correct disposal. Your error-handling (to prevent the users seeing the exception details) should be done in your UI layer, not your data-layer. It is entirely appropriate for a data-layer to throw exceptions.

string commandText = "DELETE FROM Cruise_Features WHERE CruiseID = @cruiseId";

using (SqlConnection connection = new SqlConnection(ConnectionString))
{
    SqlCommand command = new SqlCommand(commandText, connection);
    // Your code.
}

For a simple query like that a rollback is probably not necessary - the query will either succeed or fail. For a more complicated query, a rollback would be necessary if you wanted to ensure that operations were atomic.

and how can I let the presentation layer know that the function's code was executed with no errors?

This really boils down to how you want to handle these cases in general. One rule of thumb is to ask yourself a question; are you expecting an error to occur significantly often, or not? So, for example, if the method should only fail if there was a communication error, then it's perfectly fine to throw an exception to the front-end. If, on the other hand, the delete operation can fall over because of various business rules that need to be validated (or, say, foreign key violations), then you should probably not throw those as exceptions. In the latter case, your method should return some information about success (simplistic version: a boolean, complex version: your own "OperationResult" class).

I'm simplifying things a lot here, but I believe these rules of thumb are valid.

EDIT: When I say "then it's perfectly fine to throw an exception to the front-end", I mean - throw it to the front end, but have the front-end handle it gracefully!

connection.Close() is redundant - using(connection) will dispose and close it. Also, connection.Open() is usually placed in the beginning - to verify connection with the server .