Checking empty object

Question

Is there a better method for covering errors in my case? I am looking for best practice for now and future instances.

Foreseen errors that could arise:

• No attributes at all
• Some attributes could be missing
• Attribute value could be empty

---

// converting dom into array to use later
$fields = $dom->getElementsByTagName($field);
$arr['field'] = array();
foreach ($fields as $field) {
 $attributes = isset($field->attributes) ? $field->attributes : NULL;
 if (!empty($attributes)) {
 // the method getNamedItem($string) returns NULL if not found
 $name = !empty($attributes->getNamedItem('name')) ? $attributes->getNamedItem('name')->nodeValue : NULL;
 $id = !empty($attributes->getNamedItem('id')) ? $attributes->getNamedItem('id')->nodeValue : NULL;
 $field_name = !empty($attributes->getNamedItem('field_name')) ? $attributes->getNamedItem('field_name')->nodeValue : NULL;
 if (!empty($name) && !empty($id) && !empty($field_name)) {
 $arr['field'][$name][$id][$field_name] = $field->nodeValue;
 }
 }
}

Answer 1

I think it is cleaner to handle special cases early; your code winds up less indented.

// converting dom into array to use later
$fields = $dom->getElementsByTagName($field);
$arr['field'] = array();
foreach ($fields as $field) {
 $attributes = isset($field->attributes) ? $field->attributes : NULL;
 if (empty($attributes)) {
 continue;
 }
 $name = !empty($attributes->getNamedItem('name')) ? $attributes->getNamedItem('name')->nodeValue : NULL;
 $id = !empty($attributes->getNamedItem('id')) ? $attributes->getNamedItem('id')->nodeValue : NULL;
 $field_name = !empty($attributes->getNamedItem('field_name')) ? $attributes->getNamedItem('field_name')->nodeValue : NULL;
 if (empty($name) || empty($id) || empty($field_name)) {
 continue;
 }
 $arr['field'][$name][$id][$field_name] = $field->nodeValue;
}

This looks less complicated, at least to me, but it does exactly the same work.

You could also introduce a method to get the nodeValue or NULL, given $attributes and a name.

// converting dom into array to use later
$fields = $dom->getElementsByTagName($field);
$arr['field'] = array();
foreach ($fields as $field) {
 $attributes = isset($field->attributes) ? $field->attributes : NULL;
 if (empty($attributes)) {
 continue;
 }
 $name = nodeIfPresent($attributes, 'name');
 $id = nodeIfPresent($attributes, 'id');
 $field_name = nodeIfPresent($attributes, 'field_name');
 if (empty($name) || empty($id) || empty($field_name)) {
 continue;
 }
 $arr['field'][$name][$id][$field_name] = $field->nodeValue;
}

• dnelson: The second method looks very clean to me. Then I could reuse nodeIfPresent without having to have so many long if else shorthand statements.

Answer 2

You can most definitely improve some on this code. First off: DOMDocument::getElementsByTagName returns an instance of the DOMNodeList class, which in turn consists of DOMNode instances. Checking what methods and properties these classes have to offer is a good place to start.
Anyway, I'll go through your code and suggest changes, and then explain why I suggest to change your approach:

$fields = $dom->getElementsByTagName($field);
$arr['field'] = array();

