2
\$\begingroup\$

This is a continuation of my question in this link: Adding data to database using asp.net and sql-server. Now I have already incorporated the solutions that was provided in the given link.

First thing I did was, I added two constructor in the code that was generated by the entity data model. One constructor has arguments that will pass the values that was input by the user to the database. And the other one is an empty constructor for other purposes.

public partial class instrumentItem
{
   public int instrumentId { get; set; }
   public Nullable<int> typeId { get; set; }
   public Nullable<int> brandId { get; set; }
   public string model { get; set; }
   public double price { get; set; }
   public string itemimage1 { get; set; }
   public string itemimage2 { get; set; }
   public string description { get; set; }
   public string necktype { get; set; }
   public string body { get; set; }
   public string fretboard { get; set; }
   public string fret { get; set; }
   public string bridge { get; set; }
   public string neckpickup { get; set; }
   public string bridgepickup { get; set; }
   public string hardwarecolor { get; set; }

   public virtual brand brand { get; set; }
   public virtual instrumentType instrumentType { get; set; }

   public instrumentItem(int TypeId,int BrandId,string Model,double 
         Price,string Itemimage1,string Itemimage2,string Description,
         string Necktype,string Body,string Fretboard,string Fret,string 
          Bridge,string Neckpickup,string Bridgepickup,string Hardwarecolor)
   {

        typeId = TypeId;
        brandId = BrandId;
        model = Model;
        price = Price;
        itemimage1 = Itemimage1;
        itemimage2 = Itemimage2;
        description = Description;
        necktype = Necktype;
        body = Body;
        fretboard = Fretboard;
        fret = Fret;
        bridge = Bridge;
        neckpickup = Neckpickup;
        bridgepickup = Bridgepickup;
        hardwarecolor = Hardwarecolor;
    }

    public instrumentItem(){ }
}

And then, I have changed the code in my aspx file where the user input all the data to be retrieved. I have now created an instance of instrumentItem and passed on all of the values in that object.

protected void Button1_Click(object sender, EventArgs e)
{

    try
    {
        int item_brandId = 
        ConnectionClassBrands.GetIdByBrand(itemBrand.Text);
        string item_model = itemModel.Text;
        double item_price = Convert.ToDouble(itemPrice.Text);
        string item_image1 = Session["PicturePath1"].ToString();
        string item_image2 = Session["PicturePath2"].ToString();
        string item_description = itemDescription.Text;
        string item_necktype = itemNeckType.Text;
        string item_body = itemBody.Text;
        string item_fretboard = itemFretboard.Text;
        string item_fret = itemFret.Text;
        string item_bridge = itemBridge.Text;
        string item_neckpickup = itemNeckPickup.Text;
        string item_bridgepickup = itemBridgePickup.Text;
        string item_hardwarecolor = itemHardwareColor.Text;

        if (itemType1.Checked)
        {
            int item_type = 
            ConnectionClassBrands.GetIdByType(itemType1.Text);
            ItemType = item_type;
        }
        else if (itemType2.Checked)
        {
            int item_type = 
            ConnectionClassBrands.GetIdByType(itemType2.Text);
            ItemType = item_type;
        }
            instrumentItem item = new instrumentItem(ItemType, 
              item_brandId,item_model, item_price, item_image1, item_image2,
            item_description, item_necktype, item_body, item_fretboard, 
            item_fret, item_bridge, item_neckpickup, item_bridgepickup, 
            item_hardwarecolor);

            ConnectionClassGuitarItems.AddStringInstrumentItems(item);

            ClearTextFields2();

    }
    catch (Exception ex)
    {
        lblResult2.Text = ex.Message;
    }


}

And then it will be passed on to the ConnectionClassGuitarItems class under the method named AddStringInstrumentItems. Here is the code:

public static void AddStringInstrumentItems(instrumentItem item)
{
    MusicStoreDBEntities obj = new MusicStoreDBEntities();

    obj.instrumentItems.Add(item);
    obj.SaveChanges();
}

It did greatly reduces the amount of code that I need to put in AddStringInstrumentItems.

But I have noticed that there is a problem in this approach, what if I need to change the columns in my database. Let's say I wanted to remove hardwarecolor. Then I would probably need to edit the constructor and the aspx code that I have shown earlier, which is a lot of work if you have tons of data.

I just want to know if we can improve this solution further and how to eliminate the problem that I have mentioned. Also kindly include examples.

\$\endgroup\$
1

1 Answer 1

4
\$\begingroup\$

Coding/naming conventions:

All the information about this can be found in folllowing article: Naming Guidelines.

Straight from the documentation:

To differentiate words in an identifier, capitalize the first letter of each word in the identifier. Do not use underscores to differentiate words, or for that matter, anywhere in identifiers.

DO use PascalCasing for all public member, type, and namespace names consisting of multiple words.

DO use camelCasing for parameter names.

Properties like instrumentId would become InstrumentId and so on. This also goes for the name of the class and the constructor: instrumentItem --> InstrumentItem. And variables like item_model should become itemModel.

Of course I'm not going to force you, but these are the conventions and there here for a reason, mostly readability.

Using an object initializer:

I see no validation whatsoever in your constructor or the setters of your properties. This is already bad practice in my opinion. But this allows you to use an object initializer instead of the constructor to create a new instance. And I would also asign the values immediately to the properties instead of creating variables and assigning those to the properties:

int itemType;

if (itemType1.Checked)
{
    itemType = ConnectionClassBrands.GetIdByType(itemType1.Text);
}
else if (itemType2.Checked)
{
    itemType = ConnectionClassBrands.GetIdByType(itemType2.Text);
}

var newInstrument = new InstrumentItem
{
    TypeId = itemType,
    BrandId = ConnectionClassBrands.GetIdByBrand(itemBrand.Text),
    Model = itemModel.Text,
    Price = Convert.ToDouble(itemPrice.Text),
    ItemImage1 = Session["PicturePath1"].ToString(),
    ItemImage2 = Session["PicturePath2"].ToString(),
    Description = itemDescription.Text,
    NeckType = itemNeckType.Text,
    Body = itemBody.Text,
    Fretboard = itemFretboard.Text,
    Fret = itemFret.Text,
    Bridge = itemBridge.Text,
    NeckPickup = itemNeckPickup.Text,
    BridgePickup = itemBridgePickup.Text,
    HardwareColor = itemHardwareColor.Text
};
                                       
ConnectionClassGuitarItems.AddStringInstrumentItems(item);
ClearTextFields2();

If you now add/remove a property from the class, you only have to add/remove that property in the initializer. You don't have to change the constructor and all the other lines of code anymore.

But I'd recommend to use validation. You can add code to validate the values in the constructor or setter of the properties, although in the setter is strongly recommended. Or you can use DataAnnotations.

Lastly, as mentioned in the comments: try looking into the Entity Framework.

\$\endgroup\$
3
  • \$\begingroup\$ thanks for the info..but what do you mean by validation? is it a technique? \$\endgroup\$
    – BrunoEarth
    Commented Aug 23, 2017 at 10:21
  • \$\begingroup\$ I mean that the values should be validated before they are assigned to a property. For example, that required fields have non-empty values, an integer that acts as an identifier is not below zero, or the input for a price is really a number. If I'd fill in Foo in the Price field, your code will throw an exception on the line with Convert.ToDouble(itemPrice.Text). \$\endgroup\$
    – Abbas
    Commented Aug 23, 2017 at 10:57
  • \$\begingroup\$ oh i understand now. Thank you again for the info. \$\endgroup\$
    – BrunoEarth
    Commented Aug 23, 2017 at 11:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.