This is the first project I've completedfront to back (thanks to all
the in this group).
It's a pretty simple report, and this version is my summary version.

I'm pretty excited about it, but am sure that there is much room for
improvement.
I'm looking for some constructive criticism so that I can grow and
learn more effectively.

********
CODE
********
$host = "xxx";
$uid = "xxx";
$pw = "xxx";
$db = "xxx";

$moCon = MySQL_connect($host,$uid,$pw);
MySQL_select_db($db,$moCon);

// colspans have been eliminated to enable a clean csv export.
// these are the empty TableData cell groups for accurate
alignment.
$fiveCells = "<td></td> <td></td> <td></td> <td></td> <td></td>";
//$threeCells = "<td></td> <td></td> <td></td>";
//$oneCell = "<td></td>";

// begin report table
echo "<table>";

// begin --Employee-- query
$qry = "SELECT employeeID, signature FROM Employee";
$empResult = MySQL_query($qry,$moCon);
$error = mysql_error();
if(mysql_error())echo "EmpError:" . $error . "<br/>";
// end Employee query

// begin 1st tier of looping
while ($empRow = MySQL_fetch_assoc($empResult))
{
// set empty strings for clean start per saleperson
$spCount = 0;
$spVal = 0;
$spCost = 0;
$spGP = 0;
//
$spBrkdCount = 0;
$spBrkdVal = 0;
$spBrkdCost = 0;
$spBrkdGP = 0;
//
$spStkCount = 0;
$spStkVal = 0;
$spStkCost = 0;
$spStkGP = 0;

$eID = $empRow["employeeID"];
// begin --Aggregate-- query
$where = "WHERE i.shipDate >'2008-05-21' AND i.employeeID=
$eID";
$qry = "SELECT i.shipDate AS date, i.salesOrderID AS so,
i.invoiceID AS invc, c.name AS co
FROM Invoice as i
LEFT JOIN SalesOrder as so USING (salesOrderID)
LEFT JOIN Company as c ON so.companyID=c.companyID
$where";
$agrResult = MySQL_query($qry,$moCon);
$error = mysql_error();
if(mysql_error())echo "Agregate Query Error:" . $error .
"<br/>";
// end Aggregate query

// begin sub-loop for aggregate info
while ($agrRow = MySQL_fetch_assoc($agrResult))
{
// set empty strings for clean start per order
$bOrdCount = 0;
$bOrdVal = 0;
$bOrdCost = 0;
$bOrdProf = 0;
//
$sOrdCount = 0;
$sOrdVal = 0;
$sOrdCost = 0;
$sOrdProf = 0;
//
$ordCount = 0;
$ordVal = 0;
$ordCost = 0;
$ordProf = 0;

$sID = $agrRow["so"];
$iID = $agrRow["invc"];
// begin --Detail-- query
$where = "WHERE r.salesOrderID=$sID and
r.invoiceID=$iID";
$qry = "SELECT r.soItemID, p.number AS pn,
r.stockID, sl.location AS loc, sl.consignmentCode AS consCd, r.shipped
AS qty, SOi.unitAmount, sl.unitCost, cc.codeCommRate AS consRate
FROM Reserve as r
LEFT JOIN Stockline as sl USING (stockID)
LEFT JOIN Part as p USING (partID)
LEFT JOIN SOItem as SOi USING
(salesOrderID,soItemID)
LEFT JOIN ConsignmentCodes as cc ON
sl.consignmentCode=cc.ConsignmentCode
$where";
$dtlResult = MySQL_query($qry,$moCon);
$error = mysql_error();
if(mysql_error())echo "Detail Query
Error:" . $error . "<br/>";
// end Detail query

//begin 3rd tier sub-loop for detail
info
while ($dtlRow =
MySQL_fetch_assoc($dtlResult))
{
// Determine whether to use
stock cost or ConsCost
if($dtlRow["consRate"]>0 &&
$dtlRow["consRate"]<100)
{
$itemCost =
($dtlRow["consRate"]*.01)*$dtlRow["unitAmount"];
} else {
$itemCost =
$dtlRow["unitCost"];
}

// begin calculated
field
$lineVal = ($dtlRow["qty"]*
$dtlRow["unitAmount"]);
$lineCost = ($dtlRow["qty"]*
$itemCost);
$lineProf = $lineVal -
$lineCost;
$ordCount++;
$ordVal += $lineVal;
$ordCost += $lineCost;
$ordProf += $lineProf;

// Determine whether item is
stock or brokered
$loc = $dtlRow["loc"];
$pattern = '/^WIP/';
if(preg_match($pattern, $loc))
{
//$b = 'Y';
$bOrdCount ++;
$bOrdVal += $lineVal;
$bOrdCost += $lineCost;
$bOrdProf += $lineProf;
} else {
$sOrdCount ++;
$sOrdVal += $lineVal;
$sOrdCost += $lineCost;
$sOrdProf += $lineProf;
}
}
//increment the SP totals
$spCount += $ordCount;
$spVal += $ordVal;
$spCost += $ordCost;
$spGP += $ordProf;
//
$spBrkdCount += $bOrdCount;
$spBrkdVal += $bOrdVal;
$spBrkdCost += $bOrdCost;
$spBrkdGP += $bOrdProf;
//
$spStkCount += $sOrdCount;
$spStkVal += $sOrdVal;
$spStkCost += $sOrdCost;
$spStkGP += $sOrdProf;
}
if($spVal > 0)
{
// Employee Header Row
echo '<tr class="GroupHdrRow">';
$empContent = '<td class="empName">' .
$empRow["signature"] . "</td>";
$empContent .= "<td>OrderCount</td>";
$empContent .= "<td>GrossSales</td>";
$empContent .= "<td>GrossCosts</td>";
$empContent .= "<td>GrossProfits</td>";
print $empContent;
echo "</tr>";

// Employee Brokered Totals Row
echo "<tr>";
$spBrkdTtl = "<td>" . "Brokered" . "</td>";
$spBrkdTtl.= '<td class="lftBord">' . $spBrkdCount .
"</td>";
$spBrkdTtl.= "<td>" . $spBrkdVal . "</
td>";
$spBrkdTtl.= "<td>" . $spBrkdCost . "</
td>";
$spBrkdTtl.= '<td class="rtBord">' . $spBrkdGP . "</
td>";
print $spBrkdTtl;
echo "</tr>";

// Employee Stock Totals Row
echo "<tr>";
$spStkTtl = "<td>" . "Stock" . "</td>";
$spStkTtl.= '<td class="lftBord">' . $spStkCount . "</
td>";
$spStkTtl.= "<td>" . $spStkVal . "</
td>";
$spStkTtl.= "<td>" . $spStkCost . "</
td>";
$spStkTtl.= '<td class="rtBord">' . $spStkGP . "</
td>";
print $spStkTtl;
echo "</tr>";

// Employee Gross Totals Row
echo '<tr class="GroupTtlRow">';
$spTtl = "<td>" . "Total" . "</td>";
$spTtl.= '<td class="lftBord">' . $spCount . "</td>";
$spTtl.= "<td>" . $spVal . "</td>";
$spTtl.= "<td>" . $spCost . "</td>";
$spTtl.= '<td class="rtBord">' . $spGP . "</td>";
print $spTtl;
echo "</tr>";

// Row for asthetics
echo "<tr>";
print $fiveCells;
echo "</tr>";
}
}

