Skip to main content
update based on the comment
Source Link
walther
  • 358
  • 2
  • 9

Firstly, data access should be separate from your UI logic. Certainly create a new layer, where you'll manage CRUD operations.

Secondly, I'm not really fond of your naming convention of classes. Even though it may be a subjective matter, many people tend to use capitalized names for classes and certainly not using words like "getBlocks". This isn't a name for a class, that's a name for a getter method.

Thirdly, you're dealing with your parameters the wrong way, at least in my opinion. I believe it would be more appropriate to have private fields of type DropDownList and String, populate them in the constructor and then just use these private variables (and/or properties, depends on your needs) instead of specifying parameters for the method itself. Seems more OOP to me that way. Otherwise I don't really see a reason why not just create a helper class with a static method you'll call whenever needed, without the need to instantiate the class itself.

Another thing to consider - usually it's better to use an using statement instead of manually calling Dispose method.

Edit based on the comment:

1.) example using private fields

public sealed class Blocks
{
    private DropDownList _ddList;
    private int _districtId;

    public Blocks(DropDownList dropDownList, int districtId)
    {
        _ddList = dropDownList;
        _districtId = districtId;
    }

    public void PopulateDropDownList()
    { 
        var results = MyDbAccessClass.GetBlocks(_districtId); 
        
        _ddList.DataSource = results;
        _ddList.DataTextField = "blockname";
        _ddList.DataValueField = "blockid";
        _ddList.DataBind();
        _ddList.Items.Insert(0, "-- Select --");
    }
}

2.) example using static method

public sealed class MyHelperMethods
{
    public static void PopulateWithBlocks(DropDownList ddList, int districtId)
    {
         var results = MyDbAccessClass.GetBlocks(districtId); 
        
        ddList.DataSource = results;
        ddList.DataTextField = "blockname";
        ddList.DataValueField = "blockid";
        ddList.DataBind();
        ddList.Items.Insert(0, "-- Select --");
    }
}

Hope you get the idea...

Firstly, data access should be separate from your UI logic. Certainly create a new layer, where you'll manage CRUD operations.

Secondly, I'm not really fond of your naming convention of classes. Even though it may be a subjective matter, many people tend to use capitalized names for classes and certainly not using words like "getBlocks". This isn't a name for a class, that's a name for a getter method.

Thirdly, you're dealing with your parameters the wrong way, at least in my opinion. I believe it would be more appropriate to have private fields of type DropDownList and String, populate them in the constructor and then just use these private variables (and/or properties, depends on your needs) instead of specifying parameters for the method itself. Seems more OOP to me that way. Otherwise I don't really see a reason why not just create a helper class with a static method you'll call whenever needed, without the need to instantiate the class itself.

Another thing to consider - usually it's better to use an using statement instead of manually calling Dispose method.

Firstly, data access should be separate from your UI logic. Certainly create a new layer, where you'll manage CRUD operations.

Secondly, I'm not really fond of your naming convention of classes. Even though it may be a subjective matter, many people tend to use capitalized names for classes and certainly not using words like "getBlocks". This isn't a name for a class, that's a name for a getter method.

Thirdly, you're dealing with your parameters the wrong way, at least in my opinion. I believe it would be more appropriate to have private fields of type DropDownList and String, populate them in the constructor and then just use these private variables (and/or properties, depends on your needs) instead of specifying parameters for the method itself. Seems more OOP to me that way. Otherwise I don't really see a reason why not just create a helper class with a static method you'll call whenever needed, without the need to instantiate the class itself.

Another thing to consider - usually it's better to use an using statement instead of manually calling Dispose method.

Edit based on the comment:

1.) example using private fields

public sealed class Blocks
{
    private DropDownList _ddList;
    private int _districtId;

    public Blocks(DropDownList dropDownList, int districtId)
    {
        _ddList = dropDownList;
        _districtId = districtId;
    }

    public void PopulateDropDownList()
    { 
        var results = MyDbAccessClass.GetBlocks(_districtId); 
        
        _ddList.DataSource = results;
        _ddList.DataTextField = "blockname";
        _ddList.DataValueField = "blockid";
        _ddList.DataBind();
        _ddList.Items.Insert(0, "-- Select --");
    }
}

2.) example using static method

public sealed class MyHelperMethods
{
    public static void PopulateWithBlocks(DropDownList ddList, int districtId)
    {
         var results = MyDbAccessClass.GetBlocks(districtId); 
        
        ddList.DataSource = results;
        ddList.DataTextField = "blockname";
        ddList.DataValueField = "blockid";
        ddList.DataBind();
        ddList.Items.Insert(0, "-- Select --");
    }
}

Hope you get the idea...

Source Link
walther
  • 358
  • 2
  • 9

Firstly, data access should be separate from your UI logic. Certainly create a new layer, where you'll manage CRUD operations.

Secondly, I'm not really fond of your naming convention of classes. Even though it may be a subjective matter, many people tend to use capitalized names for classes and certainly not using words like "getBlocks". This isn't a name for a class, that's a name for a getter method.

Thirdly, you're dealing with your parameters the wrong way, at least in my opinion. I believe it would be more appropriate to have private fields of type DropDownList and String, populate them in the constructor and then just use these private variables (and/or properties, depends on your needs) instead of specifying parameters for the method itself. Seems more OOP to me that way. Otherwise I don't really see a reason why not just create a helper class with a static method you'll call whenever needed, without the need to instantiate the class itself.

Another thing to consider - usually it's better to use an using statement instead of manually calling Dispose method.