3
\$\begingroup\$

I wanted to know other ways where you can access data from the database into the asp.net website that I've created for displaying details and images. As well as looking for better ways to use those retrieved data for displaying in a web page. Also, kindly correct me if my explanation to the flow of my code is incorrect, I'm still a beginner.

Let's say I have a table name called guitarItems and inside it are the following data:

id  type    brand   model   price
1   Guitar  Ibanez  ARz307  9000
2   Guitar  Ibanez  DT420   10000
3   Guitar  Ibanez  JBM100  18000

The first thing I did was to create a class where the constructor named GuitarItems would get and set the data retrieved from the SQL connection, which I will show you later.

public int Id { get; set; }
public string Type { get; set; }
public string Brand { 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 GuitarItems(int id, string type, string brand,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)
  {
    Id = id;
    Type = type;
    Brand = brand;
    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;
  }

Next is the code that will connect to the database to get the data. As you can see below, the data retrieved from the database is being supplemented to the constructor that I created previously so that we can use it to other classes in the project.

public static ArrayList GetGuitarItems(string itemCategory)
{
    ArrayList list = new ArrayList();
    string query = string.Format("SELECT * FROM guitarItems WHERE brand LIKE @brand");

    try
    {
        conn1.Open();
        command1.CommandText = query;
        command1.Parameters.Add(new SqlParameter("brand", itemCategory));
        SqlDataReader reader = command1.ExecuteReader();

        while (reader.Read())
        {
            int id = reader.GetInt32(0);
            string type = reader.GetString(1);
            string brand = reader.GetString(2);
            string model = reader.GetString(3);
            double price = reader.GetDouble(4);
            string itemimage1 = reader.GetString(5);
            string itemimage2 = reader.GetString(6);
            string description = reader.GetString(7);
            string necktype = reader.GetString(8);
            string body = reader.GetString(9);
            string fretboard = reader.GetString(10);
            string fret = reader.GetString(11);
            string bridge = reader.GetString(12);
            string neckpickup = reader.GetString(13);
            string bridgepickup = reader.GetString(14);
            string hardwarecolor = reader.GetString(15);

            GuitarItems gItems = new GuitarItems(id, type, brand, model, price, itemimage1, itemimage2, description, necktype, body,
                fretboard, fret, bridge, neckpickup, bridgepickup, hardwarecolor);
            list.Add(gItems);
        }
    }
    finally
    {
        conn1.Close();
        command1.Parameters.Clear();
    }

    return list;
}

And then lastly, here is the code for displaying the data retrieved from the database. As for this page, it is only designed to retrieve one ibanez product. Other products would have their own web page respectively. Now, I used the GetGuitarItems method from the previous example to specify the product that I want to retrieve for this page into an ArrayList variable called itemDetails. Then I'm using a foreach loop with a reference variable for GuitarItems called gList to get the data from the ArrayList itemDetails. The reference variable gListwould then be used to implementing it in the HTML tags for displaying.

public partial class Pages_GuitarItemsIbanezDetails1 : System.Web.UI.Page
{
private string guitarBrandType = "Ibanez";
private int x = 0;
protected void Page_Load(object sender, EventArgs e)
{
    FillPage();
}

public void FillPage()
{
    ArrayList itemDetails = new ArrayList();

    itemDetails = ConnectionClassGuitarItems.GetGuitarItems(guitarBrandType);

    StringBuilder sb = new StringBuilder();

    foreach (GuitarItems gList in itemDetails)
    {
        if (x != 0)
        {
            x++;
            continue;
        }
        sb.Append(
           string.Format(
               @"<div class='guitarItemsDetailsWrapper'>
                        <div class='guitarItemsDetailsImage'>
                            <img runat='server' src='{3}' />
                        </div>

                        <div class='guitarItemsDetailsStyle'>
                            <h2>Name: </h2><p>{0} {1}</p>
                            <br/>
                            <h2>Price: </h2><p>${2}</p>
                            <br/>
                            <h2>Description: </h2><p>{4}</p>
                            <br/>
                            <h2>Neck Type: </h2><p>{5}</p>
                            <br/>
                            <h2>Body: </h2><p>{6}</p>
                            <br/>
                            <h2>Fretboard: </h2><p>{7}</p>
                            <br/>
                            <h2>Fret: </h2><p>{8}</p>
                            <br/>
                            <h2>Bridge: </h2><p>{9}</p>
                            <br/>
                            <h2>Neck Pickup: </h2><p>{10}</p>
                            <br/>
                            <h2>Bridge Pickup: </h2><p>{11}</p>
                            <br/>
                            <h2>Hardware Color: </h2><p>{12}</p>
                            <br/>
                        </div>
                    </div>", gList.Brand, gList.Model, gList.Price, gList.ItemImage2, gList.Description, gList.NeckType,
                    gList.Body, gList.Fretboard, gList.Fret, gList.Bridge, gList.NeckPickup, gList.BridgePickup, gList.HardwareColor));
        if (x == 0)
        {
            break;
        }
        x++;

    }




    lblOutput.Text = sb.ToString();

}

Also, you might have noticed I'm using break and continue inside my foreach. Because if I don't, it will display everything that it retrieved from the database. The outcome would be three guitar products displayed in one page. Like I said previously, one product should only be displayed in one page. So right now, based on the code above, I'm retrieving the data for product id no. 1. If I want to display product id no. 2, I'd change the if statement to this:

if (x != 1)
{
    x++;
    continue;
}

if (x == 1)
{
   break;
}

As of now, my website is working okay by using the codes presented above. But I'm really not sure if this is the right way to do it. Kindly suggest the appropriate techniques to achieve the same goal as I have explained above.

\$\endgroup\$
  • 1
    \$\begingroup\$ Just a quick note: ArrayList has been obsolete since C# 2.0 and the advent of generics. Consider returning an IEnumerable<GuitarItems> or IReadOnlyList<GuitarItems>, so that you can't add/remove items from the calling code ...and GuitarItems (the type, not the table) should be singular; plural denotes a bunch of guitar items, soGuitarItems reads like some IEnumerable<GuitarItem>. \$\endgroup\$ – Mathieu Guindon May 26 '17 at 13:57
  • 1
    \$\begingroup\$ You will get many nice suggestions about how to improve this code (and you need to try them all to understand why they're important) but after you learned...please use an ORM (let's play with Entity Framework, for example). Also do not build HTML strings "by hand", you may produce broken HTML code. Try, for example, to set vendor to "Carvin A&I" and check your output... \$\endgroup\$ – Adriano Repetti May 26 '17 at 13:58
  • \$\begingroup\$ What version of C# are you using? Reviewers will likely assume you can use C# 6.0. \$\endgroup\$ – Mathieu Guindon May 26 '17 at 14:05
  • \$\begingroup\$ @Mat'sMug - yes c# 6.0 is okay. \$\endgroup\$ – RockStar May 27 '17 at 6:03
  • 1
    \$\begingroup\$ In FillPage() you format HTML string using text from DB. If text contains "invalid" HTML characters then you produce broken HTML. It has to be escaped. \$\endgroup\$ – Adriano Repetti May 27 '17 at 7:20
3
\$\begingroup\$

Introduction: no one reviewed your code then let me do it, even if after few days. The problem is that you're in the middle of the learning process then many things need to be changed/updated. I won't rewrite your code (it won't help you to learn) but I will try to go through it leaving the fixes as exercise for the reader.


Let's start from your domain class GuitarItems. First important thing is name: It has not to be plural, it represents a single guitar. Why it's not simply Guitar? I'd also like to see your DB schema for this but let it be...

I don't see class definition but it's used only internally then it should be internal (default) and sealed because you won't derive any other class. Something like this:

sealed class Guitar {
}

All your properties have public getter and setter howerver you set values only from within constructor. Restrict access to what absolutely necessary! We're gonna go to change your ctor and I wouldn't introduce more logical tiers in your application because they have no use for you (now) but I'll mention them later. First of all let's make all your properties as read-only (because you change them only in ctor):

sealed class Guitar {
    public int Id { get; }
    public string Type { get; }
}

Your class is reach of properties and you have one parameter for them in your ctor. It's absolutely a nightmare to maintain. Think if you need to add another column in your database, you will have to:

  • Update DB schema.
  • Update code where you read values from your query (and you're using ordinals, pretty error-prone).
  • Add another parameter to your constructor.
  • Change code where call ctor and be extra careful to add the new parameter in the right position.

Isn't it too easy to introduce bugs? You have at least two options here:

  • Rollback our previous change and make your property setters public again but fill them when reading data.
  • Pass the IDataReader to the ctor and do the reading there, your ctor will always have one parameter regardless how many properties you have.

First case:

while (reader.Read())
{
    var guitar = new Guitar
    {
        Id = reader.GetInt32(0),
        Type = reader.GetString(1),
    }
}

Second case:

while (reader.Read())
    list.Add(new Guitar(reader));

Together with:

public Guitar(SqlDataReader reader)
{
    Id = reader.GetInt32(0);
    Type = reader.GetString(1);
}

Exercise 1: try to rewrite your code to use both solutions. Which one you feel more comfortable with? Which are the drawbacks? How many responsibilities has the Guitar class in the second case?

Exercise 2: answering those questions you might feel you need to introduce another class to map DB data to objects. In future you will learn to use ORMs (for example Entity Framework) but for now try to write your mapper:

sealed class DbDataMapper
{
    public Guitar ToGuitar()
    {
        return new Guitar
        {
            Id = _reader.GetInt32(0),
            Type = _reader.GetString(1),
        };
    }
}

That's just a proof of concept, experiment with that. Get the SqlDataReader in DbDataMapper ctor, get is as parameter of ToGuitar() method...

Now you probably can see another problem. You're using column index to map data from DB to properties. That's fragile and error-prone. What can you do?! For now you can start using names:

Id = _reader.GetInt32(_reader.GetOrdinal("Id"));

Still it's not optimal because you have hard-coded strings but it's a step to improve readability. If names match (properties and DB columns) then you might also do this:

Id = _reader.GetInt32(_reader.GetOrdinal(nameof(Guitar.Id)));

A big step in the right direction will be to decorate your class with proper attributes to keep mapping details close to related properties. Let's imagine we have a ColumnAttribute for this purpose and we can cycle through the properties using Reflection:

var guitar = new Guitar();
foreach (var property in typeof(Guitar).GetProperties())
{
    var columnName = property.GetAttribute<ColumnAttribute>().Name;
    var columnIndex = reader.GetOrdinal(columnName);

    property.SetValue(guitar, reader.GetValue(columnIndex));
}

Exercise 3: write a ColumnAttribute class, decorate Guitar properties with that attribute and rewrite your code to support it. Experiment with conversions. What will happen if a property has not that attribute? Handle that case. Rewrite this method to be generic, unrelated to the Guitar class.


We're now to take a look to GetGuitarItems() method. conn1 isn't a meaningful name and it seems to be an instance field. It might simply be a local variable. Also you're using try/finally to dispose connection (right!) but it's the purpose of using statement. Note that SqlCommand and SqlDataReader has to be disposed too. Note that query can be a private const string and you do not need String.Format:

const string SqlQueryGuitarsByBrand = "SELECT * FROM guitarItems WHERE brand LIKE @brand";

using (var connection = new SqlConnection("..."))
{
    connection.Open();

    using (var command = connection.CreateCommand())
    {
        command.CommandText = SqlQueryGuitarsByBrand;

        // ...

        using (var reader = command.ExecuteReader())
        {
            // ...
        }
    }
}

Exercise 4: complete the above code...


And now we're finally in your FillPage() method. First of all let's see what your code is doing:

  • If x is 0 then you break the loop.
  • If x is non zero then...you consume the list doing nothing with them.

Exercise 5: if you really need to display only the first item then fix your logic to break after the first item and do not use an instance field for function local logic. Drop ArrayList, it's a .NET 1.1 legacy type, in favor of List<Guitar>.

Now let's see a ready-to-use method to do the same job: First(). Let's also make your list paginated:

static IEnumerable<Guitar> GetGuitars(string brand, int pageIndex, int itemsPerPage)
{
    return GetGuitars(brand)
        .Skip(pageIndex * itemsPerPage)
        .Take(itemsPerPage);
}

Exercise 6: handle corner cases (like not enough items in the returned list) and rewrite your code to use it. Think about how pagination works and do some tests to see what's returned. Add an ordering clause to have repeatable results.

How to build HTML? I already told you in comments that you can't simply build HTML code with text from a data source because it need to be encoded.

Exercise 7: rewrite your code to call HttpUtility.HtmlEncode().

As you can see to write HTML inside C# isn't optimal. The best would be to have a separate HTML file with the template you want to use. Not to mention that you're actually writing HTML code into a Label then it won't be formatted. You're using ASP.NET then you already have a pretty powerful mechanism in-place. For now try to write something like this in your aspx page (fill guitar with the list of guitars after search), it's exactly the purpose of ASPX pages!

<div>
  <% foreach (var guitar in guitars) { %>
    <h2>Name</h2><p><%=guitar.Name %></p>
  <% } %>
</div>

Exercise 8: rewrite your page code to avoid HTML in code-behind.


There are still many things you may want to improve (not to mention that we still not have any error handling) and technologies you may want to use but I think this is enough to start with...

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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