Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:90359
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain ncsu.edu from 209.85.217.181 cause and error)
MIME-Version: 1.0
In-Reply-To: <CA+kxMuR4kcDYMAiYjFPp0+anFRgYHiFbxmZcG2-x0yAGttiP-w@mail.gmail.com>
References: <CADMz3wco4JNLHQV+NvnaA5npF=hTnzp0zdB3gK+oDbL6tTDuFA@mail.gmail.com>
	<CA+kxMuR4kcDYMAiYjFPp0+anFRgYHiFbxmZcG2-x0yAGttiP-w@mail.gmail.com>
Date: Fri, 8 Jan 2016 11:05:03 -0500
Message-ID: <CADMz3wea8fvTyR7n=Per+G0aGLxFkdHYAboNdMwaqOG9zg+uXA@mail.gmail.com>
To: Dan Ackroyd <danack@basereality.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] [RFC] Class Friendship
From: mdwheele@ncsu.edu (Dustin Wheeler)

Hi Dan,

On Fri, Jan 8, 2016 at 9:56 AM, Dan Ackroyd <danack@basereality.com> wrote:
>
> Hi Dustin,
>
> I try to avoid sounding too negative when people come up with ideas
> that I might not agree with. However....I feel obliged to give honest
> feedback earlier rather than later; I can't see any circumstances
> under which I'd vote for this RFC.
>

No need to hesitate with honest feedback. I understand that to be part
of the RFC process. This RFC is already close to "paying for itself"
from my perspective, in that it collects critiques and feedback to be
attached to the document so that in the event this is declined there
is "official" documentation of why. Currently, there are a handful of
list archives, chat logs, etc. scattered across the web. This process
bakes a more official opinion on the feature.

In addition, as part of doing this I learned a ton that I can use to
make further contributions myself and to help others get started.
Plus, I have an opportunity to go through the process, which is
another learning experience! :)

>
> Although they solve a particular problem, they do so in a way that has
> too many downsides. From the examples:
>
> class Fibonacci {
>     friend FibonacciTest;
>     ....
> }
>
> [snip]
>
> The classes that are exposing their protected properties have to know
> about what classes are going to be using them. It means if I want to
> add a new test similar to the FibonacciTest, I also have to edit the
> Fibonacci class itself.....this is pretty terrible.
>

I agree that by exposing protected properties to friended classes,
you're tightly-coupling an object to another. In the case of white-box
testing, friendship is a purposefully introduced smell, in my opinion.
Production code marked as coupled to test-cases is 100% a smell, but I
see this specific usage as a tactical strategy that is part of a
longer-running refactor. In-progress, I might leave these couplings in
place, but I would have sniffers that signaled these areas of the
project were targets for continued improvement.

Your testing example (''IntrospectableFibonacci'') is a great example
of alternative solution. I can't argue that friendship offers much
over this since we have deviated from the C++ implementation whereby
friends have access to ''private'' members. The only benefit I see
(currently and for this specific example, which is intentionally
trivial) is the class being statically marked for purposes of sniffing
/ analysis.

>
> That does not scale well - either in just the amount of code typed, or
> in how difficult it is to reason about the code. What ends up
> happening is that if a property needs to be exposed to friends once,
> it ends up being exposed to lots of friends.
>

If a class ends up exposed to many friends without real reason
semantic to a domain model, my opinion would be that it is a
misapplication of the feature. You'd be effectively shooting yourself
in the foot with the "tightly-coupled gun". I can make a similar
argument against a feature like Traits in that if I over-apply or
misapply traits, I am shooting myself in the foot with the "copy-paste
and duplication of code gun". I do agree with you that this feature
does expose an opportunity for abuse and headache, but I think that
can be said about many features in the language.

It's hard for me to dictate best practice versus simply the mechanics
of the feature. Do you think this type of information is worth baking
into the RFC to clarify intended usage of the feature?

>
> From the other example:
>
> class HumanResourceReport
> {
>     public function getFullName() {
>         // HumanResourceReport would not have access to protected
>         // members of Person if not explicitly listed as a friend.
>         return $this->person->firstName . ' ' . $this->person->lastName;
>     }
> }
>
> Again, this has a massive downside. The 'domain logic' of how to
> construct a full name from a first name and last name belongs in the
> person class. By allowing the individual elements to be accessed, the
> code is violating the 'separation of concerns'.
>

This is really an *overly* simplified example intended to express the
mechanics of the feature and not-necessarily "best practice". To a
degree, I agree with you that this specific type of presentational
logic might belong as part of a single object's API. Over time, as
that object grows, I see friendship as an expression of 'separation of
concerns', not a violation. Consider CQS (command-query separation),
whereby an object's API is constructed by either methods that return
observable state free of side-effect or methods that mutate state and
do not return anything. As that API grows, it may be appropriate to
refactor that model to separate concerns. In many cases, deepened
discovery in a domain might grant clarity that you've actually modeled
a conflation of ideas. In other cases, you might just have a case
where the abstraction *is* correct, but the API serves two concerns
(read and write, query and command). In these cases, you grab for
friendship as a means of expressing physical separation of concerns of
the command and query portions of a given object. Taken further, CQRS
(command-query responsibility segregation) expresses totally
segregated models for reads and writes across a system.

> When middle names are added to the Person object,
> the HumanResourcesReport will continue to generate full names as it
> has always done....until someone notices a files a bug report.

This would be a misstep modeling and failure in testing, in my
opinion. Granted, again, that this is an intentionally trivial example
so the argument will by extension seem trivial.

Keep in mind, that in this example Person might represent solely the
business rules and invariants of what it means to be a Person.
HumanResourcesReport is a separation of concerns describing what a
Person looks like in the domain of human resources. They are truly
separate concerns. In this way, I would argue that simply the addition
of middle name as an invariant of Person does not imply that it would
be incorrect (or a "bug") for the report to continue generating full
name as it always had. This is an advantage of friendship, not a
failing! Can it be abused? Of course!

The tests for HumanResourcesReport should dictate when it's time to
change behaviour of that query. Imagine if, instead, Human Resources
says, "We like what we see in the report, but we're now required by
law to include middle names as part of the full printed name.". We'd
write a test for the friended class to add this behaviour and that
would influence changes that might (or might not) need to be made on
Person.

>
> Apologies again for being so negative.
>

No need to apologize! I appreciate the feedback and look forward to
continued discussion. Good stuff!

Thanks

-- 
Dustin Wheeler | Software Developer
NC State University
m: mdwheele@ncsu.edu | w: 5-9786
"If you don't know where you're going, it's easy to iteratively not get there."