The Trunk: Environments-cmm.37.mcz

Chris Muller uploaded a new version of Environments to project The Trunk:
http://source.squeak.org/trunk/Environments-cmm.37.mcz

==================== Summary ====================

Name: Environments-cmm.37
Author: cmm
Time: 20 December 2013, 2:42:33.974 pm
UUID: c2e36521-ef29-4aa3-9425-84793a2d007c
Ancestors: Environments-nice.36

Minor cleanups while trying to learn Environments.

=============== Diff against Environments-nice.36 ===============

Item was changed:
  ----- Method: AddPrefixNamePolicy class>>prefix: (in category 'as yet unclassified') -----
+ prefix: aString
+ ^ self new
+ setPrefix: aString ;
+ yourself!
- prefix: aString
- ^ self basicNew initializeWithPrefix: aString!

Item was removed:
- ----- Method: AddPrefixNamePolicy>>initializeWithPrefix: (in category 'as yet unclassified') -----
- initializeWithPrefix: aString
- self initialize.
- prefix := aString!

Item was changed:
+ ----- Method: AddPrefixNamePolicy>>name:do: (in category 'overriding') -----
- ----- Method: AddPrefixNamePolicy>>name:do: (in category 'as yet unclassified') -----
  name: aSymbol do: aBlock
  ^ (aSymbol beginsWith: prefix) ifFalse:
  [aBlock value: (prefix, aSymbol) asSymbol].
  !

Item was added:
+ ----- Method: AddPrefixNamePolicy>>setPrefix: (in category 'initialize-release') -----
+ setPrefix: aString
+ prefix := aString!

Item was changed:
+ ----- Method: BindingPolicy class>>namespace: (in category 'create') -----
+ namespace: aDictionary
+ ^ self namespace: aDictionary next: nil!
- ----- Method: BindingPolicy class>>namespace: (in category 'as yet unclassified') -----
- namespace: aNamespace
- ^ self namespace: aNamespace next: nil!

Item was changed:
+ ----- Method: BindingPolicy class>>namespace:next: (in category 'create') -----
+ namespace: aDictionary next: anImport
- ----- Method: BindingPolicy class>>namespace:next: (in category 'as yet unclassified') -----
- namespace: aNamespace next: anImport
  ^ self
+ namespace: aDictionary
- namespace: aNamespace
  policy: AllNamePolicy new
  next: anImport!

Item was changed:
+ ----- Method: BindingPolicy class>>namespace:policy: (in category 'create') -----
+ namespace: aDictionary policy: aNamePolicy
+ ^ self
+ namespace: aDictionary
+ policy: aNamePolicy
+ next: nil!
- ----- Method: BindingPolicy class>>namespace:policy: (in category 'as yet unclassified') -----
- namespace: aNamespace policy: aNamePolicy
- ^ self namespace: aNamespace policy: aNamePolicy next: nil!

Item was changed:
+ ----- Method: BindingPolicy class>>namespace:policy:next: (in category 'create') -----
+ namespace: aDictionary policy: aNamePolicy next: anImport
+ ^ self new
+ initializeWithNamespace: aDictionary
- ----- Method: BindingPolicy class>>namespace:policy:next: (in category 'as yet unclassified') -----
- namespace: aNamespace policy: aNamePolicy next: anImport
- ^ self basicNew
- initializeWithNamespace: aNamespace
  policy: aNamePolicy
+ next: anImport ;
+
+ yourself!
- next: anImport!

Item was changed:
+ ----- Method: BindingPolicy class>>null (in category 'create') -----
- ----- Method: BindingPolicy class>>null (in category 'as yet unclassified') -----
  null
  ^ self namespace: IdentityDictionary new!

Item was changed:
  ----- Method: BindingPolicy>>initializeWithNamespace:policy:next: (in category 'initialize-release') -----
+ initializeWithNamespace: namespaceDictionary policy: aNamePolicy next: anImport
+ namespace := namespaceDictionary.
- initializeWithNamespace: aNamespace policy: aNamePolicy next: anImport
- self initialize.
- namespace := aNamespace.
  policy := aNamePolicy.
  next := anImport!

Item was changed:
  Object subclass: #Environment
  instanceVariableNames: 'info imports exports declarations references public undeclared'
  classVariableNames: 'Default Instances'
  poolDictionaries: ''
  category: 'Environments-Core'!
 
