4
\$\begingroup\$

I believe any data structure can be converted to RDF, and I wrote the subroutine below which sort of does this, at least for arbitrarily complex hashes.

However, I'm unhappy with it because:

  • $hash2rdf_count should not be global. I don't see an easy fix for this.

  • Where I comment "hopefully it's referring to a scalar at this point!", I'm not actually convinced. Are there better ways to check?

  • @triplets should not be global. I think I can fix this by passing it as a parameter, but is there an easier way?

Other notes:

  • In my specific use case, I'm using JSON::from_json($data) to convert JSON data to a hash, which I then feed to the subroutine below. I then print out @triplets and create an SQLite3 DB out of it. So it's really: JSON2hash, hash2rdf, rdf2sql. The entire program: https://github.com/barrycarter/bcapps/blob/master/bc-get-discogs.pl

  • JSON::GRDDL failed with an out-of-memory error on my data. My subroutine doesn't. Is there another "prepackaged" solution I could use?

  • I realize what I'm doing is a very minimal form of RDF.

=item hash2rdf($ref,$name="") 

Given a scalar/array/hash reference $ref, returns RDF triplets recursively. 

If $name is given, assigns name $name to $ref. Otherwise, creates a name 

Pretty much does what var_dump() does, only in RDF 

=cut 

