C# OOP - Return a string from a Business Object that performs Validation and Inserts a record - Good Practice or Not?

Just curious if someone can shed some light on if this is a good practice or not?

Currently I am working on a C# project that performs and Inserts a record and runs through 4 or 5 methods to validate that the record can be added and returns a string that tells the presentation layer if the record has been submitted or not.

Is this a good practice? Pros/Cons?

The call from the presentation is:

protected void btnProduct_Click(object sender, EventArgs e)
{

    lblProduct.Text =  ProductBLL.CreateProduct(txtProductType.Text, txtProduct.Text, Convert.ToInt32(txtID.Text);

}

The BLL method is:

public class AccountBLL
    {
        // Create The Product w/ all rules validated
        public static string CreateProduct(string prod开发者_开发知识库uctType, string product, int id)
    {

        // CHECK IF PRODUCT NAME IN DB                
        else if (ValidateIfProductNameExists(product) == true)             
    {
            return "Invalid Product Name";
        }
        // CHECK IF 50 PRODUCTS CREATED   
        else if (ValidateProductCount(id) == true)             
   {
            return "Max # of Products created Can't add Product";
        }

        // CHECK IF PRODUCT TYPE CREATED 
        else if (ValidateProductType(productType) == false)             
    {
            return "No Product Type Created";
        }

         // NOW ADD PRODUCT
         InsertProduct(productType, product,id);        


         return "Product Created Successfully";


    }

---

As mentioned in the previous post, use Enum types. Below is a sample code that could be used in your application.

public struct Result
{
    public Result(ActionType action,  Boolean success, ErrorType error) :
            this()
    {
        this.Action = action;
        this.HasSuceeded = success;
        this.Error = error;
    }

    public ActionType Action { get; private set; }
    public Boolean HasSuceeded { get; private set; }
    public ErrorType Error { get; private set; }
}

    public enum ErrorType
    {
        InvalidProductName, InvalidProductType, MaxProductLimitExceeded, None,
        InvalidCategoryName // and so on 
    }

    public enum ActionType
    {
        CreateProduct, UpdateProduct, DeleteProduct, AddCustomer // and so on
    }

    public class ProductBLL
    {
        public Result CreateProduct(String type, String name, Int32 id)
        {
            Boolean success = false;
            // try to create the product
            // and set the result appropriately
            // could create the product without errors?
            success = true;

            return new Result(ActionType.CreateProduct, success, ErrorType.None);
        }
    }

---

Don't use hardcoded strings.
Use an Enum for the return value, you can do much more and more efficiently with enums.

Validations must be done, only thing you can improve is to put the whole validation process in a single method.

After you call the method, you can have a single if sentence in the main method to check the enum returned.

if (IsValidated(productType, product,id) == MyEnumType.Success) { }

---

I'd use exceptions rather than a string or a enum...

---

I would recommend looking at the Validation framework used by Imar Spaanjaar in his N-Layer architecture series. The framework he uses if very versatile and it even supports Localization through using Resource files for the validation strings.

---

It is not a best practice to return a string with the status of the method.

The main reason is that it violates the separation of concerns between the UI layer and the business layer. You've taken the time to separate out the business logic into its own business layer; that's a good thing. But now the business layer is basically returning the error message directly to the UI. The error message to display to the user should be determined by the UI layer.

With the current implementation the business layer also becomes hard to use (for anyone without explicit knowledge of the implementation) because there is no contract. The current contract is that the method will return a string that you should display to the user. This approach makes reuse difficult. Two common scenarios that could cause headaches are if you want to support a new language (localization) or if you want to expose some of these business methods as a service.

I've been bitten when trying to use some old code like this before. The scenario is that I want to reuse the method because it does exactly what I want but that I want to take some action if a specific error occurs. In this case you end up either rewriting the business logic (which is sometimes not possible) or you end up having to hard code a horrible if statement into your application. e.g.

if (ProductBLL.CreateProduct(productType, product, ID) == 
    "Max # of Products created Can't add Product")
{
...
}

Then a requirement comes down that the message should be changed to something different ("The maximum number of products has been exceeded. Please add less products and try again."). This will break the above code. In production. On a Saturday night.

So in summary: don't do it.