How can I make my AS3/Air code better?

Question

Hello everyone this is my little Frankenstein code, don't make fun of it, it works! So you would pass in the table name and a data as an Associative array which are objects. I'm pretty sure this is not good code as I was and still am learning ActionScript. So what can I change or how would you guys make it better?

public function save(table:String,data:Object):void
        {
            var conn:SQLConnection = new SQLConnection();
            var folder:File = File.applicationStorageDirectory;
            var dbFile:File = folder.resolvePath("task.db");
            conn.open(dbFile);

            var stat:SQLStatement=new SQLStatement();
            stat.sqlConnection=conn;

            //make fields and values
            var fields:String="";
            var values:String="";
            for(var sRole:String in data)
            {
                fields=fields+sRole+",:";
                stat.parameters[":"+sRole]=data[sRole];
            }
            //trim off white space
            var s:String=new String(fields);
            var cleanString:String=s.slice( 0, -2 );

            //over here we add : infront of the values I forget why
            var find:RegExp=/:/g;
            var mymyField:String=new String(cleanString.replace(find,""));
            cleanString=":"+cleanString;

            var SQLFields:String=mymyField;
            var SQLValues:String=cleanString;

            stat.text="INSERT INTO "+table+" ("+SQLFields+")VALUES("+SQLValues+")";

            stat.execute();
        }

Answer 1

Someone once told me that a stupid idea that works isn't stupid. As programmer's our first goal is (often) to solve business issues; and as long as our code does that then we are successful. You don't need to apologize for code that works.

In terms of what I would do to change your snippet; I might just encapsulate it a bit more. Can the folder, dbFile, and db file name (task.db) be added either as properties to your class or arguments to the method?

Can you separate out the creation of the SQL Statement from the connection handling from your data parsing?

Answer 2

Some remarks,

• As said before you can factorise out all the db connection so you can reuse the function without rewriting it if you need to change the db name.

• Don't use new String() you can avoid it

• it's not usefull to clean white space between your field :a, :b is the same as :a,:b

• Some convention don't begin your local var name with an uppercase, and it's not usefull to reassign them to another var

If i don't get wrong after your //make fields and values can be rewritten for example as :

//make fields and values
 var fields:String = "";
 var values:String = "";
 var fieldSeparator:String = "";

 for(var sRole:String in data)
 {
  fields += fieldSeparator + sRole;

  var paramName:String = ":" + sRole;
  values += fieldSeparator + paramName;

  stat.parameters[paramName] = data[sRole];

  fieldSeparator = ",";
 }

 stat.text = "INSERT INTO " + table +" (" + fields + ") VALUES (" + values + ")";

 stat.execute();

Answer 3

The part where you build your query is quite a horror, to be honest. Half the code removes junk you added just a few lines before. This makes it hard to read and understand. Which is a sign of poor code quality. The following is far shorter and simpler:

        //make fields and values
        var fields:Array = [];
        for(var field:String in data) {
            fields.push(field);
            stat.parameters[":"+field]=data[fieldName];
        }
        var sqlFields:String = fields.join(",");
        var sqlValues:String = ":"+fields.join(",:");

        stat.text="INSERT INTO "+table+" ("+sqlFields+")VALUES("+sqlValues+")";

        stat.execute();