3
\$\begingroup\$

This code is one of the models developed for the Book Inventory application that is open for review. It is also based on a comment on this answer to a C# question. Based on the comment I did some research and found this stackoverflow question.

The code presented for review is the database model for a series of books in the Book Inventory. A series of books has an author and a title. The author is represented by a key into the author table. To add a series to the database the user has to select the author from a list of authors already in the database. They then have to add the title or name of the series.

Below I present a before and after, the before is one function that was used in the answer above, the after is the entire code to be reviewed including the refactored function.

Questions:

Is there anything else I can do to prevent SQL Injection attacks?

What else can I do to improve the code, keep in mind I've been on this site for a while, I'm not asking "Does my code suck?", all code sucks, I'm asking how does my code suck.

Before

From my answer

    private DataRow GetRawSeriesData(uint seriesId)
    {
        DataRow rawData = null;

        if (seriesId > 0)
        {
            try
            {
                using (MySqlConnection conn = new MySqlConnection(_dbConnectionString))
                {
                    string queryString = "SELECT * FROM series WHERE idSeries = '" + seriesId.ToString() + "';";
                    int ResultCount = 0;
                    DataTable Dt = new DataTable();
                    conn.Open();
                    using (MySqlCommand cmd = new MySqlCommand())
                    {
                        cmd.Connection = conn;
                        cmd.CommandType = CommandType.Text;
                        cmd.CommandText = queryString;

                        MySqlDataAdapter sda = new MySqlDataAdapter(cmd);
                        ResultCount = sda.Fill(Dt);
                        if (ResultCount > 0)
                        {
                            rawData = Dt.Rows[0];
                        }
                    }
                }
            }
            catch (Exception ex)
            {
                string errorMsg = "Database Error: " + ex.Message;
                MessageBox.Show(errorMsg);
            }
        }

        return rawData;
    }

After: Code to be Reviewed: SeriesTableModel.cs

using System;
using System.Collections.Generic;
using System.Data;
using System.Windows;
using MySql.Data.MySqlClient;

namespace pacsw.BookInventory.Models
{
    public class SeriesTableModel : CDataTableModel
    {
        private int seriesTitleIndex;
        private int seriesKeyIndex;
        private int seriesAuthorKeyIndex;

        public SeriesTableModel() : base("series", "getAllSeriesData", "addAuthorSeries")
        {
            seriesTitleIndex = GetDBColumnData("SeriesName").IndexBasedOnOrdinal;
            seriesKeyIndex = GetDBColumnData("idSeries").IndexBasedOnOrdinal;
            seriesAuthorKeyIndex = GetDBColumnData("AuthorOfSeries").IndexBasedOnOrdinal;
        }

        public DataTable Series { get { return DataTable; } }

        public bool AddSeries(ISeriesModel iSeriesData)
        {
            SeriesModel seriesModel = (SeriesModel)iSeriesData;
            return addItem(seriesModel);
        }

        public bool AddSeries(SeriesModel seriesModel)
        {
            return addItem(seriesModel);
        }

        public List<string> SeriesSelectionListCreator(AuthorModel author)
        {
            List<string> seriesSelectionList = new List<string>();

            if (author != null && author.IsValid)
            {
                DataTable currentSeriesList = Series;
                string filterString = "LastName = '" + author.LastName + "' AND FirstName = '" + author.FirstName + "'";
                DataRow[] seriesTitleList = currentSeriesList.Select(filterString);

                foreach (DataRow row in seriesTitleList)
                {
                    seriesSelectionList.Add(row[seriesTitleIndex].ToString());
                }

            }

            return seriesSelectionList;
        }

        public uint GetSeriesKey(AuthorModel author, string seriesTitle)
        {
            uint key = 0;

            if (author != null && author.IsValid)
            {
                string SqlQuery = "SELECT series.idSeries FROM series WHERE series.SeriesName = @title AND series.AuthorOfSeries = @authorid;";

                using (MySqlConnection conn = new MySqlConnection(_dbConnectionString))
                {
                    int ResultCount = 0;
                    DataTable Dt = new DataTable();
                    try
                    {
                        conn.Open();
                        using (MySqlCommand cmd = new MySqlCommand())
                        {
                            cmd.Connection = conn;
                            cmd.CommandType = CommandType.Text;
                            cmd.CommandText = SqlQuery;
                            cmd.Parameters.Add("@title", MySqlDbType.String);
                            cmd.Parameters["@title"].Value = seriesTitle;
                            cmd.Parameters.Add("@authorid", MySqlDbType.UInt32);
                            cmd.Parameters["@authorid"].Value = author.AuthorId;

                            cmd.ExecuteNonQuery();
                            MySqlDataAdapter sda = new MySqlDataAdapter(cmd);
                            ResultCount = sda.Fill(Dt);
                            if (ResultCount > 0)
                            {
                                key = Dt.Rows[0].Field<uint>(0);
                            }
                        }
                    }
                    catch (Exception ex)
                    {
                        string errorMsg = "Database Error: " + ex.Message;
                        MessageBox.Show(errorMsg);
                        key = 0;
                    }
                }
            }

            return key;
        }

