Problem
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);
}
Solution
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.
How can I make this code clearer without turning it into a mess of flow control?
- Single Responsibility Principle
- Encapsulation
Addressknows how to clean up its own properties.AddressSqlMapknow how to make Sql, given certain attributes.Spralknows the arguments to instantiate aAddressSqlMapand 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 );
}
}
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.