// end report table
echo "</table>";

MySQL_close($moCon);

Mo wrote:

> $fiveCells = "<td></td> <td></td> <td></td> <td></td> <td></td>";

Don't. If you need spacers in a table, do:

echo "<td colspan='5'></td>";

> $empContent = '<td class="empName">' .
> $empRow["signature"] . "</td>";
> $empContent .= "<td>OrderCount</td>";
> $empContent .= "<td>GrossSales</td>";
> $empContent .= "<td>GrossCosts</td>";
> $empContent .= "<td>GrossProfits</td>";
> print $empContent;

You will compact your code and make it more legible if you just output
everything from the beginning in those cases. There will be situations in
which you'll need to delay the output (as discussed), but in the code you
sent, there are lots of constructs like the one above. Try to avoid that if
you can.



Besides that, it's a long piece of spaghetti code - long, with no functions,
and quite a lot of nested loops. That, in the long run, tends to make code
ilegible. Please try to design your code so that it is more modular and
less spaghetti.



Cheers,
--
----------------------------------
Iván Sánchez Ortega -ivansanchez-algarroba-escomposlinux-punto-org-

Now listening to: Fly Gang - Calambre Techno - Te Gira la Cabeza (disc 1:
Sistema de Sonido) (1999) - [7] Feeling pt 1 (5:35) (86.750000%)