        public string GetSeriesTitle(uint seriesId)
        {
            string title = string.Empty;

            if (seriesId > 0)
            {
                string SqlQuery = "SELECT series.SeriesName FROM series WHERE series.idSeries = @seriesid;";

                using (MySqlConnection conn = new MySqlConnection(_dbConnectionString))
                {
                    int ResultCount = 0;
                    DataTable Dt = new DataTable();
                    try
                    {
                        conn.Open();
                        using (MySqlCommand cmd = new MySqlCommand())
                        {
                            cmd.Connection = conn;
                            cmd.CommandType = CommandType.Text;
                            cmd.CommandText = SqlQuery;
                            cmd.Parameters.Add("@seriesid", MySqlDbType.UInt32);
                            cmd.Parameters["@seriesid"].Value = seriesId;

                            cmd.ExecuteNonQuery();
                            MySqlDataAdapter sda = new MySqlDataAdapter(cmd);
                            ResultCount = sda.Fill(Dt);
                            if (ResultCount > 0)
                            {
                                title = Dt.Rows[0].Field<string>(0);
                            }
                        }
                    }
                    catch (Exception ex)
                    {
                        string errorMsg = "Database Error: " + ex.Message;
                        MessageBox.Show(errorMsg);
                    }
                }
            }

                return title;
        }

        public SeriesModel GetSeriesModel(uint seriesId)
        {
            SeriesModel seriesData = null;
            DataRow rawSeriesData = GetRawSeriesData(seriesId);

            if (rawSeriesData != null)
            {
                seriesData = ConvertDataRowToSeriesModel(rawSeriesData);
            }

            return seriesData;
        }

        protected override void InitializeSqlCommandParameters()
        {
            AuthorTableModel authorTable = ((App)Application.Current).Model.AuthorTable;
            MySqlParameterCollection parameters = AddItemParameters;

            _addSqlCommandParameter("First Name", authorTable.GetDBColumnData("FirstName"), parameters["@authorFirst"]);
            _addSqlCommandParameter("Last Name", authorTable.GetDBColumnData("LastName"), parameters["@authorLast"]);
            _addSqlCommandParameter("Series Title", GetDBColumnData("SeriesName"), parameters["@seriesTitle"]);
        }

        private SeriesModel ConvertDataRowToSeriesModel(DataRow rawSeriesData)
        {
            uint authorId;
            uint.TryParse(rawSeriesData[seriesAuthorKeyIndex].ToString(), out authorId);
            string title = rawSeriesData[seriesTitleIndex].ToString();

            AuthorModel author = ((App)Application.Current).Model.AuthorTable.GetAuthorFromId(authorId);

            SeriesModel seriesModel = new SeriesModel(author, title);

            return seriesModel;
        }

        private DataRow GetRawSeriesData(uint seriesId)
        {
            DataRow rawData = null;

            if (seriesId > 0)
            {
                try
                {
                    using (MySqlConnection conn = new MySqlConnection(_dbConnectionString))
                    {
                        string queryString = "SELECT * FROM series WHERE idSeries = @seriesid;";
                        int ResultCount = 0;
                        DataTable Dt = new DataTable();
                        conn.Open();
                        using (MySqlCommand cmd = new MySqlCommand())
                        {
                            cmd.Connection = conn;
                            cmd.CommandType = CommandType.Text;
                            cmd.CommandText = queryString;
                            cmd.Parameters.Add("@seriesid", MySqlDbType.UInt32);
                            cmd.Parameters["@seriesid"].Value = seriesId;

                            MySqlDataAdapter sda = new MySqlDataAdapter(cmd);
                            ResultCount = sda.Fill(Dt);
                            if (ResultCount > 0)
                            {
                                rawData = Dt.Rows[0];
                            }
                        }
                    }
                }
                catch (Exception ex)
                {
                    string errorMsg = "Database Error: " + ex.Message;
                    MessageBox.Show(errorMsg);
                }
            }

            return rawData;
        }

    }
}
\$\endgroup\$
4
\$\begingroup\$