sub hash2rdf { 
  my($ref,$name) = @_; 
  my($type) = ref($ref); 

  # if no type at all, just return self (after cleanup) 
  unless ($type) { 
    $ref=~s/\n/\r/isg; 
    $ref=~s/\"/\\"/isg; 
    return $ref; 
  } 

  # name I will give $ref if not given 
  unless ($name) {$name = "REF".++$hash2rdf_count;} 
  # TODO: making $hash2rdf_count global is bad 
  # TODO: making @triplets global is unacceptably bad (but just testing now) 
  # my(@triplets); 


  if ($type eq "ARRAY") { 
    # interim var 
    my(@l) = @{$ref}; 
    # push triplets for my children 
    for $i (0..$#l) {push(@triplets, [$name, $i, hash2rdf($l[$i])]);} 
    # return the name I gave myself 
    return $name; 
  } 

  if ($type eq "HASH") { 
    # interim var 
    my(%h) = %$ref; 
    for $i (keys %h) {push(@triplets, [$name, $i, hash2rdf($h{$i})]);} 
    return $name; 
  } 

  # hopefully it's referring to a scalar at this point! 
  return $$ref; 
} 
\$\endgroup\$
  • \$\begingroup\$ Could you provide an example of usage and expected output? \$\endgroup\$ – 200_success Dec 21 '13 at 23:52
3
\$\begingroup\$

I'm not sure what you're trying to do, so I'll just review the code itself, not what you're trying to do.

I looked into the whole script on GitHub and was shocked to see that you didn't use strict; use warnings. This would usually abort any review, but let's continue for the sake of the argument.

I think it's very nice that you've used POD documentation for your script.

Let's look at this excerpt for a moment:

unless ($type) { 
    $ref=~s/\n/\r/isg; 
    $ref=~s/\"/\\"/isg; 
    return $ref; 
}
  1. ref $foo can be false, but a reference (consider $foo = bless \$bar, "0"). Use either length ref $foo or reftype $foo, where reftype is imported from Scalar::Util.
  2. unless is widely considered to be half a design error. if (not ...) is cleared most of the time.
  3. On each of your regexes you specify the flags /isg.
    • The /i does case insensitive matching but you don't match anything that has a case. As this flag reduces performance, you should use it sparingly.
    • The /s flag causes the . to match any character including newlines. It is generally a good idea to use this flag (the default behavior of . is better written as [^\n]). However, you do not use the . metacharacter, so /s is useless here.
  4. In the pattern, " does not have to be escaped as it isn't a metacharacter.
  5. Use proper spacing around operators (usually a single space) to improve readability.

Suggested refactoring:

if (not length $type) { 
    $ref =~ s/\n/\r/g; 
    $ref =~ s/"/\\"/g; 
    return $ref; 
}

Now let's take a look at unless ($name) {$name = "REF".++$hash2rdf_count;}:

  1. As per Larry's style guide, you shouldn't use a semicolon if the closing brace is on the same line.
  2. Never use the conditional (...) {BLOCK} form on a single line. Either put the statement on a line of its own, or use the statement modifier form:

    $name = "REF" . ++$hash2rdf_count unless length $name;
    

    or

    if (not length $name) {
        $name = "REF" . ++$hash2rdf_count;
    }
    
  3. If you only want to overwrite $name if it is undef, you could either test for definedness: if (not defined $name) or since perl 5.10, you can use the defined-or operator //:

    $name //= "REF" . ++$hash2rdf_count;
    

What to do about the $has2rdf_count? The simplest solution that does not involve globals is to declare it outside of the subroutine:

my $hash2rdf_count = 0;
sub hash2rdf {

A better solution (since perl 5.10) is to use state variables:

use feture 'state';
...
state $hash2rdf_count = 0;
$name //= "REF" . ++$hash2rdf_count;

A state variable belongs to a certain scope like my variables, but is initialized only at the first execution.

However, it may be better to take this as another argument (will be discussed later).

my(@l) = @{$ref} there is no need to perform a copy of the array, and even if you feel the need to do that, please don't use a bad variable name like @l. Especially the charcterl l should be avoided on its own. Also, variable names don't have to be so short. Given an array reference, we can access an entry like $ref->[$i] and get the highest index like $#$ref (or with curlies: $#{$ref}).

By now we know how a line like for $i (0..$#l) {push(@triplets, [$name, $i, hash2rdf($l[$i])]);} can be improved. But I would like you to call builtins like push without parens: They aren't subroutines, but operators.

With a statement modifier:

push @triplets, [$name, $_, hash2rdf($ref->[$_])] for 0 .. $#$ref;

Without:

for my $i (0 .. $#ref) {
    push @triplets, [$name, $i, hash2rdf($ref->[$i])];
}

And then there is a variant with map:

push @triplets, map { [$name, $_, hash2rdf($ref->[$_])] } 0 .. $#$ref;

The same holds for the HASH-reference case. Recommended refactoring there:

for my $key (keys %$ref) {
    push @triplets, [$name, $key, hash2rdf($ref->{$key})];
}

Note that I'm not using $i here, as the hash key won't generally be an integer.

How can we solve the problem that @triplets is a global? We take a reference to that array as another argument:

sub hash2rdf {
    my ($ref, $triplets, $name) = @_;
    ...
    push @$triplets, ...

This means that we have to change the way how we call ourselves when recursing:

hash2rdf($ref->[$i], $triplets)

On the outside, we call the subroutine like

hash2rdf(\@releases, \my @triplets, "root");

Eww, that is ugly (out-arguments always bother me). Let's rename hash2rdf to hash2rdf_inner, and create a wrapper. This is also a good opportunity to pass $has2rdf_count as an argument, and to rename the wrapper to something better.

sub ref2rdf {
  my ($data, $name) = @_;
  my $count = 0;
  my @triplets;
  ref2rdf_inner($data, \@triplets, $name, \$count);
  return @triplets;
}

sub ref2rdf_inner {
  my ($ref, $triplets, $name, $count_ref) = @_;
  ...
  $name //= "REF" . ++$$count_ref;
  ...    
  ref2rdf_inner($ref->[$i], $triplets, undef, $count_ref)
  ...
}

We could now dump a data structure like

use feature 'say'; # say is like print, but with newline

for my $triplet (ref2rdf(\@releases, "root")) {
  say join "\t", @$triplet;
}

Happy refactoring!


Edit: This code still has unresolved issues, e.g. hash keys can contain arbitrary strings which may include NUL, LF, tabs, spaces, or quotes. Those would have to be properly sanitized as well.

\$\endgroup\$
  • \$\begingroup\$ Thank you! I didn't realize until just now that Perl had added static variables and default assignments. I've been using Perl for ~20 years and have been waiting for those features! \$\endgroup\$ – Barry Carter Dec 25 '13 at 16:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.