Yes, I have some remarks here already:
Are you 100% sure that the value of $field is always going to be a lower-case string (as is recommended to pass to getElementsByTagName? If not:

$fields = $dom->getElementsByTagName(
 strtolower(
 trim($field)//trim, lower-case
 )
);

Next: $arr isn't declared as an array anywhere. This issues a notice, so perhaps consider writing:

$arr = array(
 'field' => array()
);

Or just use $arr = array();, because as far as I can tell, the $arr array will only have one single key "field", where all the data is stored. Why bother with that field key, then?

foreach ($fields as $field) {

Hold on a tick: as $field? But the variable $field was used as the value for the tag name. I'd consider using a different var name, perhaps something more descriptive for its contents: $node seems like a good fit.

 $attributes = isset($field->attributes) ? $field->attributes : NULL;
 if (!empty($attributes)) {

Why use a messy ternary followed by !empty($attribtues) when you are using an API that offers a clean from the bat:

if ($node->hasAttributes())
{
 //code here
}

I'd use the method, it's easy to read, and makes your code a lot easier to follow/maintain/debug.

 $name = !empty($attributes->getNamedItem('name')) ? $attributes->getNamedItem('name')->nodeValue : NULL;
 $id = !empty($attributes->getNamedItem('id')) ? $attributes->getNamedItem('id')->nodeValue : NULL;
 $field_name = !empty($attributes->getNamedItem('field_name')) ? $attributes->getNamedItem('field_name')->nodeValue : NULL;

Again: don't go ternary-crazy. $node->attributes is an instance of the DOMNamedNodeMap class, which indeed has a getNamedItem method. Check the docs. This method returns either a DOMNode instance or null, if the named item does not exist. Why use !empty? Wouldn't this be shorter:

$name = $node->attributes->getNamedItem('name');

The result is the same either way: $name will be an object, or null. The ternary just adds code-smell/noise/clutter.

 if (!empty($name) && !empty($id) && !empty($field_name)) {
 $arr['field'][$name][$id][$field_name] = $field->nodeValue;
 }

Again: I'd recommend you ditch the field key, and properly declare any array you plan on using. This code will emit notices (change your ini settings to display_errors=1 and error_reporting= E_STRICT | E_ALL).
The if is completely pointless either way, here's what I'd write:

if ($node->attributes->getNamedItem('name'))
{
 $name = $node->attributes->getNamedItem('name')->nodeValue;
 //assume $arr was declared as $arr = array() -> no field key
 $arr[$name] = isset($arr[$name) ? $arr[$name] : array();//create array if required
}
else
 continue;
if ($node->attributes->getNamedItem('id'))
{
 $id = $node->attributes->getNamedItem('id')->nodeValue;
 $arr[$name][$id] = array();//assuming id is unique, else => ternary like above
}
else
 continue;
if (!$node->attributes->getNamedItem('field_name'))//note the !
 continue;
//field_name exists
$arr[$name][$id][$node->attributes->getNamedItem('field_name')->nodeValue] = $node->nodeValue;

The code above could be shortened using a nested loop, though:

//inside foreach ($fields as $node)
$names = array(
 'name' => null,
 'id' => null,
 'field_name' => null
);
foreach ($names as $name => $val)
{
 if (!$node->attributes->getNamedItem($name))
 continue 2;//continue 2 breaks this loop, has the effect of continue in the outer loop
 $names[$name] = $node->attributes->getNamedItem($name)->nodeValue;
}
//set $arr

Putting it all together:

$fields = $dom->getElementsByTagName(
 strtolower(
 trim(
 $field
 )
 )
);
$names = array(
 'name' => null,
 'id' => null,
 'field_name' => null
);
$arr = array();
foreach ($fields as $node)
{
 if (!$node->hasAttributes())
 continue;//no attributes, skip
 foreach ($names as $key => $val)
 {
 $val = $node->attributes->getNamedItem($key);//saves typing
 if (!$val)
 continue 2;
 $names[$key] = $val->nodeValue;//<-- notably here
 }
 //if we get here, all attributes in $names were found
 //Assuming id is unique:
 if (!isset($arr[$names['name']])
 $arr[$names['name']] = array();
 $arr[$names['name']][$names['id']] = array(
 $names['field_name'] => $node->nodeValue
 );
}
var_dump($arr);//should be exactly what you're after

If none of the attribute values (like id, name and field_name) are guaranteed to be unique, you could write:

if (!isset($arr[$names['name']]))
 $arr[$names[$name]] = array(
 $names['id'] => array(
 $names['field_name'] => $node->nodeValue
 );
 );
else if (!isset($arr[$names['name']][$names['id']]))
 $arr[$names[$name]][$names['id']] = array(
 $names['field_name'] => $node->nodeValue
 );
else
 $arr[$names[$name]][$names['id']][$names['field_name']] = $node->nodeValue;

Which gives you the desired output, still.