On Jun 17, 4:24 pm, Iván Sánchez Ortega <ivansanchez-...@rroba-
escomposlinux.-.punto.-.org> wrote:
> Mo wrote:
> > $fiveCells = "<td></td> <td></td> <td></td> <td></td> <td></td>";
>
> Don't. If you need spacers in a table, do:
>
> echo "<td colspan='5'></td>";
>
> > $empContent = '<td class="empName">' .
> > $empRow["signature"] . "</td>";
> > $empContent .= "<td>OrderCount</td>";
> > $empContent .= "<td>GrossSales</td>";
> > $empContent .= "<td>GrossCosts</td>";
> > $empContent .= "<td>GrossProfits</td>";
> > print $empContent;
>
> You will compact your code and make it more legible if you just output
> everything from the beginning in those cases. There will be situations in
> which you'll need to delay the output (as discussed), but in the code you
> sent, there are lots of constructs like the one above. Try to avoid that if
> you can.
>
> Besides that, it's a long piece of spaghetti code - long, with no functions,
> and quite a lot of nested loops. That, in the long run, tends to make code
> ilegible. Please try to design your code so that it is more modular and
> less spaghetti.
>
> Cheers,
> --
> ----------------------------------
> Iván Sánchez Ortega -ivansanchez-algarroba-escomposlinux-punto-org-
>
> Now listening to: Fly Gang - Calambre Techno - Te Gira la Cabeza (disc 1:
> Sistema de Sonido) (1999) - [7] Feeling pt 1 (5:35) (86.750000%)

The reason I avoided using colspan is because I've seen it cause
problems when it comes to trying to export the data to a .CSV format.
I do plan on getting this report to the point where that is an feature/
option, and am just trying to plan ahead.
Any further advice?

As for making the spaghetti code more modular, do you have any
suggestions?
What parts would be prime candidates for being written into a
function?
Also, are you referring breaking up the code into multiple pages and
using INCLUDE function? (I do plan to do this with the DB connection
parts of the code.)

Thanks.
~Mo

~Mo

Mo wrote:

[...]
> The reason I avoided using colspan is because I've seen it cause
> problems when it comes to trying to export the data to a .CSV format.

Uh, what?

How (on earth) are you doing the html -> csv conversion?

It's much easier, IMHO, to use a MVC pattern to output either HTML or CSV.

> As for making the spaghetti code more modular, do you have any
> suggestions?

Not really: how the code is structured depends on the design, not on the
coding itself. I do think that reading about design patterns would
enlighten you.

> Also, are you referring breaking up the code into multiple pages and
> using INCLUDE function? (I do plan to do this with the DB connection
> parts of the code.)

Yes, but you really need to plan that in advance, and for planning it
*right*, you'll just need some experience.

Cheers,
--
----------------------------------
Iván Sánchez Ortega -ivansanchez-algarroba-escomposlinux-punto-org-

Now listening to: Chambers Brothers - CSI: Crime Scene Investigation
(2002) - [11] Time Has Come Today (4:52) (87.444397%)

Mo wrote:
> This is the first project I've completedfront to back (thanks to all
> the in this group).
> It's a pretty simple report, and this version is my summary version.
>
> I'm pretty excited about it, but am sure that there is much room for
> improvement.
> I'm looking for some constructive criticism so that I can grow and
> learn more effectively.