+ !Environment commentStamp: 'cmm 12/20/2013 14:10' prior: 0!
+ I am a context for compiling methods. I maintain the namespace of classes and global variables that are visible to the methods compiled within me.
- !Environment commentStamp: 'cwp 5/27/2013 10:10' prior: 0!
- I am a context for compiling methods. I maintain the namespace of classes and gobal variables that are visible to the methods compiled within me.
 
  I have the following instance variables:
 
  info <EnvironmentInfo>
+ Metadata about me and the code I contain.
- Metadata about me and the code I contain
 
  imports <Import>
+ Rules for importing globals from other environments.
- Rules for importing globals from other environments
 
  exports <Export>
+ Rules for exposing globals to other environments.
- Rules for exposing globals to other environments
 
  declarations <IdentityDictionary>
+ Bindings for globals that have been declared inside me.
- Bindings for globals that have been declared inside me
 
  references      <IdentityDictionary>
  Bindings for globals that are used by methods compiled inside me.
 
  public <IdentityDictionary>
+ Bindings for classes that have been declared inside me, and which satisfy the export rules contain in 'exports'.
- Bindings for classes that have been declared inside me, and which satisfy
- the export rules contain in 'exports'.
 
  undeclared      <Dictionary>
+ Bindings for globals that are used by methods compiled inside me, but which aren't present in 'references' and couldn't be found via the rules in 'imports'.!
- Bindings for globals that are used by methods compiled inside me, but
- which aren't present 'references' and couldn't be found via the rules in
- 'imports'.!

Item was changed:
+ ----- Method: ExplicitNamePolicy class>>aliases: (in category 'create') -----
+ aliases: aCollection
+ ^ self new
+ setAliases: aCollection ;
+ yourself!
- ----- Method: ExplicitNamePolicy class>>aliases: (in category 'as yet unclassified') -----
- aliases: aCollection
- ^ self basicNew initializeWithAliases: aCollection!

Item was changed:
+ ----- Method: ExplicitNamePolicy class>>flattenSpec:into: (in category 'create') -----
- ----- Method: ExplicitNamePolicy class>>flattenSpec:into: (in category 'as yet unclassified') -----
  flattenSpec: anObject into: names
  anObject isSymbol ifTrue:
  [^ names at: anObject put: anObject].
  anObject isVariableBinding ifTrue:
  [^ names add: anObject].
  anObject isDictionary ifTrue:
  [^ names addAll: anObject].
  anObject do:
  [:ea | self flattenSpec: ea into: names]!

Item was changed:
+ ----- Method: ExplicitNamePolicy class>>spec: (in category 'create') -----
- ----- Method: ExplicitNamePolicy class>>spec: (in category 'as yet unclassified') -----
  spec: anObject
  | aliases |
  (anObject isKindOf: NamePolicy) ifTrue: [^ anObject].
  aliases := IdentityDictionary new.
  self flattenSpec: anObject into: aliases.
  ^ self aliases: aliases!

Item was removed:
- ----- Method: ExplicitNamePolicy>>initializeWithAliases: (in category 'as yet unclassified') -----
- initializeWithAliases: aCollection
- self initialize.
- aliases := IdentityDictionary withAll: aCollection!

Item was changed:
+ ----- Method: ExplicitNamePolicy>>name:do: (in category 'overriding') -----
- ----- Method: ExplicitNamePolicy>>name:do: (in category 'as yet unclassified') -----
  name: aSymbol do: aBlock
  ^ aBlock value: (aliases at: aSymbol ifAbsent: [^ nil])!

Item was added:
+ ----- Method: ExplicitNamePolicy>>setAliases: (in category 'initialize-release') -----
+ setAliases: aCollection
+ aliases := IdentityDictionary withAll: aCollection!

Item was changed:
+ ----- Method: RemovePrefixNamePolicy class>>prefix: (in category 'create') -----
+ prefix: aString
+ ^ self new
+ setPrefix: aString ;
+ yourself!
- ----- Method: RemovePrefixNamePolicy class>>prefix: (in category 'as yet unclassified') -----
- prefix: aString
- ^ self basicNew initializeWithPrefix: aString!

Item was removed:
- ----- Method: RemovePrefixNamePolicy>>initializeWithPrefix: (in category 'as yet unclassified') -----
- initializeWithPrefix: aString
- self initialize.
- prefix := aString!

Item was changed:
+ ----- Method: RemovePrefixNamePolicy>>name:do: (in category 'overriding') -----
- ----- Method: RemovePrefixNamePolicy>>name:do: (in category 'as yet unclassified') -----
  name: aSymbol do: aBlock
  ^ (aSymbol beginsWith: prefix)
  ifTrue: [aBlock value: (aSymbol allButFirst: prefix size) asSymbol]!

