INSERT/REPLACE functionality

Review Request #12 — Created Nov. 7, 2012 and submitted

Felipe
Lunr
https://github.com/madsmash/lunr/tree/insert
lunr
Modified Query Builder to support INSERT and REPLACE functionality. Supports using extended inserts, SET and SELECT statements for the data to insert.

Set clause can be used in two ways, either with multiple calls like:
$builder->set(array('column1'=>'value1'))->set(array('column2'=>'value2'))...
AND/OR
$builder->set(array('column1'=>'value1', 'column2'=>'value2')

Values clause can be used in two ways as well, either with multiple calls like:
$builder->values(array('value1col1','value1col2','value1col3'))->values(array('value2col1','value2col2','value2col3'))....
AND/OR
$builder->values(array(array('value1col1','value1col2','value1col3'),array('value2col1','value2col2','value2col3')))


I will continue making small changes to spam your email until someone reviews it...
All unitests passed
Loading file attachments...

  • 0
  • 0
  • 6
  • 1
  • 7
Description From Last Updated
Felipe
pprkut
  1. I think the syntax of the functions is pretty much fine like this, just a bit of optimization work to do on the implementation.
    
    However, a few notes on coding style:
    
    The correct if clause syntax is like this:
    
    if ()
    {
        //something
    }
    elseif ()
    {
        //something
    }
    else
    {
        //something
    }
    
    All brackets and keywords are on a new line.
    Also, string concatenation:
    
    "string" . "string" . function() . $variable
    
    notice the surrounding spaces around the concatenation operator
    1. I thought brackets on the same line were fine for else and else if, will change it. The spaces around operators as well :)
    1. Typo?? according to tfd, one of the meanings is: A diffuse, comet-shaped image of a point source of light or radiation caused by aberration in a lens or mirror.
      which is exactly what I mean... has nothing to do with 'comma' being 'coma' in spanish...
  2. simply append the comma by default and do a trim() at the end
    1. I already knew there was a better solution when I was writing this... just didn't know which one :P 
  3. append by default, trim at the end
  4. interesting. I was going to suggest using something else, but it turns out using strpos is the fastest way to check something at the beginning of a string. Basically it's like this:
    
    strpos < strncmp < substr
    
    I'm pretty sure we use substr in a couple places, so I may have to adjust that ;)
    1. Your comment makes me think that you believe I don't research functions efficiency when I write my code... which is kind of true but still offensive 
  5. system/libraries/dataaccess/class.mysqldmlquerybuilder.inc.php (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    insert into select does not support DELAYED, so you have to implement some kind of mode checking.
    And since you have to do that anyway, you might just as well merge replace_mode in there as well.
    1. I don't want to merge with replace mode because REPLACE does support DELAYED when you use it with SELECT
  6. 
      
olivier
  1. I would just say that maybe for end-user it would be more understandable to name the methods and property
    insert_into or even insert instead of into.
    otherwise the global seems good. the tests will tell
    1. I prefer to keep it as into since INSERT doesn't allow any expressions in between it and into, you may get the impression it works like SELECT or DELETE if we make that change.
      let's see what Heinz thinks
    2. Why? What's confusing about:
      
      $this->db->into('table')
      $this->db->column_names(array());
      $this->db->values(array());
      
      $query = $this->con->query($this-db->get_insert_query());
      
      or
      
      $this->db->into('table')
      $this->db->column_names(array());
      $this->db->values(array());
      
      $query = $this->con->query($this-db->get_replace_query());
      
  2. 
      
Felipe
pprkut
  1. I still want replace_mode and insert_mode merged. There's no reason to handle them separately.
  2. putting empty lines between the blocks improves readability
  3. system/libraries/dataaccess/class.databasedmlquerybuilder.inc.php (Diff revisions 1 - 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    ok, I'd do a small hack to make the code simpler.
    If there is only one array level (so $values[0] is not an array), wrap it in an array ($values = array($values);).
    That way later on you only have to care about multidimensional arrays.
    I leave it up to you if you like it like that or not :)
  4. that was actually good before. It improves readability of the code if a group of assignments with reasonably similar length on the left side of the assignment has the '=' aligned.
    1. I can't do that with aptana's autoformatter
  5. why the wrapping?
    1. Removed comment wrapping from the auto formatter, sorry.
  6. keep the empty line pls
  7. 
      
Felipe
Felipe
Felipe
Felipe
Felipe
Felipe
Felipe
Felipe
olivier
  1. Ship It!
  2. 
      
Felipe
Felipe
Review request changed

Status: Closed (submitted)

Change Summary:

Merged into master.
Loading...