17

Came across this coding standard from Oracle:

Do not use a static array of strings.

What is the reason for this recommendation?

Zack
  • 3,819
  • 3
  • 27
  • 48
  • 1
    A few of these tips are there mainly for readability purposes. I guess maybe the idea is that you would use an enum with String values or something along those lines instead? – Cricket Aug 15 '16 at 17:17
  • 1
    The reason might just be valid in the context of the Oracle retail applications this document refers to but it might be to counter bad style like overuse of static variables (and strings might be the most common case). – Thomas Aug 15 '16 at 17:18
  • 2
    A lot of the recommendations in this document are quite questionable nowadays. The most obvious being "Do not concatenate strings. Use StringBuilder instead." – Stefan Zobel Aug 15 '16 at 17:21
  • @StefanZobel agreed, that's what I was thinking too. Definitely seems a bit dated, but probably a reason for the static array. I just can't seem to think of it (or a use case for a static string array, for that matter). – Zack Aug 15 '16 at 17:23
  • 3
    @Cricket I think the reason could be this predates 1.5 (which is when enums were added, I believe). – Zack Aug 15 '16 at 17:26
  • 1
    @StefanZobel That's a reasonable statement, it just didn't include the context of when to follow the rule: when concatenating within a loop (it'll generate multiple `StringBuilder` instances). As for the static `String[]`, my only guess is it's refering to the use of constants, with arrays not having descriptive identifiers. But it's just a guess. – Vince Aug 15 '16 at 17:28
  • @VinceEmigh are you saying this could refer to using a static string array that is comprised of constant values? – Zack Aug 15 '16 at 17:30
  • kinda similar, may be this can help you: http://stackoverflow.com/questions/2842169/why-are-public-static-final-array-a-security-hole – Shahid Aug 15 '16 at 17:34
  • 1
    @Shahid Possibly. But the recommmendation says noting about `public` static final String[] – Stefan Zobel Aug 15 '16 at 17:36
  • 2
    @VinceEmigh That might have been the intended context. But I find this interpretation a bit stretched. Who would concatenate a constant string in a loop over and over again? – Stefan Zobel Aug 15 '16 at 17:39
  • 1
    @StefanZobel It doesn't apply to only constants, it applys to any type of string. And if you wanted to prefix a list of URLs, for example with `.pdf` or something consistent, where `".pdf"` is the constant. Not a stretch at all. – Vince Aug 15 '16 at 19:04
  • 1
    @VinceEmigh I guess I see what you mean now. You are referring to something like `result = (new StringBuilder(String.valueOf(result))).append(s).append(".pdf").toString();` inside a loop body. That's correct of course. I believe we had a reciprocal misunderstanding here. – Stefan Zobel Aug 15 '16 at 19:27
  • 1
    The biggest problem with the statement is the lack of context. Does this apply only when `String` literals are used as values, or would the principle still apply when using `new String(...)`? Does it apply to `private static`? Is it a mutation problem, and if so is it related to concurrency? Would memory barriers make a difference to the principle? I could keep asking questions. Seeing how they don't mention performance-heavy autoboxing pitfalls, this could have been based on code prior to Java 5 that lacked enumeration, which may have had the largest code-base at the time. – Vince Aug 16 '16 at 00:46

4 Answers4

3

Arrays are mutable containers. Global mutable container without built-in synchronisation is multi-threaded horror waiting to happen. You can store reference to that array in some local context and then have it's content changed at any time without you knowing it. Or vice versa you may assume that this is convenient global state, but then every client must know the exact correct way to synchronise its access to that state. And somebody will forget and bug will be born.

Nikem
  • 5,716
  • 3
  • 32
  • 59
  • 6
    But why Strings in particular? – Diego Martinoia Aug 15 '16 at 17:29
  • 3
    Truth be told: no idea. IMO this should be relevant to arrays of any class. – Nikem Aug 15 '16 at 17:30
  • 3
    @DiegoMartinoia, I would guess Strings are called out in particular due to the statement two lines above, "_Oracle Retail products tend to be string-intensive_". Most likely, Oracle had some specific issues with developers using String arrays in the past. – jaco0646 Aug 15 '16 at 17:38
  • 3
    This doesn't sound right. As others have mentioned, this applies to *all* types. But what makes me lean further towards disagreement is that memory barriers could be used to prevent concurrency issues, as one should do with any type in this situation. – Vince Aug 15 '16 at 19:24
  • 2
    The problem with memory barriers in this case is that EVERY client of that arrays must know the right way to use them (on which monitor to synchronise) and remember to do it every single time. This is bad. – Nikem Aug 16 '16 at 05:39
  • 1
    @Nikem You could always hide the variable expose use of it through thread-safe methods. The article does mention "*avoid public attributes*" – Vince Aug 16 '16 at 13:29
  • This seems like the most reasonable answer. I agree this is not limited to Strings, but it is likely that the authors specifically called out Strings because the framework is *string-intensive* as @jaco0646 pointed out. – Zack Aug 16 '16 at 18:46
  • 2
    @ZackTeater I feel the most reasonable answer would be enumeration. It would have been just as easy to say "*avoid static arrays*" or "*avoid static arrays of objects*". If global state was an issue, why not mention singletons? If concurrency was an issue, why not mention anything about threads in the article? It's quite misleading and extremely cryptic. Seeing how developers used `String` for types long after enums were released (people *still* do it today), it makes a lot more sense than global state or concurrency. That's just my perspective though – Vince Aug 16 '16 at 19:22
2

I believe it's due to developers using String as a replacement (most likely unintentionally) for enumeration, and the static String[] was a way to group these types.

It could be targetting design flaws that were common at the time the article was written

For all we know, this article could be targetting the Java 4- code base.

Not saying this was written during Java 4, but that most production code existing at the time may have been written in Java 4. The lack of autoboxing pitfalls (which are quite common) and no mentions of generics (raw types) lead me to believe this.

Why would Oracle enforce such a cryptic principle?

I don't believe it has to do with global state, since I'm sure singletons would have been mentioned (being the pinnicle of global state).

I doubt concurrency is the issue, since the article does not mention anything else about sharing data across threads, or even using multiple threads. You would assume they would have mentioned SOMETHING about the thread environment.

Misusing String for type enumeration purposes was common back in the day, so I'm sure this statement (if related to what I think it is) was a lot easier to understand back then. I find it to be quite a stretch that this statement was just as cryptic back then, yet no one has cared to question it, resulting in the lack of google results when digging into this.

My Answer

Developers to this day still misuse String for type information. With the lack of enumeration, I could see how a developer may be tempted to do something along the lines of:

class Card {
    static String HEARTS = "HEARTS";
    static String DIAMONDS = "DIAMONDS";
    static String CLUBS = "CLUBS";
    static String SPADES = "SPADES";

    static String[] CARD_TYPE = {
        HEARTS, DIAMONDS, CLUBS, SPADES
    };

    private String type;
    //...
}

Using the array for easy looping (as one would do with Enum.values()).

Using String as enums is frowned upon:

  1. It's not type safe. If MAGE can be passed to a method, ANY String could be passed in it's place. This leads us to...

  2. Mistakes are easier to make. It's possible for a typo to go unnoticed, which may seem small for the average developer, but could be catastrophic for businesses with large code bases and many consumers (such as Oracle). For example: a hidden typo results in a comparison returning false. This prevents a service from launching. The lack of this service results in a bug. If not found before users notice the bug, it could lead to exploits.

This could be fix by using Java's built-in type system:

abstract class CardType {

}

class Hearts extends CardType {

}

....

Or

enum CardType { HEARTS, DIAMONDS, ...; }

Not only is this less error-prone, but it allows you to use polymorphism, which prevents the need to check the value in order to trigger a specific behavior. The behavior can be contained in the type itself.

Although I can't promise this is the correct answer, it seems to be the only answer that does not depend on the use of modifiers that weren't mentioned in the statement.

Yes, the example above does not use proper constants (lacks the final). But although constants should be preferred, they are not required to benefit from this design (being able to use String for type information) nor does the design require it to work . Since people may not have always used constants for this, they could have left out the final or "array of constants" for this purpose.

Vince
  • 14,470
  • 7
  • 39
  • 84
  • 1
    You make an excellent point on enumeration and I think you're right about this being based on Java 4. At least one of the Oracle POS frameworks (Tour) does seem to use Strings to hold state (among other things). – Zack Aug 16 '16 at 19:56
1

This way undermines the object oriented paradigm. Moreover it's a security hole, because the final keyword assures only reference value and leaves the content to be changed easily.

Nikolas Charalambidis
  • 40,893
  • 16
  • 117
  • 183
1

Some of them are arbitrary. For example, having a single return statement is a matter of style (maybe related to some analysis tool that in the past couldn't handle multiple return paths?).

Some of them are outdated. Like the one about StringBuilding.

I'd assume that these are a mixture of old performance recommendations and style guides. I'd reckon the String static array one falls in the latter category, with maybe a hint towards what others have said about the possible link with the introduction of enums (maybe that's why Strings in particular).

Some of them are actually completely unfeasible when you try to apply them to modern frameworks: "Do not use a switch to make a call based on the object type.", for example, is at the core of Akka Java (onReceive) method and in general in any type of pattern matching in Java...

While I admit it's curious, I'd say it's probably something related to some specific tool they used in the past or to the enum possibility mentioned. But it's just my opinion, not a definitive answer.

Diego Martinoia
  • 4,592
  • 1
  • 17
  • 36
  • 1
    Doesn't switching on object type negate polymorphism? – Zack Aug 16 '16 at 12:28
  • 1
    @ZachTeater Not really. Say you have interface A implemented by B and C (strategy pattern-ish). You have a collection of B and C types (maybe your requests to be handled), so you do List . Then you can process them by (for a:listA -> case B -> processB(a) caseC -> processC(a)) or something similar. It's convenient, and it uses polymorphism. – Diego Martinoia Aug 16 '16 at 16:48
  • 1
    @ZackTeater another example is Akka as mentioned. Your receive method (in Java) will be all if (msg instanceof X).. elseif (msg instanceof Y)... – Diego Martinoia Aug 16 '16 at 16:48
  • 1
    wouldn't polymorphism be for A to have a method called process(), which is implemented differently by B and C. Then you could do ... for a:listA --> process(a). – Zack Aug 16 '16 at 18:06
  • 1
    Also, isn't using instanceOf the anti-pattern for polymorphism? – Zack Aug 16 '16 at 18:08
  • 1
    Agreed on the first one (my bad for picking a bad example). Re. the second one... it's the only way I can think of to do pattern matching in Java (or something similar to it). While you don't NEED pm, it's nice to have – Diego Martinoia Aug 17 '16 at 08:48
  • 1
    @DiegoMartinoia The visitor pattern allows multi-dispatch to elements of different types without `instanceof`. Check out [this answer](http://stackoverflow.com/questions/29458676/how-to-avoid-instanceof-when-implementing-factory-design-pattern/29459571#29459571) I wrote about implementing the pattern. – Vince Aug 17 '16 at 15:09
  • @VinceEmigh amazing answer, I must say. It's definitely the way to go if you have complex logic for each different type, as it splits such logic form the container, but I'm still partial to the fact that, for simple handling, instanceof will do just fine: the overhead is justified only for complex scenarios. – Diego Martinoia Aug 17 '16 at 15:35
  • @DiegoMartinoia Agreed. Design principles, such as the ones in Oracle's article, are to help solve problems. If there is no problem, chances are you're doing nothing more than increasing your code's complexity, which can be a problem in itself. People shouldn't stop using `static String[]` because they're told to. They should find *why* it shouldn't be used (the benefit), then only apply it if they feel the benefit is worth it. – Vince Aug 17 '16 at 16:22
  • The `StringBuilder` principle is not outdated, as mentioned in the comments of the original post. "*Do not use switch to make a call based on object type*" - Akka only does this due to objects being expesnive in Java and the lack of value types. It's still a design flaw though, goes against OOP philosophy. Having a single return statement is based on [Single-Entry, Single-Exit](https://en.m.wikipedia.org/wiki/Single-entry_single-exit), which is logical. Keep in mind, the revision of this article was released in 2015. – Vince Aug 17 '16 at 21:17