10

I have a table full of input text they look like this:

<input type='text' value='{Value}' id='{Model.ID}' class='ChckNumber' />

the class name is different based off what column it is, the Model.ID is different based off the row and the column is different based off the column and row.

When the input text becomes unfocused I call an api to update the value with what the user entered like so:

$("input[type=text]").on("focusout", function () {

        var id = $(this).attr("id");
        var column = $(this).attr("class");
        var value = $(this).val();

        if (typeof id !== "undefined" && typeof column !== "undefined" && typeof value !== "undefined") {

            $.ajax({
                url: "/api/Action/UpdateTable?id=" + id + "&column=" + column + "&value=" + value,
                type: "GET",
                error: function (request, status, error) {
                    InsertLogEntry(request.responseText + " From API Call /api/Action/UpdateTable");
                },
                success: function (data) {
                }
            });
        }

    });

Here is the API Controller its calling:

[HttpGet]
        public void UpdateTable(int id, string column, string value)
        {
            MethodClass method = new MethodClass();

            if(column != null && column != "" && id != 0)
            {
                method.UpdateTable(id, column, value);
            }
        }

and here is the method call I am making:

public void UpdateTable(int id, string column, string value)
        {

            connection = new SqlConnection(connectionString);

            if(column.Contains("Date"))
            {
                DateTime dt = Convert.ToDateTime(value);

                command = new SqlCommand("UPDATE tblCheckTrackerRegister SET " + column + " = @Date WHERE ID = " + id);

                command.Parameters.Add("@Date", System.Data.SqlDbType.DateTime);

                command.Parameters["@Date"].Value = dt.Equals(DateTime.MinValue) ? (object)DBNull.Value : dt;

            }
            else
            {

                int result;

                if (int.TryParse(value, out result))
                {
                    command = new SqlCommand("UPDATE table SET " + column + " = " + value + " WHERE ID = " + id);
                }
                else
                {
                    command = new SqlCommand("UPDATE table SET " + column + " = '" + value + "' WHERE ID = " + id);
                }

            }

            command.Connection = connection;

            connection.Open();

            command.ExecuteNonQuery();

            connection.Close();

        }

This works well, but I am looking to improve it. I honestly believe there must be a better way to go about this as my code seems messy to me, My question is, can anyone suggest another way to go about what I am trying to accomplish?

user979331
  • 11,039
  • 73
  • 223
  • 418
  • looking above code - it seems there will be issue when your value is containing decimal value like (value = 10.53) at that time it goes to the else part and will consider it as string – prog1011 Mar 14 '17 at 09:15
  • By improve what you mean ?? Javascript code improvement or asp.net code improvement ? – Mairaj Ahmad Mar 15 '17 at 12:34
  • hmmmmm...! Is there a reason why you cant write/use a standard update procedure that updates all the values regardless of wether they changed or not that is the way 99.99% percent of us, do it. And there is no performance overhead (negligible) and you don't have to maintain such bug prone code. – DaniDev Mar 16 '17 at 06:01

4 Answers4

1

First

Your code is prone for SQL Injection attack. It is a red flag for security.

command = new SqlCommand("UPDATE tblCheckTrackerRegister SET " + column + " = @Date WHERE ID = " + id);
command = new SqlCommand("UPDATE table SET " + column + " = " + value + " WHERE ID = " + id);
command = new SqlCommand("UPDATE table SET " + column + " = '" + value + "' WHERE ID = " + id);

Suggestion: Use parameterize query.

Second

You are updating a record, so you should not use GET method. It violates basic REST principle - GET action is said to be safe, and it should not change anything in the system.

Suggestion: Use Ajax POST.

$.ajax({
  ...                
  type: "POST",
  ...            
});

UX Design

I do not know the reason behind why each textbox is updated as soon as out of focus. It is not a common approach. From the user perspective, it is not friendly either because they are not notified about changes being saved.

We normally update a group of control at a time or one row at a time in Grid.

Suggestion : Not really a suggestion; it is just a personal preference, so don't get me wrong. I personally like to update a chunk of control at a time like attached StackOver screen shot -

enter image description here

Community
  • 1
  • 1
Win
  • 61,100
  • 13
  • 102
  • 181
0

Try define a set of conditions in the JS code to use multiple Controllers as your column definition needs, something like this:

$("input[type=text]").on("focusout", function () {

        var id = $(this).attr("id");
        var column = $(this).attr("class");
        var value = $(this).val();
        var controler = "UpdateTable";
        if(column=='Delete'){
           controler = "DeleteTable";
        }
        if(column=='Mail'){
           controler = "SendMail";
        }
        if (typeof id !== "undefined" && typeof column !== "undefined" && typeof value !== "undefined") {

            $.ajax({
                url: "/api/Action/" + controler + "?id=" + id + "&column=" + column + "&value=" + value,
                type: "GET",
                error: function (request, status, error) {
                    InsertLogEntry(request.responseText + " From API Call /api/Action/" + controler);
                },
                success: function (data) {
                }
            });
        }

    });
JC Hernández
  • 777
  • 3
  • 13
0

using entity framework you can easily update the table using much cleaner and simple code.

using (var db = new MyEntities())
{
    var result = db.YourTable.SingleOrDefault(b => b.column == finditembyid);
    if (result != null)
    {
        try
        {
            result.Date = new DateTime();
            db.SaveChanges();
        }
        catch (Exception ex)
        {
            throw;
        }
    }
}
Kapila Perera
  • 837
  • 1
  • 11
  • 24
0

Main thing I can think of is, why not use 'change' instead of 'focusout'? Focusout will run your code even if the value has not changed, do you need that?

Another thing:

var id = $(this).attr("id");
var column = $(this).attr("class");
var value = $(this).val();

Can be improved to:

var $this = $(this);
var id = $this.attr("id");
var column = $this.attr("class");
var value = $this.val();
Martina
  • 739
  • 3
  • 13