review: refactor: PrinterHelper prints tabs automatically

[pvojtechovsky]

PrinterHelper can print tabs automatically. So all TokenWriter.writeTabs and related calls are useless and can be removed - simpler code!

[pvojtechovsky]

Can I remove all 45 `writeTabs()' calls from

• CommentHelper
• DefaultJavaPrettyPrinter
• DefaultTokenWriter
• PrinterHelper

Should I keep PrinterHelper#writeTabs() deprecated?

Will you accept such PR?

[monperrus]

Can I remove all 45 `writeTabs()' calls?

Interesting. I'd say the intelligent logic shouldn't go into PrinterHelper (only basic simple operation) but should go into DefaultTokenWriter, which also encapsulates the writeln. (and we would indeed remove writeTabs from PrinterHelper).
WDYT?

[pvojtechovsky]

... logic shouldn't go into PrinterHelper

I had the same opinion at the beginning too, but later I found that this tabWritting perfectly fits and belongs to PrinterHelper. Note that PrinterHelper is quite nice component independent on TokenWriter. I started to use PrinterHelper for writing of reports of analysis. And here it is quite natural too that tabs are written automatically after each EOL.

In different words: if PrinterHelper knows how much tabs it should use for indendation, why it should not use them and why it should be possible to generate text which ignores that indentation?

[monperrus]

I see your point. Before changing PrinterHelper, I'd propose to introduce an interface for it, together with an implementation class DefaultPrinterHelper. Next we could discuss whether new behavior should go as a new implementation class, or as a subclass of DefaultPrinterHelper.

[pvojtechovsky]

I'd propose to introduce an interface for PrinterHelper

Interesting idea - to have an interface.

Next we could discuss whether new behavior should go as a new implementation class, or as a subclass of DefaultPrinterHelper.

It sounds like you want to keep writeTabs() method in the interface. I do not think it is good idea.

Actually I think that writeTabs is pure internal method of PrinterHelper (or future DefaultPrinterHelper). Or why to bother clients with that method if nobody wants to call it? Or do you see any use case when somebody might need that? It is just source of problems. It is much simpler when PrinterHelper calls writeTabs internally - and correctly.

If you agree that writeTabs is internal then we should first merge this PR and then create interface. Otherwise we have to refactor that interface later again.

[monperrus]

I think that writeTabs/incTab/decTab go very well together. If one removes writeTabs, the usage of incTab/decTab becomes very implicit and hard to understand without reading the implementation.

Or do you see any use case when somebody might need that?

For instance, if one wants to write non-indented comments to have super clear separation // ----------------------------------------         tabContent() Regarding interfaces, I would remove writeTabs/incTab/decTab from TokenWriter, because 1) it has nothing to do with writing tokens (it is about formatting), and 2 clients can still call getPrinterHelper().decTab()

…

On 10/2/17 9:56 PM, Pavel Vojtechovsky wrote: I'd propose to introduce an interface for PrinterHelper Interesting idea - to have an interface. Next we could discuss whether new behavior should go as a new implementation class, or as a subclass of DefaultPrinterHelper. It sounds like you want to keep |writeTabs()| method in the interface. I do not think it is good idea. Actually I think that |writeTabs| is pure internal method of PrinterHelper (or future DefaultPrinterHelper). Or why to bother clients with that method if nobody wants to call it? Or do you see any use case when somebody might need that? It is just source of problems. It is much simpler when PrinterHelper calls |writeTabs| internally - and correctly. If you agree that |writeTabs| is internal then we should first merge this PR and then create interface. Otherwise we have to refactor that interface later again. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1566 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxDUkadpwZTYOyAuaQCSn_j9gR3vKSxks5soT_wgaJpZM4Pn1JV>.

[pvojtechovsky]

Regarding interfaces, I would remove writeTabs/incTab/decTab from TokenWriter

in that case we should also remove writeln and writeSpace.

But all these changes would completely kill the main idea why I come with TokenWriter!

