4
\$\begingroup\$

This is a single function in C#. It works fine. My concern is that although I've made the code as short and as multi-functional as I can, what it's doing seems very unclear. It looks like the sort of thing I'm going to come back to in six months and hate myself for writing.

Its purpose is to build and execute generic "upsert" sql statements for a variety of scenarios. Each record that it comes across has a name, an address and a flag to say whether the operation on that record is a delete and whether that record represents someone who's gone away. The former tells us whether we're setting a bit value to true or false, while the latter determines the column name.

How can I make this code clearer without turning it into a mess of flow control?

private void Sprail(FileRow fr, string columnName)
{
    List<Suppression> addressMatches = GetMatchingRecordsFromDb(SanitiseForMySql(fr.CleanedAddress), SanitiseForMySql(fr.CleanedName));

    using (MySqlConnection con = new MySqlConnection(_connectionString))
    {
        string sqlTargetFlag = columnName;
        string sqlTargetValue = "1";
        string sql = "";

        if (fr.IsDeleted)
        {
            sqlTargetValue = "0";
        }

        if (fr.IsGoneaway)
        {
            sqlTargetFlag = sqlTargetFlag + "Goneaway";
        }

        if (string.IsNullOrWhiteSpace(fr.CleanedAddress))
        {
            sql = BuildInsertSql(sqlTargetFlag, fr, sqlTargetValue);
        }

        foreach (Suppression s in addressMatches)
        {
            if (fr.CleanedName.ToString().ToLower() == s.NameKey.ToLower())
            {
                sql = string.Format("UPDATE MyTable set {0} = {1} where SuppressionId = {2}", sqlTargetFlag, sqlTargetValue, s.SuppressionId);
                break;
            }
        }

        if(sql != "")
        {
            con.Open();
            MySqlCommand cmd = new MySqlCommand(sql, con);
            cmd.ExecuteNonQuery();
        }
    }
}

private string BuildInsertSql(string sqlTargetFlag, FileRow fr, string sqlTargetValue)
{
    return string.Format(@"INSERT INTO MyTable (`AddressKey`, `NameKey`, `{0}`) VALUES ('{1}', '{2}', '{3}')", 
                                sqlTargetFlag, SanitiseForMySql(fr.CleanedAddress), SanitiseForMySql(fr.CleanedName), sqlTargetValue);
}
\$\endgroup\$

3 Answers 3

4
\$\begingroup\$

Code like this is bound to be a bit hairy.

However

You aren't using parameters

This is a big security vulnerability. What's that SanitiseForMySql method doing? Is it doing everything by the book? Does it handle every possible obscure corner case? Let ADO the MySQL connector manipulate your query, especially if there's a chance you're handling values supplied by the user.

MySQL can perform the so-called MERGE (also called UPSERT)

You can do it via INSERT ... ON DUPLICATE KEY UPDATE statement. In case you are not aware what that is, it's basically a way to either insert or update in case the INSERT fails because of a duplicate key error. It's for sure faster than making a separate query to retrieve the records that are currently present.

By the way, code like this leaves your code open to race conditions in case another user inserts a record exactly between when you read the current records and the moment you execute your INSERT. This is bad, and you're not currently guarding against it. MySQL can do it for you, take advantage of it.

EDIT

Also

