Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

First things first: you really shouldn't be using the mysql_* functions any more. There are better alternatives available, see for instance this question on Stack Overflow: Why shouldn't I use mysql_* functions in PHP? Why shouldn't I use mysql_* functions in PHP?

First things first: you really shouldn't be using the mysql_* functions any more. There are better alternatives available, see for instance this question on Stack Overflow: Why shouldn't I use mysql_* functions in PHP?

First things first: you really shouldn't be using the mysql_* functions any more. There are better alternatives available, see for instance this question on Stack Overflow: Why shouldn't I use mysql_* functions in PHP?

Source Link
Mat
  • 3k
  • 1
  • 22
  • 25

(Note: I don't know HTML or JavaScript enough to comment on the jQuery part or the layout.)

First things first: you really shouldn't be using the mysql_* functions any more. There are better alternatives available, see for instance this question on Stack Overflow: Why shouldn't I use mysql_* functions in PHP?

That won't directly help you here though. What you need to do is refactor your code a bit.

  • All the code blocks that output the individual rows are identical. That's a sure sign you should have created a function to do so. Create a function output_one_row($row) that takes one record and does the same thing as the blocks inside the while(mysql_fetch_assoc()) loops.
  • All the individual selects on Anunturi are identical except for the month queried. You could also create a function that produces the query for a month (given as a parameter) and returns the result set for that month. Using a bind variable would be even better.

Also don't use select *, select only the columns you will need to output. This might not matter for you right now, but if that table gets additional columns in the future, you'll be making your database do unnecessary work to retrieve data you're not using.

An order by for the date column could make sense, if that's relevant for your use case.

  • You're repeating the same code pattern for the twelve distinct months. You should instead have a single function that lays out the table for a given month, and call that function twelve times. The function could take only a month, and do the querying and output based on the two functions above.

Use an associative array to get the month name from the month number. (Or a PHP builtin if one exists that is properly localized for your environment.)

  • Don't do select * from ... then count in PHP if you only need the count. Rather, you should select count(*) from ..., i.e. let the database do the counting (which it can sometimes optimize from indexes, or actually have buffered already). Don't retrieve the whole dataset just to count it.

If you do that, your code would look like (pseudo-code):

function output_one_row($row) { /* ... */ }
function select_offers($month) {
 // build query for the specified month
 // execute it
 // return result set
}
function output_month_table($month) {
 // get result set for the month
 // check if there is any data (leave if not, returning 0)
 // output table header
 // output individual rows using output_row
 // output table footer
 // return number of rows printed
}
// "main"
$total_rows_output = 0;
for ($month in 1 .. 12) {
 $total_rows_output += output_month_table($month);
}
if ($total_rows_output == 0) {
 // print "no data found"
}

Notice that you don't need the global select count with this, you derive it from the individual months.

You can also avoid the num_rows calls completely by using a bit more state in the output_month_table, something like:

 $rows_processed = 0;
 while ($row = mysql_fetch_assoc(...)) {
 if ($rows_processed == 0) {
 // output month header
 }
 // output the table row
 $rows_processed++;
 }
 if ($rows_processed > 0) {
 // output month footer
 }
 return $rows_processed;

Now that's still twelve queries, when a single one could be enough. If you want to reduce the number of queries, it's possible too but requires a bit more state. The idea for that would be to select the data for the complete year, sorted by date, and include the month(DataIntroducere) in the selected columns (for convenience, you could derive it in PHP too).

Then, as you traverse the (ordered) result set, you do check if the current row is in the same month as the previous one. If it is, you simply output the row. If it isn't, you close the previous month's table, open the (now current) month's header, and output the row.

In pseudo-code:

$result_set = // select [columns] from your_table where [current year] order by date
$current_month = -1; // invalid month number
$total_rows = 0;
foreach ($row in $result_set) {
 if ($row["month"] != $current_month) {
 if ($total_rows != 0) {
 // output the HTML to close the previously output month table
 }
 $current_month = $row["month"];
 // output current month table header
 }
 // output row
 $total_rows++;
}
if ($total_rows > 0) {
 // close last month table
} else {
 // no data found at all
}
default

AltStyle によって変換されたページ (->オリジナル) /