6

Android is designed in such a way that in order for a method to read a resource, it must have a access to a Context.

Since most of the classes in my application rely on the flexibility of string resources (e.g. changing language without having to change code, etc.), my simple solution was to pass a Context in their constructor, to provide resource access to each such class.

It doesn't make sense for me to only pass a string in the constructor, because the class needs the flexibility of access to a varying number of strings.

So, to simplify things, I only pass Context, and whenever a string resource is needed, I just use Context.getString().

Is this a sign of bad design?

Is there a better way to accomplish this?

uTubeFan
  • 6,664
  • 12
  • 41
  • 65
  • It does feel a bit messy doesn't it... – jondavidjohn Sep 20 '11 at 19:17
  • @jondavidjohn Every design is a result of some compromise. – uTubeFan Sep 20 '11 at 19:18
  • 2
    I didn't like that Context-passing when I first saw the Android API – Bozho Sep 20 '11 at 19:28
  • @Bozho It gets worse: a package name is totally static at runtime, yet it requires Context to be read: http://stackoverflow.com/questions/2002288/static-way-to-get-context-on-android/5114361#5114361 – uTubeFan Sep 20 '11 at 19:35
  • I disagree, I think the design works for what it was intended to do. The `Context` can be hard to keep track of sometimes, but it forces a better design from the developer. At least try to be constructive and add some reasoning when you post a comment. – Austyn Mahoney Sep 20 '11 at 21:01

4 Answers4

7

This is the Service locator pattern - you pass around a service locator (often called "Context") and get the required dependencies from it. It is not an anti-pattern, and is not that much of a bad design, but usually dependency injection is considered superior.

What you are doing is - passing the service locator even further down the object graph. What is advisable is to give each class only the dependencies it needs. So instead of passing Context in the constructor, you pass all the strings it requires. That way you won't be violating the Law of Demeter

Bozho
  • 588,226
  • 146
  • 1,060
  • 1,140
  • +1 for introducing the name of LoD. I try to follow LoD wherever I can, it so intuitive, but I didn't know that is named as such. – uTubeFan Sep 20 '11 at 19:37
  • Best answer here. I always wondered what pattern was being followed when using `Context` everywhere. – Austyn Mahoney Sep 21 '11 at 03:54
  • 2
    I dont agree. KISS (keep it simple) is better principle in such cases. – ggurov Sep 21 '11 at 06:47
  • @ggurov - if the strings are more than 3-4, then you have a point. Otherwise, it is equally simple to pass the strings themselves. – Bozho Sep 21 '11 at 07:28
  • 1
    @Bozho - with Application as singleton you dont pass anything and it is simpler. Actually it is simpler in any case, because if you pass any strings, you should know what strings the other object uses. If you use context, you dont care how many and which strings any object needs. And thats OOP. – ggurov Sep 21 '11 at 08:12
  • 1
    and when you need to unit test the whole thing, you start mocking 'the whole world' :) – Bozho Sep 21 '11 at 08:21
  • 2
    Well, the last thing which I expected to hear here is that data encapsulation is bad and leads to "mocking the whole world". GL with your understanding of OOP. – ggurov Sep 21 '11 at 09:52
  • @ggurov - wow, hold your horses. This has nothing to do with data encapsulation. Watch the presentation I linked. – Bozho Sep 21 '11 at 09:56
  • 1
    My previous comment was about data encapsulation. Strings which one object uses for showing messages to the user are its own stuff and it is bad practise to send them as parameters from someone else just because they are in the resources. From programmers viewpoint these strings belong to the object which uses them, not to its caller. The caller dont need to know anything about such things or soon the programmer will be completely lost in the code. – ggurov Sep 21 '11 at 10:29
  • true, but we aren't talking about exposing these fields to the clients of the object. I mentioned constructor - they are needed when the object is constructed. And I suggested passing exactly the strings needed, rather than a wrapper object containing all strings in the application. And while the particular question is about strings, I made a bit more general statement - for all kinds of dependencies. – Bozho Sep 21 '11 at 10:31
  • 1
    Parameters of the constructor or of other methods of the object - there is no difference about knowledge of such strings. – ggurov Sep 21 '11 at 11:06
  • Since this method in itself introduces global state, like singleton, i would strongly advise _against_ the usage of this, since it _lies_ about its dependencies, and generally makes it very hard to test (and mock). – Rasive Jan 18 '18 at 13:38
4

This is one of the rare occasions where a globally accessible singleton class may be better than passing the Context to every single class.

I would consider creating a singleton for localization, then use the Context inside of there (unless you need other aspects of the Context all over the place).

Of course, this is a matter of taste and preference. YMMV

naXa stands with Ukraine
  • 35,493
  • 19
  • 190
  • 259