C1) DJPP should care only about writing java tokens. No writeTabs/incTab/decTab/writeln/writeSpace. No formatting. At this place I fully agree with you that Tokens and formatting are different things and can be separated.
C2) some default implementation of TokenWriter can add formatting following the hardcoded or configurable rules

The problem of this concept is that it needs a lot of changes in DJPP, it needs a TokenWriter, which knows all these formatting rules, which are hardcoded in DJPP, it needs a lot of effort, and it will be not a baby step. You can also argue that maintenance of pretty printing becomes more complicated because printing will be split to 2 separated parts: 1) generating tokens, 2) formatting of result.

So I came with idea of
C3) TokenWriter which contains methods for java tokens including formatting tokens writeTabs/incTab/decTab/writeln/writeSpace.

The C3) allows to let DJPP care about formatting too, but it allows

1. client specific implementation of TokenWriter to influence this formatting very easily! To change only the formatting, which does not fit to customer needs and to keep others.
2. clever implementation which looks into origin source code of printed type and automatically uses the same formatting.
3. later optional remove of calling of formatting methods from DJPP - after we have implemented configurable formatting in TokenWriter.

clients can still call getPrinterHelper().decTab()

If DJPP would call getPrinterHelper().decTab() and other formatting methods too, then you see now that it would be IMPOSSIBLE to implement ANY customer specific TokenWriter, which would influence formatting... because the formatting commands would come from DJPP directly to PrinterHelper. We would not need TokenWriters at all... because what is TokenWriter good for then?

if one wants to write non-indented comments to have super clear separation
// ----------------------------------------

   tabContent()

You are right! Interesting. There is really a use case for that. Anyway I think it is corner case which can be handled by

int tabs = getNrTabs()
setNrTabs(0)
writeComment(...)
setNrTabs(tabs)

If one removes writeTabs, the usage of incTab/decTab becomes very implicit and hard to understand without reading the implementation.

I do not agree. Documentation like "All tokens are automatically printed with indentation defined by current number of tabs" would be understandable too.
And we all like simple API, and having method writeTabs which is needed only in few corner cases. which can be handled without that method too is not that case.

And the last argument: If we implement automatic writeTabs as standard feature of some default TokenWriter or DefaultPrinterHelper, then remaining writeTabs method whose calls will be ignored will be really really confusing. And even the example non-indented comments will not work and will be rendered different then client would expect - another confusion.

[monperrus]

C1) DJPP should care only about writing java tokens. No writeTabs/incTab/decTab/writeln/writeSpace. No formatting. At this place I fully agree with you that Tokens and formatting are different things and can be separated.

100% OK with the vision. However, I'm not sure that the notion of token and the design of TokenWriter is the right abstraction for achieving this.

remove all 45 `writeTabs()' calls

So let's do this to start with.

[pvojtechovsky]

C1) DJPP should care only about writing java tokens.

100% OK with the vision.

super

However, I'm not sure that the notion of token and the design of TokenWriter is the right abstraction for achieving this.

I agree, the TokenWriter can evolve. May be we will have 2 interfaces TokenWriter and FormatedTokenWriter soon. Or what is your idea?

remove all 45 `writeTabs()' calls

So let's do this to start with.

Thanks! I believe that it is a baby step, which breaks nothing and is on good way to make it simpler.
I will do it soon. What about 2 PRs?
1st merge this PR as it is
2nd new PR which removes 45 writeTabs calls
WDYT?

[monperrus]

I'd put everything in the same PR to clearly show the added value.

[spoon-bot]

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:6.0.0-20171002.224420-60

New API: fr.inria.gforge.spoon:spoon-core:jar:6.0.0-SNAPSHOT

Detected changes: 1.

Change 1

Name	Element
Old	method spoon.reflect.visitor.DefaultTokenWriter spoon.reflect.visitor.DefaultTokenWriter::writeTabs()
New	none
Code	java.method.removed
Description	Method was removed.
Breaking	binary: breaking

[pvojtechovsky]

writeTabs() calls were removed. There remains only PrinterHelper.writeTabs() method, which is deprecated and does nothing.
WDYT?

[monperrus]

This is good to me.