BNodes are BNodes and should be treated as such #748
Conversation
ajs6f
force-pushed
the
BNodesAreBNodes
branch
4 times, most recently
from
bb4129c
to
86dca3a
Compare
|
|
||
| /** | ||
| * @param idTranslator | ||
| * @param context |
There was a problem hiding this comment.
Please change context to model
...and add meaningful descriptions.
There was a problem hiding this comment.
"Meaning descriptions"?
ajs6f
force-pushed
the
BNodesAreBNodes
branch
2 times, most recently
from
32341d5
to
b702d42
Compare
…removing unneeded reimpls of Functions.toStringFunction()
| @@ -141,6 +146,10 @@ | |||
|
|
|||
| protected abstract String externalPath(); | |||
|
|
|||
| protected Deskolemizer deskolemizer() { | |||
| return deskolemizer == null ? deskolemizer = new Deskolemizer(translator(), null) : deskolemizer; | |||
There was a problem hiding this comment.
Please unpack this single statement into its intelligible, multi-line parts.
There was a problem hiding this comment.
You should make such style considerations clear and get them into Checkstyle rules, if you intend to enforce them.
There was a problem hiding this comment.
That would be great if we could encode subjective measures of "readability" into checkstyles. I try not to encroach on other's style preferences, but when a style impedes personal readability, I raise the flag.
Thanks for the niggly update.
There was a problem hiding this comment.
If you are objecting to the ternary operator, then that should be encoded in Checkstyle. If, on the other hand, your qualm is genuinely subjective, it's not a valid criticism.
| } | ||
|
|
||
| return idTranslator; | ||
| return idTranslator == null ? idTranslator = |
There was a problem hiding this comment.
For the reasons above (and the fact that no logic is being changed here), please revert this update.
| * | ||
| * @author ajs6f | ||
| */ | ||
| public class Deskolemizer implements Function<Triple, Triple> { |
There was a problem hiding this comment.
Shouldn't Deskolemizer implement Deskolemizer?
...which would strongly argue for a different name of the implementing class, e.g. DeskolemizerImpl.
There was a problem hiding this comment.
Yes, this is an oversight. As far as the name goes, I don't know how much difference it makes. The only reason that there is a Deskolemizer interface is because the workings of this process tangle up into the kernel API, not because we have any reasonable expectation of more than one implementation of deskolemization. There is no clean way to introduce blank nodes, and this code is crap, but it's as good as I think we can expect to do. If you think it clearer, I can certainly change the name here to DeskolemizerImpl.
There was a problem hiding this comment.
I would either suggest:
- Change the implementation name to
DeskolemizerImpland have it implementDeskolemizer, or - Simply remove the
Deskolemizerinterface.
| } | ||
| private final Skolemizer skolemizer; | ||
|
|
||
| private final Service<Container> skolemAndHashCreator; |
There was a problem hiding this comment.
Given its type and use of skolemAndHashCreator in this class, I would find it more clear if the variable were named containerService.
There was a problem hiding this comment.
I disagree. containerService is just a restatement of the type of the param. skolemAndHashCreator actually explains how the param is used. I'd like to hear from someone else.
| existingAncestorPath.deleteCharAt(existingAncestorPath.length() - 1); | ||
| } | ||
| break; | ||
| String potentialPath = path.startsWith("/") ? path : "/" + path; |
There was a problem hiding this comment.
I am not sure I see the relevance of this update in this PR... but I guess it is an improvement
|
Resolved with: ceacc98 |
DO NOT MERGE YET