Eric Farr
  • 2,683
  • 21
  • 30
  • There is no need to use a Singleton for localization. If you use the Context object, Android does it for you. Why reinvent the wheel with a Singleton that does the same thing? – Austyn Mahoney Sep 20 '11 at 21:02
  • @Austyn Mahoney Please explain how "Android does it for you"? I tend to accept Eric Farr's answer, by simply initializing a static Context member in Activity.OnCreate() because it simplifies implementation immensely. Do you have a better suggestion? – uTubeFan Sep 20 '11 at 21:57
  • The `Context` already has the localization features that the OP needs, why wrap it in a singleton? What do you mean by initializing a static Context member in `onCreate()`. How are you initializing the member, are you calling `getApplicationContext()`? – Austyn Mahoney Sep 20 '11 at 22:06
  • @Austyn Mahoney Yup. 1st, declare `public static Context appContext;`. 2nd, in onCreate() initialize it: `if (appContext == null) appContext = getApplicationContext();`. This makes it available to all classes in the project... Any problem with that? – uTubeFan Sep 20 '11 at 22:49
  • Discussions like this need to be moved to chat per SO policy. Let me know if you need my comment explained. – Austyn Mahoney Sep 21 '11 at 03:52
  • Passing the context should have been handled in the internal classes Android uses. The actual implementation just forces a viral propagation of the context through all the source code, even where it is unnecessary. – Igor Popov Jan 03 '12 at 18:46
1

I don't know if it's a bad practice to use it this way, but I did time comparison that took to load 200 light-weight object into Array-list with one of the Constructors taking Context as parameter, and other one getting the required context through static method like @Eric Farr suggested. End result was that accessing context through static method instead of passing to constructor was always 5-25% faster..

@Override
public void surfaceCreated(SurfaceHolder surfaceHolder) {
    initBlocks(map); // takes context through static method
    blocks.clear();
    initBlocks(getContext(), map); // pass context down to every block object
    mainthread = new GameThread(getHolder(), this);
    mainthread.setRunning(true);
    mainthread.start();
}


I/art: Background sticky concurrent mark sweep GC freed 1070(51KB) AllocSpace 
objects, 44(3MB) LOS objects, 27% free, 8MB/11MB, paused 27.450ms total 132.688ms
I/art: Background partial concurrent mark sweep GC freed 1638(66KB) AllocSpace 
objects, 36(3MB) LOS objects, 29% free, 9MB/13MB, paused 13.982ms total 184.900ms
I/art: Background sticky concurrent mark sweep GC freed 916(44KB) AllocSpace objects, 
37(3MB) LOS objects, 16% free, 11MB/13MB, paused 17.710ms total 127.963ms
**I/System.out: TIME IT TOOK TO PROCESS >> 2254155160**
I/art: Background partial concurrent mark sweep GC freed 1780(77KB) AllocSpace 
objects, 208(8MB) LOS objects, 38% free, 6MB/10MB, paused 22.850ms total 193.625ms
I/art: WaitForGcToComplete blocked for 5.262ms for cause Background
I/art: Background sticky concurrent mark sweep GC freed 959(46KB) AllocSpace objects, 
39(3MB) LOS objects, 24% free, 7MB/10MB, paused 15.160ms total 101.055ms
I/art: Background partial concurrent mark sweep GC freed 1873(71KB) AllocSpace 
objects, 32(2MB) LOS objects, 32% free, 8MB/12MB, paused 17.253ms total 154.302ms
I/art: Background sticky concurrent mark sweep GC freed 888(42KB) AllocSpace objects, 
36(3MB) LOS objects, 17% free, 10MB/12MB, paused 42.191ms total 179.613ms
I/art: Background partial concurrent mark sweep GC freed 1243(50KB) AllocSpace 
objects, 30(2MB) LOS objects, 27% free, 10MB/14MB, paused 29.423ms total 283.968ms
**I/System.out: TIME IT TOOK TO PROCESS >> 3079467060**
I/art: Background sticky concurrent mark sweep GC freed 343(16KB) AllocSpace objects, 
14(1232KB) LOS objects, 0% free, 21MB/21MB, paused 20.481ms total 162.576ms
I/art: Background partial concurrent mark sweep GC freed 396(13KB) AllocSpace 
objects, 4(280KB) LOS objects, 15% free, 21MB/25MB, paused 21.971ms total 243.764ms
I/art: Background sticky concurrent mark sweep GC freed 471(166KB) AllocSpace 
objects, 0(0B) LOS objects, 0% free, 37MB/37MB, paused 26.350ms total 133.655ms
I/art: Background partial concurrent mark sweep GC freed 356(10KB) AllocSpace 
objects, 1(10MB) LOS objects, 12% free, 27MB/31MB, paused 41.909ms total 299.517ms

I don't know about you, but I'll take an increase in performance (y).

Candleman
  • 11
  • 1
1

Sending regularly contexts as parameters and remember them is dangerous, read here: http://developer.android.com/resources/articles/avoiding-memory-leaks.html

Much better is to subclass Application and to make singleton with it (the application object is context too). Such sollution doesn't have significant drawbacks.

ggurov
  • 1,496
  • 2
  • 15
  • 21