I think mixing database interaction, php logic and html makes your code
hard to read and hell to maintain. Try seperating those three by using
functions (and later classes) and templates.

I guess the 3 queries could have been done with one query wich would
save lotsa time and make the code cleaner.

Arjen

Iván Sánchez Ortega schreef:
> Mo wrote:
>
> [...]
>> The reason I avoided using colspan is because I've seen it cause
>> problems when it comes to trying to export the data to a .CSV format.
>
> Uh, what?
>
> How (on earth) are you doing the html -> csv conversion?
>
> It's much easier, IMHO, to use a MVC pattern to output either HTML or CSV.
>
>> As for making the spaghetti code more modular, do you have any
>> suggestions?
>
> Not really: how the code is structured depends on the design, not on the
> coding itself. I do think that reading about design patterns would
> enlighten you.
>
>> Also, are you referring breaking up the code into multiple pages and
>> using INCLUDE function? (I do plan to do this with the DB connection
>> parts of the code.)
>
> Yes, but you really need to plan that in advance, and for planning it
> *right*, you'll just need some experience.
>
> Cheers,

If I may drop in. ;-)

Ivan, first: I totally agree with the suggestions you made to improve
his code. Sound advise.

But I really don't think it is good advise to throw MVC and
designpatterns at a (selfdeclared) newbie.
In my humble opinion every new coder needs to make a lot of own mistakes
before possibly seeing the merrit of such approaches.
It is like teaching somebody math Integral transformations first, and
adding and subtracting later.

[private opinion]
About design patterns I have a very personal opinion that they do not
add to the creativity and insight of the programmer using them. It is
more like a cookbook, and the programmer has to try to form/restate
his/her problem in such a way that a design pattern can be applied.
Every starting programmer has the right to make multiple mistakes and
learn from them: it activates selfcritism and some paranoia, which makes
you a better programmer. Reading about/using design patterns can be good
fun and a source of inspiration to an experienced programmer, but don't
throw them at starters.
Feel free to shoot me now. ;-)
[/private opinion]

Regards,
Erwin Moller

Mo wrote:
> On Jun 17, 4:24 pm, Iván Sánchez Ortega <ivansanchez-...@rroba-
> escomposlinux.-.punto.-.org> wrote:
>> Mo wrote:
>>> $fiveCells = "<td></td> <td></td> <td></td> <td></td> <td></td>";
>> Don't. If you need spacers in a table, do:
>>
>> echo "<td colspan='5'></td>";
>>
>>> $empContent = '<td class="empName">' .
>>> $empRow["signature"] . "</td>";
>>> $empContent .= "<td>OrderCount</td>";
>>> $empContent .= "<td>GrossSales</td>";
>>> $empContent .= "<td>GrossCosts</td>";
>>> $empContent .= "<td>GrossProfits</td>";
>>> print $empContent;
>> You will compact your code and make it more legible if you just output
>> everything from the beginning in those cases. There will be situations in
>> which you'll need to delay the output (as discussed), but in the code you
>> sent, there are lots of constructs like the one above. Try to avoid that if
>> you can.
>>
>> Besides that, it's a long piece of spaghetti code - long, with no functions,
>> and quite a lot of nested loops. That, in the long run, tends to make code
>> ilegible. Please try to design your code so that it is more modular and
>> less spaghetti.
>>
>> Cheers,
>> --
>> ----------------------------------
>> Iván Sánchez Ortega -ivansanchez-algarroba-escomposlinux-punto-org-
>>
>> Now listening to: Fly Gang - Calambre Techno - Te Gira la Cabeza (disc 1:
>> Sistema de Sonido) (1999) - [7] Feeling pt 1 (5:35) (86.750000%)
>
> The reason I avoided using colspan is because I've seen it cause
> problems when it comes to trying to export the data to a .CSV format.
> I do plan on getting this report to the point where that is an feature/
> option, and am just trying to plan ahead.
> Any further advice?
>
> As for making the spaghetti code more modular, do you have any
> suggestions?
> What parts would be prime candidates for being written into a
> function?

