Subclass of EPStringContents are stateless no need to instantiate all the time

[Ducasse]

The classes EpNewStateVisitor and EpOldStateVisitor are stateless. They just return different strings using a visitor but for only two cases.

• It looks like this is an overkill to use visitor just for that.
• Now if we keep the visitor what we could do is at least share it because there is not need to create a visitor for any printOn: method.

@tinchodias what do you think?

[tinchodias]

Yes, iirc these are used to create the strings shown in the diff morph. Maybe they were more complex before and now there is no reason to have them.

[Ducasse]

Now a simple move would be to avoid to instantiate the visitor over and over since they are stateless.

[tinchodias]

I just browsed the code, and see that the superclass of both visitors (EpContentStringVisitor) has some auxiliary methods. These methods are only 3 on this case, but it made me remember this pattern that I used on other Epicea visitors that use either trait EpTCodeChangeVisitor or trait EpTEventVisitor, such as EpIconVisitor, EpMorphVisitor... An alternative was to place those methods on the EpEvent hierarchy like

oldStateString and #newStateString + auxiliary methods, but I chose to

have apart, on these visitors, maybe bad choise.

Yes, I also agree with the singleton

On Sun, Jun 12, 2022 at 9:13 PM StéphaneDucasse @.***> wrote:

Now a simple move would be to avoid to instantiate the visitor over and over since they are stateless.

— Reply to this email directly, view it on GitHub https://github.com/pharo-project/pharo/issues/11335#issuecomment-1153352785, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXHHKNRQCK56MF3NHUVMYDVO2DL7ANCNFSM5YRTWU7Q . You are receiving this because you were mentioned.Message ID: @.***>

[tinchodias]

Ah, now I think you didn't suggest singleton but moving to class side.

Well, we can try moving to the EpEvent hierarchy as extensions of package EpiceaBrowsers, and dependencies are kept intact.

Maybe, right now EpEvent objects look too much as simple data holders, like missing behavior which is distributed on these visitors.

[Ducasse]

What I was thinking is that we can have a simple SharedVariables for the visitors and use them

[tinchodias]

I didn't think on that alternative, it sounds also right.

BTW, this remembers me my unfinished pass to remove Ring1 dependencies, which lead me to clean up the change model API and variables a bit, specially instantiation. All in this commit: https://github.com/tinchodias/epicea/commit/11de3e79942403b5220f4ec7db9505721ab562ba

this was before your update to how Epicea handles the class definition source, but still I guess it's not too outdated.

[Ducasse]

Martin could you push you change to P12?