-1

I currently have a clunky and long-winded solution that I want to significantly streamline.

Without pasting excessive code, I'll just try to precisely describe what I'm doing and what I would like to do.

The page is used to add a club member, or a group of club members, to a specific event. There is the 'musicians' table with all members, and 'attendance' table with all attendance records for events. I also have a 'Rehearsals' table, with specifics for each event. 'Attendance' table has three columns: 'attCode', 'mus_number' and 'rhcode'.

A form on the page gives the user a drop-down menu. The choices allow to insert a group of members (four sections are offered), or all members, or any one individual member.

So, the code checks for GET or POST; if there's GET, it shows the form; if there's POST, it executes the INSERT code and redirects to a different page.

So, we first GET the rhcode (event code), then SELECT all members from the "members" table (I use that to populate the drop-down menu). We then check for POST, and if there's data, we go through series of IF / ELSEIF evaluations, checking which option was submitted from the drop-down menu. For each of the sections I create a separate SELECT query, WHERE section_ID is a specific value (based on a value from POST), and then iterate through the resulting array, building an INSERT query from the results, like this:

if (isset($musnumber) && ($musnumber == .2)) { // if lesser than 1, it's a section and not an individual

    $query_Musicians = "SELECT `number` FROM musicians WHERE instr_section == 20";
    $Musicians = mysqli_query($dbConnection, $query_Musicians) or die(mysqli_error());
    $insertSQL = "INSERT INTO attendance (musician_number, rhcode) VALUES ";
    $i = 0;
    while ($row_Musicians = mysqli_fetch_assoc($Musicians)) { 
         if ($i>0) {$insertSQL .= ", ";}
         $insertSQL .= "(".$row_Musicians['number'].", '".$rhcode."')";
         $i++;
   } 
    $Result1 = mysqli_query($dbConnection, $insertSQL) or die(mysqli_error());
}

I have this block repeat four times, once for each section (.3 for section 30, .4 for section 40, .7 for the entire list), and once more, if the '$musnumber' value form the POST >= 1 (meaning, it is an individual member, rather than a group). For that one, I simply INSERT that member info (member number, event number).

I've been trying to simplify this. My goal is, if possible, to do it in a single block. The logic is this:

IF  THEN

.1  INSERT into table WHERE source data Section == 10
.2  INSERT into table WHERE source data Section == 20
.3  INSERT into table WHERE source data Section == 30
.4  INSERT into table WHERE source data Section == 40
.7  INSERT into table WHERE source data Section == [anything] -- in other words, the whole list
>=1 INSERT a single record

So, while iterating through the source records, I would have to check for that source 'Section' column and, depending on the value submitted, only pick specific ones to add to the INSERT string.

Does anyone have a suggestion how to build that INSERT query in a single, simpler and more elegant block of code, rather than multiple separate IF / ELSEIF evaluations?

Predrag Vasić
  • 341
  • 1
  • 4
  • 14
  • Note: The [object-oriented interface to `mysqli`](https://www.php.net/manual/en/mysqli.quickstart.connections.php) is significantly less verbose, making code easier to read and audit, and is not easily confused with the obsolete `mysql_query` interface where missing a single `i` can cause trouble. Example: `$db = new mysqli(…)` and `$db->prepare("…")` The procedural interface is an artifact from the PHP 4 era and should not be used in new code. – tadman May 17 '20 at 19:15
  • 1
    **WARNING**: When using `mysqli` you should be using [parameterized queries](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php) and [`bind_param`](http://php.net/manual/en/mysqli-stmt.bind-param.php) to add any data to your query. **DO NOT** use string interpolation or concatenation to accomplish this because you have created a severe [SQL injection bug](http://bobby-tables.com/). **NEVER** put `$_POST`, `$_GET` or data *of any kind* directly into a query, it can be very harmful if someone seeks to exploit your mistake. – tadman May 17 '20 at 19:15
  • Note, it's `=` for comparisons in MySQL, not `==`. – tadman May 17 '20 at 19:17
  • Tip: A lot of problems can be detected and resolved by [enabling exceptions in `mysqli`](https://stackoverflow.com/questions/14578243/turning-query-errors-to-exceptions-in-mysqli) so errors resulting from simple mistakes made aren’t easily ignored. Without exceptions you must pay close attention to return values, many of these indicate problems you must resolve or report to the user. Exceptions allow for more sophisticated flow control as they can “bubble up” to other parts of your code where it’s more convenient to handle them. – tadman May 17 '20 at 19:19
  • You have an error. [`mysqli_error()`](https://www.php.net/manual/en/mysqli.error.php) needs one argument. Please consider switching error mode on instead. [How to get the error message in MySQLi?](https://stackoverflow.com/a/22662582/1839439) – Dharman May 17 '20 at 20:16

1 Answers1

2

None of this bashing around in PHP is necessary. Just insert the data directly:

INSERT INTO attendance (musician_number, rhcode)
  SELECT `number`, ? FROM musicians WHERE instr_section=?

Where you have two placeholders, one for rhcode the other for instr_section.

If you know you're matching against a bunch of possible values consider making this broader:

INSERT INTO attendance (musician_number, rhcode)
  SELECT `number`, ? FROM musicians WHERE instr_section IN (?,?,...)

Where you can expand the (?,?,...) part to as many placeholders as necessary. Note you'll still have to bind against these.

It's worth noting that PDO makes it a lot easier to dynamically bind, and easier still is to use an ORM that can take arrays.

tadman
  • 208,517
  • 23
  • 234
  • 262
  • Thanks for the answers and the suggestions. I have some procedural experience, but I really need to learn the object-oriented interface and PDO. It is clear to me that all the tinkering in this old, obsolete code is rather pointless...... – Predrag Vasić May 18 '20 at 00:13
  • PDO is a huge step up from `mysqli`. I'd also encourage checking out ORMs that can give you a layer of control well beyond writing SQL for *everything*. – tadman May 18 '20 at 00:14