Item was added:
+ ----- Method: RemovePrefixNamePolicy>>setPrefix: (in category 'initialize-release') -----
+ setPrefix: aString
+ prefix := aString!

On 20 December 2013 20:42,  <[hidden email]> wrote:
Why make the AddPrefixNamePolicy mutable? That seems like a bad idea.
Unless there's some seriously major gain to be had, we should _not_
add setters. The "basicNew initializeWithFoo:" idiom makes it clear
that "AddPrefixNamePolicy new" will not result in a properly
initialised object. It avoids giving people the impression that it's
OK to monkey with the object's state. It's also the closest we can get
to immutability, given that the only way we can initialise an object's
state correctly is by sending it messages. (I realise the idiom's not
in SBPP, but sometimes we can improve on the classics.)

I like how you renamed lots of parameters to indicate the kind of
things they ought to contain ("aNamespace" -> "aDictionary"), but
please, no setters.

frank

On Fri, Dec 20, 2013 at 3:01 PM, Frank Shearar <[hidden email]> wrote:
Yeah, both things are simply documented best-practice conventions.
It's best not to call basicNew except for serialization frameworks or
otherwise with a good reason to, otherwise #initialize won't get
called from the one place it should be called from.

So that requires changing the method name from #initializeWith...: to
simply #set...:, which is Kent Becks "Constructor Parameter Method" on
page 25 of his book.

Now, I've heard the suggestion to name these methods as "myPrefix:
prefixString myNamespace: aDictionary", so that, should a developer
ever think he wants to use them, it will look and read awkwardly in
the calling code.

But that relies on a convention for "privatizing" that no other
private methods can afford.

Therefore, nothing besides the categories/protocols are either needed,
nor should be used, to indicate whether a method is ok to use from the
outside.

And, I confess, Beck says Constructor Parameter Methods should be
categorized as 'private' but I trumped him because
"initialize/release" is just as private (e.g., no one calls those from
the outside) plus.. that's what it is for!  And, it co-locates it with
#initialize where it belongs and easy to see all initialization under
one protocol.

Whew!

On 20 December 2013 21:16, Chris Muller <[hidden email]> wrote:
My point is that I don't think you should send #new at all, because
that suggests that you could have an AddPrefixNamePolicy with no
prefix.

> So that requires changing the method name from #initializeWith...: to
> simply #set...:, which is Kent Becks "Constructor Parameter Method" on
> page 25 of his book.

I have a copy of SBPP (sadly not in the same country as me anymore),
which is why I specifically suggested that sometimes we can improve on
the classics :)

> Now, I've heard the suggestion to name these methods as "myPrefix:
> prefixString myNamespace: aDictionary", so that, should a developer
> ever think he wants to use them, it will look and read awkwardly in
> the calling code.
>
> But that relies on a convention for "privatizing" that no other
> private methods can afford.

As does that #new prefix: cascade.

I almost entirely agree with you. Except that I think that calling
#basicNew and initialising within the #initializeWithFoo: methods is
superior because
* you don't need a setter (as in, you don't need a method that looks
and smells and tastes exactly like a classic
expose-my-state-for-mutation setter.
* #initializeWithFoo: tells you the ways you can construct a valid
instance, and if you want to construct an _invalid_ one you have to
work at it - actively subvert the API.
* #initializeWithFoo: tells you much louder exactly what it will do,
where #setFoo: (a) looks like a Java-style mutator and (b) suggests
that you can (partly!) _reinitialise_ an object. Worse, #setFoo: only
partially initialises the object, where #initializeWithFoo: completely
initialises an object.

So in the end we're arguing about two conventions that do more or less
the same thing. I don't see any _improvement_ in using #new, and in
fact see several (admittedly minor) downsides. So what's the point
then of "fixing" this?

(As I already said, I think the parameter renaming's a great idea.
It's just the #new setFoo: stuff that I dislike.)

frank

> 1) each class that declares instance variables has exactly one
> initialization method

Well then you can't have more than one constructor type then.  Not all
objects can follow a path down to a single constructor on the
class-side that ends up calling #initializeWtih:... In several cases,
you'll need #initializeWithThis: and #initializeWithThat:.  And, in
both cases, you want to call #initialize from the top of them.  That's
repeating yourself and unconventional.

> 2) each initialization method must leave the instance in a valid state

Of course, as does any initialization method like CPM methods.

> 3) each initialization method is named verbosely starting with
> 'initializeWith' and describes its parameters

The ONLY reason you call it "initializeWithThis:that:other:thing:"
instead of "setThis:that:other:thing:" is because you want to repeat
yourself by calling #initalize at the top of each method.

