styleguides
styleguides copied to clipboard
SAP
Local classes are good
I wholeheartedly disagree with the point that local classes should be avoided: Global by default, local only in exceptional cases
It's taken me so long to raise this issue because I have a lot of thoughts on this and was debating whether to post a blog on SDN to get a wider audience's input. Well let's just get the ball rolling here and see what folks think.
To me locals embody the principle of only making things as public as they need to be. If a class is global it is effectively published and has an API and possibly other consumers that I now need to be mindful of. I see this as detrimental if a class is only responsible for functions of its enclosing class / report / function group.
I also disagree with most reasons given in the guide:
- Local classes hinder reuse because they cannot be used elsewhere.
On the one hand that's often the whole point! But on the other hand the statement is not correct because it is still possible to use (as in: consume) them elsewhere. In fact, you can even use them to create closures - whether this is a good idea is a different discussion :)
- Although they are easy to extract, people will usually fail to even find them.
Maybe it's just me, but many of my local classes grow from private methods that have become too complex or uncohesive. As such I don't see how functionality would be more or less 'findable' in a private method vs. a local class. And from a personal viewpoint, the search tool I use most is where-used, and it will find a local class that deals with table X just fine.
- A clear indication that a local class should be made global is if you have the urge to write tests for it.
Actually that's my exact criteria for extracting a private method into a new (local) class, I think this is also in either Clean Code or one of those books.
- A local class is a natural private cogwheel in its greater global class that you will usually not test.
Yes and no. A local class can be a simple utility class, or it can embody a lot of testable functionality that is completely private to the enclosing program/class/FM.
- The need for tests indicates that the class is independent from its surrounding and so complex that it should become an object of its own.
To me a local class is an object of its own. The only difference is that other classes have no business with it. And the author/owner retains the freedom to change and refactor at will.
I know one counter argument might be package encapsulation. But in my experience, only a small minority of customers use package checks, it's just too much effort to untangle the existing setup to be able to start using them. So it's a nice idea but mostly theoretical.
This section is the direct outcome of code reviews and expert discussions in our local group of three Scrum teams. For better understanding, here is the story.
We had one Scrum team that used local classes everywhere and for everything. All of their global classes would be only a couple of lines long and implement the factoy pattern:
CLASS some_class DEFINITION PUBLIC CREATE PRIVATE.
PUBLIC SECTION.
INTERFACES some_interface.
CLASS-METHODS get_instance
RETURNING
VALUE(result) TYPE REF TO some_interface.
ENDCLASS.
CLASS some_class IMPLEMENTATION.
METHOD get_instance.
CASE requested_type.
WHEN a.
result = NEW lcl_variant_a( ).
WHEN b.
" ...
ENDCASE.
ENDMETHOD.
ENDCLASS.
The local class include would then contain up to ~12 local classes that served the factory variants, plus up to an additional ~10 classes and interfaces for utilities, enumerations, and other helpers. This local codebase usually amounted to several thousands lines of code. The corresponding unit tests were even longer, inflated by custom mocks.
The team's main argument was that this pattern enforces decoupling because consumers are literally unable to depend on concrete implementations. In their eyes, the global class was more like a package, less like a real class.
While there is truth in that, there is also truth in the consequences we observed:
- The code structure was obscure to outsiders, and backlog estimations for little changes regularly amounted to several weeks of effort.
- Code duplication was all over the place, because local classes by definition cannot call the local classes in other global classes.
- Orientation, navigation, and debugging in the thousands-of-lines includes was tedious and annoying.
- We observed "lock-wars", when several developers needed to work on different aspects of the same global class, because only one can lock the include for editing.
- Finally, we found that making everything local ignores the original idea of local classes as "highly specialized niche solutions of no value to others".
Refactoring this code to global classes improved our situation: estimations went down from ~100 days to ~30 days for the same things, several people could work in parallel on related parts, speeding up total implementation speed, code duplication dropped, and people just felt much more comfortable around the code.
What you describe sounds like a more healthy use of local classes ("extract some private methods to own objects, e.g. to enable testing"). Nobody will object to that. So far, I'd say we have the same understanding and that it is more the formulation of the text (too harsh, doesn't make the points clear enough) we need to talk about?
Yes, I do think it's harshly formulated, it suggests that locals are all but verboten. "Take care when using local classes" is an approach I'd be comfortable with. Your explanation would be a great addition to this section.
I fully understand your scenario: the global class is the facade and the application is hidden within. I am aware of this danger - and guilty of it myself on occasion too. Sometimes it is justified, often it should not be.
Example where I thought it was appropriate: a class creates a bespoke multi-worksheet Excel using ABAP2XLSX. A single entry point and a single result. Inner classes generate and render worksheets with headers, body and other elements. It's highly specific, not used by anyone else, and a single piece of functionality that one person will work on at a time.
Another scenario is classic dynpro reports using SALV in an MVC framework built from local classes (though the Model is often global).
Writing this, I realise that specificity is an important factor to consider.
Let me reopen this as an anchor to think about some better text formulation and maybe add some details from our story. For example, I think the lock-wars thin isn't even addressed as of now.
Started a pull request with a suggestion how to rewrite the section to accomodate the findings and suggestions from above.
Hi Florian,
Overall very well explained. However I feel that this bit is too subjective:
Orientation, navigation, and debugging in very long local class includes is tedious and annoying.
I don't disagree, but it can go both ways which is why I find "tedious and annoying" subjective. Two counter-examples:
From personal experience at the other extreme, I've worked on a customer project that took the single responsibility principle to the extreme and favoured 'micro classes' (many with as few as 5-10 statements), resulting in literally thousands of global classes. At the end of a debugging session I'd have 30-40 tabs open in Eclipse and had no idea where up or down was anymore. The call stack functions perfectly well through both local or global classes, so to me debugging is not an issue.
Secondly, I use Ctrl-O to navigate in Eclipse and it works the same for me whether I'm in a large locals section, a global class, or a DPC class with over 10k LoC in hundreds of methods. In some instances it actually makes it easier: What if I know the method and can't remember which class it belongs to? Ctrl-O finds it in a local class or report include. Or a local interface with several implementations, Ctrl-O lists all the classes that implement a particular method and lets me choose the one I'm after. So there are some pluses to navigation in local classes too.
Perhaps write something along the lines of: Consider the impact of long source code segments on orientation, navigation and debugging. If the source code is too large, it could be a sign of low cohesion, and it may be a good idea to split some local classes and their dependents out into a new global class.
Just as an idea, another angle could be reports. For an ALV report (SALV), I'll typically create several view and controller classes and a variety of support functions. The only global classes are the model classes. Not sure if you want to include such an example?
My humble opinion on this topic (sorry but I am strongly against local classes :p)
Example where I thought it was appropriate: a class creates a bespoke multi-worksheet Excel using ABAP2XLSX. A single entry point and a single result. Inner classes generate and render worksheets with headers, body and other elements. It's highly specific, not used by anyone else, and a single piece of functionality that one person will work on at a time.
The point is that its not used by anyone else at the moment... but could be in the future. Imagine that the business would want the same extract from another report ? Two similar layouts depending on authorizations ? What would you do ? rewrite a copy of your "highly specific" worksheet in another local class ?
To me the point of OO programming is to foresee and anticipate this as much as possible to allow reuse. If you are doing the job, why not doing it in a reusable manner while you are on it ?
Another scenario is classic dynpro reports using SALV in an MVC framework built from local classes (though the Model is often global).
From a maintenance point of view SALV should be banned as it does not covers all possibilities (if the business wants to add complex eventing by example you would have to redo it using CL_GUI_ALV_GRID).
Why not building a bespoke template class inheriting CL_GUI_ALV_GRID for the specific GRID part + an interface for data selection. Then when creating a new report, you inherits your template with your specific data structure + an ABAP class implementing your interface for the selection.
I also have to add that local classes are highly dangerous as you can directly use global data from the report, select options, parameters... bypassing the visibility. I agree that it should not be done this way but I am seeing this too often.
The point is that its not used by anyone else at the moment... but could be in the future. Imagine that the business would want the same extract from another report
Don't future proof code - a scenario like this would be a good opportunity to refactor your existing code while moving it to the data dictionary. You have no idea at the time of writing if another business unit would want exactly the same functionality or something slightly different. Trying to build a one-size-fits-all class to begin with could leave you with some bad design decisions that are harder to unwind later on.
Oh please come on... there is a difference between making something clean and reusable and making it future proof (which is not possible).
Dont exaggerate :)
a scenario like this would be a good opportunity to refactor your existing code while moving it to the data dictionary
It is the same that if I replied "ok so lets hardcode everything and it will be a good opportunity to refactor later" ;)
I like a good discussion :-)
But I still don't agree. Your "at the moment" argument is one that can go both ways. Have a look at the idea behind YAGNI (grab a cup of coffee, worth taking the time to read and understand fully).
I can write a specific local class a lot quicker than a global, and - if it's well structured and clean - it's a doddle to extract and convert into a global class when the need arises. I've converted local classes to global many times, the basics take 5 minutes, the bigger effort comes in to make it "properly global" (or API-like if you wish), in other words generalising it and dealing with uncontrolled access. This is the potentially wasted effort that you'd have to put in up front, and it can be difficult to predict what need will arise if it doesn't exist at the time of coding so you might even cater for the wrong thing.
To use the Excel example, you illustrate my point quite nicely: which of the various scenarios you describe will come to be? Why spend effort in trying to cater for something that is not definite today?
So for low probability or unknown scenarios of re-use, let's not waste effort on someday-maybe and focus on the now. Modern development practices and editors make refactoring so, so much easier and safer than it used to be, and local classes are IMHO an important part of a flexible architecture. Global classes immediately make things more rigid, so I think local classes are worth considering when there is a high specificity and low probability of re-use.
But this all assumes modern development practices, ADT, unit testing and so on.
It is the same that if I replied "ok so lets hardcode everything and it will be a good opportunity to refactor later" ;)
Now who's exaggerating?
If you genuinely believe that something will be of use elsewhere then sure make it a global class, but this does not mean that all classes are equivalent and most definitely does not mean we should never make local classes.
Have a look at the idea behind YAGNI (grab a cup of coffee, worth taking the time to read and understand fully).
I read the link about YAGNI, thanks for it I didnt know about it and I think it is interesting. I quite agree with the autor. I think there is even a sentence which summarize what I am talking bout :
> Yagni only applies to capabilities built into the software to support a presumptive feature, it does not apply to effort to make the software easier to modify
I never talked about building presumptive features but at least making it possible.
To use the Excel example, you illustrate my point quite nicely: which of the various scenarios you describe will come to be? Why spend effort in trying to cater for something that is not definite today?
You are just making it easier to modify and evolve by building it in a global API like class. You could also go futher if needed : I already saw a template Excel development which is reading customizing table to build the Excel layout. Any user can create/modify their layout and style using a transaction.
Hmm, are you not mixing ease of modification with ease of re-use?
A local class is certainly easier to modify, as you don't have the constraint of a public API and you have complete control over its usage. That's one of my main motivators for using them.
Putting effort into a building a public API on the assumption that someone may, in future, want to use a class differently = presumptive feature.
Also consider the XP and agile principle of Minimum Viable Product. Leave the bells and shiny lights until they're called for.
Given that the difference in effort of making a class global during initial build vs. afterwards is almost negligible, a bigger benefit is knowing the exact interface that is needed if we delay globalisation until re-use becomes reality. So in effect this saves time and simplifies things. At least that's been my experience.
Bear in mind, much of the earlier points were about localising highly specific functions - things that are related only to the application at hand or for other reasons should not be available externally (legacy code). So from the outset this discussion was mainly concerned with functions with a low probability of reuse. Which really makes it a presumptive feature.
PS. For what it's worth, the Excel example I mentioned was a Web Dynpro app with a download button that generated a workbook of multiple sheets with specific features including formats, protected cells, tables, formulas, clickable links to navigate around the spreadsheet. No real value in making that generic before we know what the next 'similar' usage might be, and certainly no off the shelf tool for it.
Hmm, are you not mixing ease of modification with ease of re-use?
Closely linked in my opinion. If it is well built, it is easier to modify and to re-use.
A local class is certainly easier to modify, as you don't have the constraint of a public API and you have complete control over its usage. That's one of my main motivators for using them.
Not that much easier and as I mentioned it is also dangerous (global data / attributes). To me it is almost the same than using form routines in this aspect. Plus building a global class doesn't force you to support all use cases either.
I understand your point of view about building local class for highly specific functions but I still feel that it is more because you are confortable with it rather than it being the cleanest solution.
Given that the difference in effort of making a class global during initial build vs. afterwards is almost negligible, a bigger benefit is knowing the exact interface that is needed if we delay globalisation until re-use becomes reality. So in effect this saves time and simplifies things. At least that's been my experience.
Precisely. There have been times when I've started by making a global class only to find that I totally need to change my approach later on and the fact that everything is defined in the DDIC makes this more difficult. On the other hand I can start with a local class and easily transfer it to a global class later on.
as I mentioned it is also dangerous (global data / attributes)
No more dangerous than the things you can abuse with globally defined classes. At least with a local class that abuse is only going to have a localised effect.
To be clear there is a place and time for both. And as always, guidelines are guidelines, not an immutable truth.
Closely linked in my opinion. If it is well built, it is easier to modify and to re-use.
Hmm, I'm not convinced, I've seen plenty of coding horror behind a perfectly usable API and vice versa (why else would so many developers write their own BOPF wrapper? :-0 )
I don't see global data as a danger specific to local classes either. A global singleton class is essentially the same thing, and plenty of people mis/use those.
Hi there. Thanks for the lively discussion.
I would use local classes more often if the ABAP Testdouble framework would support local interfaces, which it unfortunately does not. That's why I am not making use of them at all. Building my own testdoubles each time is too time consuming in comparison to using the ABAP Testdouble framework. Having a unified way of mocking things also adds so much to a team, when maintaining each others code.
My two cents.
@pixelbaker I have to admit, I'd never heard of ABAP Testdouble framework before your post, thanks for the info! After a cursory glance over it, it seems like SAP's take on the Moq framework. Nice!
@pixelbaker interesting point.
While TDF is a useful framework, I think it's a bit heavy duty and can be overkill at times. See Writing simple, readable unit tests. TDF would just add an unnecessary layer in this scenario.
But I agree if you have a class that would benefit from TDF then that's a good argument for making it global.
@cjbailey Clean ABAP actually recommends to use the ABAP test doubles.
While @pokrakam's has a point that cl_abap_testdouble is overkill for simple input-to-output calculations, and can be quite verbose in its setups, it is usually still preferrable to requiring the reader to get familiar with own, custom doubles.
@pixelbaker We found it a worthwhile compromise to make the interfaces global, while using local classes to implement them. The interfaces alone are usually worthless, such that the probability that somebody outside will use them is negligible.
A lot of this discussion revolves around when we should design something for reuse ("API style"), and when we shouldn't ("private" / "local"). This question involves a lot more than only whether to make a class local or global. For example, we encounter the same line of thought in the section FINAL if not designed for inheritance. Should we make the you-ain't-gonna-need-it principle more prominent, both in the local classes section, and the guide overall?
Another story from our internal discussions. In our daily work, we often encountered code that we would have liked to reuse, but couldn't, because it was heavily safeguarded. Local classes, glocal friends, and private constructors prevented instantiating classes, while in-line instantiation of dependencies instead of dependency inversion made it nearly impossible to disentangle the safeguarded classes.
Our teams appreciated the notion of making clear that something is not good enough yet for reuse, or that it should not be used elsewhere because it was still too "liquid". However, they also found it a little disquieting to what considerable lengths some people would go to actively prevent others from reusing their code.
In an expert discussion, the majority preferred to keep the code global and accessible, with little to no active restrictions who may see and construct the classes. They agreed that we should prefer other constructs to discourage usage, such as package interfaces, some "API" property on objects, or some variant of "! DO NOT USE annotation.
@pixelbaker We found it a worthwhile compromise to make the interfaces global, while using local classes to implement them. The interfaces alone are usually worthless, such that the probability that somebody outside will use them is negligible.
@HrFlorianHoffmann That's an excellent idea. I like to give that a shot and see how that approach goes. Thanks for sharing Florian.
@HrFlorianHoffmann thanks for the valuable insights into how you guys work, I for one really appreciate it!
One thing I might clarify, when I talk about API, I do not mean it in the sense of "designed for reuse". It is just the way I view the distinction between public and protected/private components. Call it defensive coding if you wish - all public components form the API of a class. This viewpoint helps me to be more careful and mindful of scope and visibility at all times. This also relates to unit testing only public methods - I think of it as testing the class through its API.
Your description of "liquid" is spot on, that's exactly the analogy I was looking for. But I am surprised that people make access difficult, I must say it is not something I've experienced much. I would welcome someone taking a local helper class I wrote and turning it into a more generalised global class. To me this would be YAGNI at work: I built what I needed, the next person needs something similar and extends it with an API (in the re-usability sense). Similar effort, no guesswork, the end result suits both of our use cases, win-win.
I have now worked through the whole discussion and hey, that's a lively party. Could also be a good blog on SAP Community... anyhow, at the end I have to add my thinking here and hey, sorry for all the local class lovers. I don't see any reason to have a local class, execpt the statement I want to use make it necessary. I don't see an advantage of a local class. IF a global class is not reused or even cannot reused, where's the problem. It's just nearly the same code at another place on the storage.. but if there is a day and I need to add or reuse something I'm always open to anything in mind when used something global... So of course all the arguments with the visibility and all others I can understand, but do not agree from my past experiences.
Long discussion, but a point that I also found irritating (I have to admit that I only looked at the rules superficially so far). I think it is important to acknowledge in the ABAP world that there are actually two different "worlds", in which sometimes different (even opposite) recommendations make sense:
- SAP internal or Third Party application / AddOn development
- SAP customer development (usually much smaller, often only little "deltas" to standard functionality, much shorter "release cycle")
It seems to me that the differences between Mike Pokraka's and Florian Hoffmann's opinion can very well be understood when you assume that Mike's work is more in the "customer development" world.
In the "customer development" world (in which I also live now, after about 7 years at SAP), I find that for more than 90 or even 95% of code, reuse does not make sense. In our main system, for example, I see multiple historic, ambitious efforts to create "reusable" classes like ZCL_M_MATERIAL. But when I look in detail (where-use list), I find that there is actually very little real reuse. Most global class methods are only used in exactly one report.
Therefore, in the "customer development" world, I think it is more important not to clutter the "PUBLIC" namespace with things that very very likely, no-one will ever want to reuse anyway.
(And in the exceptional case that you come upon a reuse case later on, it is easy to convert from local to global class.)
I have to strongly agree with @pokrakam and use local classes for specific casses. This is the only way, to keep a class privat. The same discussion would be, why to use "private" methods,... it´s simply a part of information hidding. In our case, the class itself is the information.