This: if (fr.IsGoneaway) { sqlTargetFlag = sqlTargetFlag + "Goneaway"; } are you sure this is correct? Concatenating a column name to the other like that? Are you assuming the column name will be passed in followed by a comma? And anyway, isn't it going to blow up with a syntax error when you do the `UPDATE`? Or will the column name get passed as an empty string? Might be a good idea to check what garbage you are receiving through your parameters, assuming said checks aren't already in place at an upper layer. How can you be sure all callers will remember to pass your arguments exactly the way you expect them? Plus, just my two cents, but if my assumptions are correct, this is not a very fun API to work with.
\$\endgroup\$
4
  • \$\begingroup\$ Thanks for this. It's not quite as hairy as it looks: there is no user input here, and no other code which changes the table at the same time as this is running. And yes, that concatenation is correct - if there's a column called MyColumn there will also be one called MyColumnGoneaway. It's a bit rubbish, but it suits the purposes it was designed for. \$\endgroup\$
    – Bob Tway
    Jun 13, 2017 at 15:07
  • \$\begingroup\$ @MattThrower ah, okay. Good that there's this sort of convention in place. I will gladly edit out that part of the answer, but I think much of rest still stands. \$\endgroup\$
    – s.m.
    Jun 13, 2017 at 15:09
  • \$\begingroup\$ You can leave it in. Your assumption about it not being a fun API to work with is fair and accurate. This whole thing is the tip of a PITA iceberg and I hate it :( \$\endgroup\$
    – Bob Tway
    Jun 13, 2017 at 15:12
  • \$\begingroup\$ @MattThrower no problem. I'm currently dealing with Visual FoxPro, I hear you ;) \$\endgroup\$
    – s.m.
    Jun 13, 2017 at 15:13
2
\$\begingroup\$

How can I make this code clearer without turning it into a mess of flow control?

  • Single Responsibility Principle
  • Encapsulation

  • Address knows how to clean up its own properties.
  • AddressSqlMap know how to make Sql, given certain attributes.
  • Spral knows the arguments to instantiate a AddressSqlMap and knows how to call the database.

// client code
Address theAddress = new Address (aFileRowObject, aColumnName);
Sprail(theAddress);

// all the rest

    // NOTE: this method is not shown inside of a class    
    private void Sprail(Address theAddress)    {
    List<Suppression> addressMatches = GetMatchingRecordsFromDb(theAddress.Address, theAddress.Name);

    AddressSqlMap mappy;
    using (MySqlConnection con = new MySqlConnection(_connectionString))
    {
        foreach (Suppression s in addressMatches)
        {
           mappy = new AddressSqlMap(theAddress, s);
           if( mappy.HasSql)
           {
               con.Open();
               MySqlCommand cmd = new MySqlCommand(mappy.Sql, con);
               cmd.ExecuteNonQuery();
           }
        }
    }



// Wrap FileRow so we control how it's used. Also use a more "domain specific" name ( I hope).
public class Address {
    protected FileRow fr { get; set; }  // kept this for OP cross-reference
    protected FileRow frSanitized { get; set; } // for internal use only

    // assuming we only want sanitary properties

    public string Address    { get { return frSanitized.address; }
    public string Name       { get { return frSanitized.name; }
    public string ColumnName { get; protected set; }
    public bool   IsDeleted      { get { return frSanitized.IsDeleted; }}
    public bool   IsAddressEmpty { get { return string.IsNullOrWhiteSpace ( Address ); }}
    // and so on...


    public Address ( FileRow addressData, string columnName ) { 
        if ( addressData == null )
           return new ArgumentNullException ("Address constructor argument is null");

        if ( string.IsNullOrWhiteSpace(columnName) )
           return new ArgumentNullException ("columnName constructor argument is null");

        fr = addressData;
        frSanitized = new FileRow();
        Sanitize();
    }



    private void Sanitize () {
       frSanitized.address = SanitizeAddress();
       frSanitized.name    = SanitizeName();
       // and so on ... be sure to copy over all properties
    }

    private string SanitizeAddress() {
       frSanitized.Address = fr.Address ?? string.Empty;
       frSanitized.Address = fr.Address.ToLower();
    }
    // and so on...      
}



public class AddressSqlMap {
    protected Address     MyAddress  { get; set; }
    protected Suppression MySuppress { get; set; }
    public string Sql  { get; protected set; }
    public bool HasSql { get { return string.IsNullOrWhiteSpace(Sql); }}

    protected string  InsertSql = 
       @"INSERT INTO MyTable (`AddressKey`, `NameKey`, `{0}`) VALUES ('{1}', '{2}', '{3}')";

    protected string UpdateSql = 
       "UPDATE MyTable set {0} = {1} where SuppressionId = {2}";

    public AddressSqlMap ( Address theAddress, Suppression whatEver ) {
       if ( theAddress == null )
          return new ArgumentNullException ( "theAddress argument is null" );

       if ( whatEver == null )
          return new ArgumentNullException ( "whatEver argument is null" );

       MyAddress = theAddress;             
       SetTargets();      
       BuildSql();
    }

    private void SetTargets() {
       sqlTargetFlag = MyAddress.ColumnName;
       sqlTargetValue = "1";

       if (MyAddress.IsDeleted)  sqlTargetValue = "0";
       if (MyAddress.IsGoneaway) sqlTargetFlag = sqlTargetFlag + "Goneaway";
    }

    private void BuildSql() {
       Sql = string.Empty;

       if (MyAddress.IsAddressEmpty) { 
          Sql = BuildInsertSql();
          return;
       }

       if ( MyAddress.Name == MySuppress.NamedKey ) {
          Sql = BuildUpdateSql();
          return;
       }
    }

    private string BuildUpdateSql() {
        return string.Format( UpdateSql, sqlTargetFlag, sqlTargetValue, MySuppress.SuppressionId );
    }

    private string BuildInsertSql() {
       return string.Format( InsertSql, sqlTargetFlag, theAddress.Address, theAddress.Name, sqlTargetValue );
    }
}
\$\endgroup\$
1
\$\begingroup\$

I don't like there are multiple path to assign sql

A value can be overwritten. Seems like bad stuff could get through.

Updating like that is a injection risk. I don't care if it did not get entered by the user. Hack the address is a point of failure.

\$\endgroup\$

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.