Also, your #1 said "exactly-one initialization method," but here you
say, "each initialization method."

> 4) each initialization method must call the initialization method of its
> super class

So who calls "super initialize" then?

> 5) the super initializer probably has a different name (see #2), so we send
> it to self, not super, to allow overrides

Of course, that's true for everything, it has nothing to do with this
(anti)pattern.

> 6) there is one or more class-side constructors which send #basicNew and the
> initialization message

Why basicNew instead of new?  Honestly, there's no good reason for
that and it forces you to repeat yourself in all of your
#initializeWith: methods.

> I've found that this is a subtle, but profoundly useful pattern. It

I'd call it an anti-pattern.

> eliminates the need for trivial setters just to get a valid instance
> created, which allows for better encapsulation of state and often full
> immutability. I've been using it long enough that I've started to see
> setters as a code smell.

You and Frank both need to read the CPM pattern because you missed it.
 You're confusing CPM as a setter.

No it doesn't.  #new is how you create initialized instances.
#basicNew is how a framework creates empty instances.

Neither "implies" anything about proper construction of the object.
For that you should look at the class-side protocols.  Maybe what you
want is to override #new as a shouldNotImplement or signal an Error or
something like that, but I don't think that's necessary -- again,
because the *protocols* define 'instance creation' methods.

When you use a class, you need look at it first -- understand its API
before you go sending messages.

>> Now, I've heard the suggestion to name these methods as "myPrefix:
>> prefixString myNamespace: aDictionary", so that, should a developer
>> ever think he wants to use them, it will look and read awkwardly in
>> the calling code.
>>
>> But that relies on a convention for "privatizing" that no other
>> private methods can afford.
>
> As does that #new prefix: cascade.

No it doesn't.  "initialize/release" methods are private, they're
called by the system and everyone knows that.

> I almost entirely agree with you. Except that I think that calling
> #basicNew and initialising within the #initializeWithFoo: methods is
> superior because
> * you don't need a setter (as in, you don't need a method that looks
> and smells and tastes exactly like a classic

There are no setters here Frank!  It's a Constructor Parameter Method,
not a setter method.  Setter methods do not begin with "set" as they
do in Java.  Setter methods do not set multiple variables like CPM's
do.

> expose-my-state-for-mutation setter.

What the heck does the #initializeWith: method do?  Same exact thing!
It's a mutator!

> * #initializeWithFoo: tells you the ways you can construct a valid
> instance, and if you want to construct an _invalid_ one you have to
> work at it - actively subvert the API.

Same exact thing with CPM's.  You cannot create an invalid instance
using this pattern.  You really need to read the pattern in Kent Becks
book.

> * #initializeWithFoo: tells you much louder exactly what it will do,
> where #setFoo: (a) looks like a Java-style mutator and (b) suggests
> that you can (partly!) _reinitialise_ an object. Worse, #setFoo: only
> partially initialises the object, where #initializeWithFoo: completely
> initialises an object.

Wrong.  It's not "setFoo", it's #setFoo:bar:diddle:daddle:.  Take a
look at Point class>>#x:y:.  It bypasses #new in favor of #basicNew
for performance, but otherwise follows the CPM pattern correctly.

> So in the end we're arguing about two conventions that do more or less
> the same thing. I don't see any _improvement_ in using #new, and in
> fact see several (admittedly minor) downsides. So what's the point
> then of "fixing" this?

Because your code should say everything just once.  For the
initializeWith pattern, you have to call #initialize from each one, so
you're repeating yourself.

The expectation is that #initialize is called when creating new
objects.  If new state is added to the object later that is not set by
the constructor, it will get initialized without having to remember to
cahnge the constructor.  Calling basicNew is just wrong.

I should not have called it an "anti-pattern" because it has the same
_principles_ as CPM.  It's just that the implementation repeats itself
in all the initializeWith methods ("self initialize") whereas CPM says
it only once.  CPM also reinforces the idea that there is trying to be
just _one way_ to create objects and that is with #new, not sometimes
new and sometimes basicNew.

That leaves the "set" vs. "initializeWith" nomenclature.  The former
is more easier to type and say, follows documented best-practice
convention, and leaves no doubt about what it does, and _only_ what it
does.  "set" means it _only_ populates the state.  "initializeWith..."
means any number of "actiony" stuff it might do, possibly calling
other builder methods, etc.  That building stuff should be abstracted
away from simply populating the vars to ensure a well-formed object.
Colins pattern commingles those two.



On Sat, Dec 21, 2013 at 12:02 PM, Chris Muller <[hidden email]> wrote: