I am having a few issues building up quite a complex array. I make 4 different API calls and each of them return simpleXML data which I will show below.
With one API call I essentially get returned a bunch of Leads. For each Lead, I then need collect whatever data I can for that Lead. This is an example response of a Lead.
SimpleXMLElement {#289 ▼
+"Leads": SimpleXMLElement {#297 ▼
+"Lead": array:12 [▼
0 => SimpleXMLElement {#300 ▼
+"ID": "1266283"
+"State": "Current"
+"Name": "Test project"
+"Description": "Test project"
+"EstimatedValue": "2.00"
+"Date": "2016-05-26T00:00:00"
+"Client": SimpleXMLElement {#316 ▼
+"ID": "8549201"
+"Name": "Test Client"
}
}
]
}
}
I then have a bunch of client XML data. I need to link the Lead to the Client and collect some client data. This is an example data section for a Client.
SimpleXMLElement {#290 ▼
+"Clients": SimpleXMLElement {#298 ▼
+"Client": array:41 [▼
34 => SimpleXMLElement {#335 ▼
+"ID": "8549201"
+"Name": "Test Client"
+"IsProspect": "No"
+"BusinessStructure": "IT"
}
]
}
}
So at this point, I have Leads and I have matched the correct Client to a Lead and obtained data about that client. The nest two API calls refers to Quotes. One API call returns all current Quotes. Another API call returns draft quotes.
This is an example for current Quotes
SimpleXMLElement {#291 ▼
+"Quotes": SimpleXMLElement {#299 ▼
+"Quote": array:24 [▼
0 => SimpleXMLElement {#302 ▼
+"ID": "Q12415"
+"Type": "Quote"
+"State": "Issued"
+"Name": "Test Quote"
+"LeadID": "1266283"
+"Date": "2016-05-20T00:00:00"
+"Amount": "6100.00"
+"AmountTax": "1220.00"
+"AmountIncludingTax": "7320.00"
+"Client": SimpleXMLElement {#331 ▼
+"ID": "8549201"
+"Name": "Test Client"
}
}
]
}
}
This is an example for draft Quotes
SimpleXMLElement {#292 ▼
+"Quotes": SimpleXMLElement {#300 ▼
+"Quote": SimpleXMLElement {#303 ▼
+"ID": "Q12456"
+"Type": "Quote"
+"State": "Draft"
+"Name": "Test project"
+"LeadID": "1266283"
+"Date": "2016-05-26T00:00:00"
+"Amount": "2000.00"
+"AmountTax": "400.00"
+"AmountIncludingTax": "2400.00"
+"Client": SimpleXMLElement {#305 ▼
+"ID": "8549201"
+"Name": "Test Client"
}
}
}
}
Anyways, this is what I currently have
public function getForecastReportForLeads() {
$getCurrentLeads = Helper::getCurrentLeads();
$currentLeadsXML = new \SimpleXMLElement($getCurrentLeads);
$getCurrentClients = Helper::getClientList();
$currentClientsXML = new \SimpleXMLElement($getCurrentClients);
$getCurrentQuotes = Helper::getCurrentQuotes();
$currentQuotesXML = new \SimpleXMLElement($getCurrentQuotes);
$getDraftQuotes = Helper::getDraftQuotes();
$draftQuotesXML = new \SimpleXMLElement($getDraftQuotes);
$forecastArray = array();
$iterator = 0;
foreach($currentLeadsXML->Leads->Lead as $lead) {
$seconditerator = 0;
$thirditerator = 0;
$fourthiterator = 0;
$dateIdentified = date("d/m/Y", strtotime($lead->Date));
$forecastArray[$iterator]["leadData"] = array(
'LeadID' => (string)$lead->ID,
'DateIdentified' => $dateIdentified,
'Client' => (string)$lead->Client->Name,
'LeadName' => (string)$lead->Name,
'Owner' => (string)$lead->Owner->Name,
'Category' => (string)$lead->Category
);
foreach ($currentClientsXML->Clients->Client as $client) {
if((string)$lead->Client->Name == $client->Name) {
$forecastArray[$iterator]["clientData"] = array(
'BusinessStructure' => (string)$client->BusinessStructure,
'IsProspect' => (string)$client->IsProspect
);
$seconditerator++;
}
}
foreach ($currentQuotesXML->Quotes->Quote as $quote) {
if ((string)$lead->ID == (string)$quote->LeadID) {
$forecastArray[$iterator]["quoteDataIssued"] = array(
'QuoteID' => (string)$quote->ID,
'ProjectName' => (string)$quote->Name,
'Amount' => (string)$quote->Amount,
'AmountTax' => (string)$quote->AmountTax,
'AmountIncludingTax' => (string)$quote->AmountIncludingTax
);
$thirditerator++;
}
}
foreach ($draftQuotesXML->Quotes->Quote as $draftQuote) {
if ((string)$lead->ID == (string)$draftQuote->LeadID) {
$forecastArray[$iterator]["quoteDataDraft"] = array(
'QuoteID' => (string)$draftQuote->ID,
'ProjectName' => (string)$draftQuote->Name,
'Amount' => (string)$draftQuote->Amount,
'AmountTax' => (string)$draftQuote->AmountTax,
'AmountIncludingTax' => (string)$draftQuote->AmountIncludingTax
);
$fourthiterator++;
}
}
$iterator++;
}
return $forecastArray;
}
One thing to note is that sometimes a Quote does not have a LeadID. If this is the case, the Quote can be ignored which is something I am not currently handling.
Another thing to note is that I only want to get this data if the client the lead is related too has a IsProspect value of "No". This is something else I am struggling to get in place.
Any advice on getting this properly in place with my requirements, or how I can improve on my current code greatly appreciated.
Many thanks
Just get rid of counters, and code will be much more readable:
function getForecastReportForLeads()
{
/* ... */
$forecastArray = array();
foreach($currentLeadsXML->Leads->Lead as $lead) {
$reportItem = array;
$dateIdentified = date("d/m/Y", strtotime($lead->Date));
$reportItem["leadData"] = array(/* ... */);
foreach ($currentClientsXML->Clients->Client as $client) {
if((string)$lead->Client->Name == $client->Name) {
$reportItem["clientData"] = array(/* ... */);
if ('No' != (string)$client->IsProspect) {
continue;
}
}
}
foreach ($currentQuotesXML->Quotes->Quote as $quote) {
if ((string)$lead->ID == (string)$quote->LeadID) {
$forecastArray["quoteDataIssued"][] = array(/* ... */);
}
}
foreach ($draftQuotesXML->Quotes->Quote as $draftQuote) {
if ((string)$lead->ID == (string)$draftQuote->LeadID) {
$reportItem["quoteDataDraft"][] = array(/* ... */);
}
}
$forecastArray[] = $reportItem;
}
return $forecastArray;
}
And to omit clients with prospect!=No simply go for :
if ('No' != (string)$client->IsProspect) {
continue;
}
as shown above. That will work as long as there is a client for every lead.
And the second option. To improve that code you can try to move logic responsible for traversing XMLs and converting XML to arrays to separate classes. One class per data collection. So here is possible structure of such classes:
class LeadsCollection
{
function __construct(\SimpleXMLElement $leads){}
/**
* Returns list of leads in form of assoc arrays.
*
* @return array
*/
function asArray(){}
}
class ClientsCollection
{
function __construct(\SimpleXMLElement $clients){}
/**
* @return array
*/
function getClientByName($name){}
/**
* retub bool
*/
function isProspect($name){}
}
class QuotesCollection
{
function __construct(\SimpleXMLElement $quotes){}
/**
* @retun array|null - list of quotes related with given lead
*/
function getQuotesByLeadId($leadId){}
}
class DraftQuotesCollection
{
function __construct(\SimpleXMLElement $draftQuotes){}
/**
* @retun array|null - list of draft quotes related with given lead
*/
function getDraftQuotesByLeadId($leadId){}
}
And then your array creation code will be much much cleaner and more concise:
function getForecastReportForLeads() {
$leads = new LeadsCollection(new \SimpleXMLElement(Helper::getCurrentLeads()));
$clients = new ClientsCollection(new \SimpleXMLElement(Helper::getClientList());
$quotes = new QuotesCollection(new \SimpleXMLElement(Helper::getCurrentQuotes()));
$draftQuotes = new DraftQuotesCollection(Helper::getDraftQuotes(new \SimpleXMLElement(Helper::getDraftQuotes())));
$report = array();
foreach ($leads->asArray() as $lead) {
if ($clients->isProspect($lead['Client'])) {
continue;
}
$clientData = $clients->getClientByName($lead['Client']);
$quotesData = $quotes->getQuotesByLeadId($lead['LeadId']);
$draftQuotesData = $draftQuotes->getDraftQuotesByLeadId($lead['LeadId']);
$reportItem = array(
'leadData' => $lead,
'clientData' => $clientData,
);
empty($quotesData) || $reportItem['quoteDataIssued'] = $quotesData;
empty($draftQuotesData) || $reportItem['quoteDataDraft'] = $draftQuotesData;
$report[] = $reportItem;
}
return $report;
}
If for any reason, you don't want to put 4 new classes into your project, then instead you can try to breakdown your monolith method into several private methods. You can even follow "design" suggested above, but instead of having 4 classes, you can put all their methods into single one. Not the cleanest solution, but still better than having one big procedural in style method. And can make sense if you know that you will not need to reuse that code in any other place of your system.
Depending on the size of your data, your approach with iterating through whole data set again and again may be not most effective:
foreach ($currentClientsXML->Clients->Client as $client) {
if((string)$lead->Client->Name == $client->Name) {
/* ... */
It may pay off if you rearrange/group leads and quotes by variables, by which you want to access it, so by leadId. and client name.