Anytime I see something like these I wonder "is there a better way we can do this?":

cmd.Parameters.Add("@title", MySqlDbType.String);
cmd.Parameters["@title"].Value = seriesTitle;
cmd.Parameters.Add("@authorid", MySqlDbType.UInt32);
cmd.Parameters["@authorid"].Value = author.AuthorId;

Well, in this case, I'm going to suggest a slightly-functional approach to this process.

C# has a (mostly) handy feature called "Extension Methods". In general, I try to avoid them unless there's a good application for them, such as here.

I would define an extension method here quick:

public static MySqlCommand AddParameter<T>(this MySqlCommand command, string name, MySqlDbType type, T value) {
    command.Parameters.Add(name, type);
    command.Parameters[name].Value = value;
    return command;
}

Now, here, I returned the MySqlCommand on purpose, because I want to chain this:

command.AddParameter("@title", MySqlDbType.String, seriesTitle)
       .AddParameter("@authorid", MySqlDbType.UInt32, author.AuthorId);

Since you aren't using an ORM (and I suspect you have reasons for that, I definitely won't try to persuade you to use one) I would do a couple of these small things to make life a little easier on myself. It's pretty cheap, and it allows us to work our code more easily.


I would take some of these shorter functions and apply some expression-bodied members:

public DataTable Series { get { return DataTable; } }

public bool AddSeries(ISeriesModel iSeriesData)
{
    SeriesModel seriesModel = (SeriesModel)iSeriesData;
    return addItem(seriesModel);
}

public bool AddSeries(SeriesModel seriesModel)
{
    return addItem(seriesModel);
}

To:

public DataTable Series => DataTable;
public bool AddSeries(ISeriesModel iSeriesData) => addItem((SeriesModel)iSeriesData);
public bool AddSeries(SeriesModel seriesModel) => addItem(seriesModel);

For simple functions like those, it's trivial to do and saves you some vertical space.

\$\endgroup\$
  • \$\begingroup\$ If I implement the first suggestion in the abstract class CDataTableModel in my original question will it work in all the inherited DataTable models classes? \$\endgroup\$ – pacmaninbw Apr 9 at 18:34
  • 1
    \$\begingroup\$ No, you will need to make a new public static class for the extension methods, but then as long as the namespace containing that public static class is available, it will work in any class. \$\endgroup\$ – 410_Gone Apr 9 at 18:37
  • \$\begingroup\$ Usually it's a better idea to cast a class into an interface rather the the other way around as in your last two lines but I guess this is just a blind copy/paste of OP's code and turning the methods into expression-bodied ones but in this case the overload taking a SeriesModel is no longer necessary. \$\endgroup\$ – t3chb0t Apr 10 at 10:35
  • \$\begingroup\$ @t3chb0t Yeah I didn't look at any of the dependency trees, just went off what was in the question. \$\endgroup\$ – 410_Gone Apr 10 at 11:37
3
\$\begingroup\$
public bool AddSeries(ISeriesModel iSeriesData)
{
    SeriesModel seriesModel = (SeriesModel)iSeriesData;
    return addItem(seriesModel);
}

public bool AddSeries(SeriesModel seriesModel)
{
    return addItem(seriesModel);
}

If SeriesModel implements ISeriesModel interface (which I suppose it does) then you need only the overload taking the interface.

Also casting an interface into a class is rarely a good idea because we then loose the advantages of having the interface in the first place like using a mock type.

\$\endgroup\$
  • \$\begingroup\$ Is there a good source for when to use and interface and when not to use an interface? I was using it to hide data. \$\endgroup\$ – pacmaninbw Apr 10 at 12:57
  • 1
    \$\begingroup\$ @pacmaninbw I would look on software engineering but in general you use it when you need a level of abstraction to decouple the interface you're working with from the type implementing it. This allows us to easily exchange the implementation without breaking anything. Another usage can be when several types should share some common api like CalculateArea if it was a shape. In python it would work with duck typing. \$\endgroup\$ – t3chb0t Apr 10 at 13:03

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.