I'm just learning php, so some of my "advice" may be a bit off.

You have a mix of collating strings and just echoing them out a line at
a time. Consider using a heredoc to minimize.

echo <<<EOT
<tr>$empRow["signature"]<td><td>OrderCount</td><td>GrossSales</td>...
....


EOT;

It looks to me that most of your table could be in a single heredoc. It
will be much easier to read and organize your html.

You have a lot of loops where you create a new query, consider
preparing your sql outside the loops and doing the executing inside. You
may wish to look at PDO and prepare.


Break as much as you can into functions (or classes), you'll be heading
in that direction sooner or later anyways.

Jeff


> Also, are you referring breaking up the code into multiple pages and
> using INCLUDE function? (I do plan to do this with the DB connection
> parts of the code.)
>
> Thanks.
> ~Mo
>
> ~Mo

Erwin Moller wrote:

[...]
> But I really don't think it is good advise to throw MVC and
> designpatterns at a (selfdeclared) newbie.
> In my humble opinion every new coder needs to make a lot of own mistakes
> before possibly seeing the merrit of such approaches.
> It is like teaching somebody math Integral transformations first, and
> adding and subtracting later.

I agree with you, but I have to ask:

What should be appropiate for a coder with this little experience? OOP
principles? Basic algorithmics? Functional programming principles?
Flowcharting?

> [...] It is more like a cookbook, and the programmer has to try to
> form/restate his/her problem in such a way that a design pattern can be
> applied.

IMHO, one should never implement a design pattern "as per the book". They're
not recipes, they're ideas - they you to devise new ways to look at
your problem, and new approaches to build complex systems.

OK, maybe they are a bit overkill, but one has to start making errors
somewhere to get experience, right? ;-)

> Feel free to shoot me now. ;-)

/me loads his shotgun

:-P

--
----------------------------------
Iván Sánchez Ortega -ivansanchez-algarroba-escomposlinux-punto-org-

Un ordenador no es un televisor ni un microondas, es una herramienta
compleja.

Iván Sánchez Ortega wrote:
> Mo wrote:
>
> [...]
>> The reason I avoided using colspan is because I've seen it cause
>> problems when it comes to trying to export the data to a .CSV format.
>
> Uh, what?
>
> How (on earth) are you doing the html -> csv conversion?
>
> It's much easier, IMHO, to use a MVC pattern to output either HTML or CSV.
>
>> As for making the spaghetti code more modular, do you have any
>> suggestions?
>
> Not really: how the code is structured depends on the design, not on the
> coding itself. I do think that reading about design patterns would
> enlighten you.

I'm a new PHP programmer, but not new at programming. I'm unfamiliar
with this. Could you throw an example or a link my way? What I've just
googled up makes this look like a RAD. When I look at PHP Cake examples,
it looks like a collection of classes. I'm really confused.

My own style is to use a template and call a class depending on what
the page instructions are. The end result is to have reusable bits that
can be customized and reconfigured. Instructions could be sent on the
query string or be related to the page path. I'm not sure how that fits
in with this.

Jeff

>
>> Also, are you referring breaking up the code into multiple pages and
>> using INCLUDE function? (I do plan to do this with the DB connection
>> parts of the code.)
>
> Yes, but you really need to plan that in advance, and for planning it
> *right*, you'll just need some experience.
>
> Cheers,

Jeff wrote:
> I'm a new PHP programmer, but not new at programming. I'm unfamiliar
> with this. Could you throw an example or a link my way?

Here you go:
http://en.wikipedia.org/wiki/Design_...mputer_science)
http://en.wikipedia.org/wiki/Design_Patterns
http://en.wikipedia.org/wiki/Code_Complete
http://en.wikipedia.org/wiki/Anti-pattern

I think you'll have enough research material ;-)

--
----------------------------------
Iván Sánchez Ortega -ivansanchez-algarroba-escomposlinux-punto-org-

Un ordenador no es un televisor ni un microondas, es una herramienta
compleja.