Constants class
I have a Constants class that defines some constant values that are used in various places in my web application. Some of these constant values are set by a startup servlet so cannot be declared final. For example:
publicclass Constants{
publicstaticint SOME_VALUE;
publicstatic String[] SOME_OTHER_VALUES;
// ...
}
Now if I run findbugs against this class it warns me that "A mutable static field could be changed by malicious code or by accident from another package" and also for the String[] "A final static field references an array and can be accessed by malicious code or by accident from another package. This code can freely modify the contents of the array."
If I could make them final I could have getSomeValue()/getSomeOtherValue() methods but since I need to set them at startup I cannot do this. So I am not sure what to do really? I could have a protected set method and move this class into the same package as the startup servlet then make the variables private and implement the get methods? But I am not convinced it is worth the effort. Any other ideas?
[1444 byte] By [
jakain2a] at [2007-11-27 5:48:23]
# 1
> public class Constants {
>public static int SOME_VALUE;
> public static String[] SOME_OTHER_VALUES;
>// ...
>
> [/code]
public class Constants {
//--Values set at startup
private int SOME_VALUE;
public int getSOME_VALUE() { return this.SOME_VALUE;}
public void setSOME_VALUE(int x) {this.SOME_VALUE = x;
private String [] SOME_OTHER_VALUES;
public String [] getSOME_OTHER_VALUES() { return this.SOME_OTHER_VALUES;}
public void setSOME_OTHER_VALUES(String [] x) {this.SOME_OTHER_VALUES = x;}
}
# 2
So you're saying that your primary motivation is to keep findbugs happy?
How about
public class Constants {
public static final in SOME_VALUE = ConstantsHelper.getSomeValue();
....
}
Where ConstantsHelper is what looks up, generates or otherwise obtains the values
# 3
GhostRadioTwo - that does not help me though because any other code can set SOME_VALUE and also SOME_OTHER_VALUES would need to be cloned to prevent external modification.
# 4
> So you're saying that your primary motivation is to
> keep findbugs happy?
No but it is there to show any mistakes in my code so I am trying to think of a better solution.
In your example I think you are just moving the problem into ConstantsHelper, because other code can set the value here?
# 5
How will other code set these values? These values are to be set by a startup servlet, like you mentioned. Paranoia about alien coders that can read minds is not helpful when designing software, in my opinion.
# 6
He's not just moving the problem. The code that sets your variable is only called once.Also, for the array values, strongly consider the use of Enumerated Types.
# 7
> He's not just moving the problem. The code that sets
> your variable is only called once.
Well this is true for my code now but the point is that it is not enforced. How do you enforce this? Or maybe I should not worry about these 'alien coders'?
> Also, for the array values, strongly consider the use
> of Enumerated Types.
I will look into this option thanks.
# 8
To illustrate my point, this website is a Java application. How could any coder execute a program that sets the values of any variable or even call a method of this application?How would they be able to install code on the web server and then get it to execute?
# 9
> > He's not just moving the problem. The code that
> sets
> > your variable is only called once.
>
> Well this is true for my code now but the point is
> that it is not enforced. How do you enforce this? Or
> maybe I should not worry about these 'alien coders'?
It's a final variable. Final variables cannot be changed once they
are assigned. Clients of your class cannot modify the value,
because it's fixed by the time they get access to it.
If you're worried about other developers going into your class and
mucking with it, that's what code reviews are for.
# 10
> > So you're saying that your primary motivation is
> to
> > keep findbugs happy?
>
> No but it is there to show any mistakes in my code so
> I am trying to think of a better solution.
Findbugs is not - nor does it claim to be - completely accurate. It can only recognize patterns of code that might be a problem. It's up to you to evaluate whether it's right or not
> In your example I think you are just moving the
> problem into ConstantsHelper, because other code can
> set the value here?
No. Your constants will actually be constants
# 11
> It's a final variable. Final variables cannot be
> changed once they
> are assigned. Clients of your class cannot modify
> the value,
> because it's fixed by the time they get access to
> it.
>
They can be modified either via JNI or via reflection. There are some limits on that although those probably shouldn't matter in a case where they are being initialized at startup with configuration values.
System.in is an example of this. It is final and yet it can be modified via setIn().
# 12
> Any other ideas?
Don't fix things that aren't broken.
Evaluate whether this is even a problem that is relevant to the problem domain that your app runs in.
If the problem domain suggests that this might be a problem then find another solution. Alternatively recognize that this is a spurious error and set up your build to specifically ignore that error report for that file.
# 13
> It's a final variable. Final variables cannot be
> changed once they
> are assigned. Clients of your class cannot modify
> the value,
> because it's fixed by the time they get access to
> it.
No I can't make them final because they are assigned at startup by a servlet. The servlet retrieves these values from the web.xml file.
# 14
> No. Your constants will actually be constantsHow? I don't see how you have solved the problem because you don't show how you enforce that the constant values are only set once. Having a set method means any other code can reset the variable.
# 15
A Constant is typically considered to have a single (Constant) value. A user key for instance might be 'user'.
public static final String USER_KEY = 'user';
The value is hardcoded in a Constants class because it is know only have a single value, the same value, at all times, forever, and ever.
A configurable value that is derived from a web.xml instance is not a known, single, value that will always be the same forever and ever and ever. If it is, then you should hard code it in the Contants class like the user key above.
If it is not a constant, then it shouldn't be associated with a Constants class. It should just be a parameter (attribute) stored in either ApplicationContext, ServletContext, or SessionContext.
# 16
> > No. Your constants will actually be constants
>
> How? I don't see how you have solved the problem
> because you don't show how you enforce that the
> constant values are only set once. Having a set
> method means any other code can reset the variable.
Indeed you're correct. A 'set' method would mean that any other code can reset the variable. But I haven't posted anything here with a set method
# 17
how's about (uncompiled/tested but you get the idea)
public final class DelayedConstant<T> {
private T value;
public void init(T value) {
if (value == null) { //assuming null not valid; use boolean flag otherwise
this.value = value;
} else {
throw new IllegalStateException("already initialized" + /*...*/);
}
}
public T get() {
return value;
}
}
# 18
OnBringer - Thanks, I like that approach. I think I will move all "constants" that get set at startup into a class of that design.
# 19
> OnBringer - Thanks, I like that approach. I think I
> will move all "constants" that get set at startup
> into a class of that design.
Although it would be trivial to break that example. You will need an instance of the class from which to obtain the "constants", and there's nothing to stop multiple instances being created, each with different values of the "constants". What you could do is create a sole instance at startup of the servlet, and put it in the application context, making sure you only ever access that and never create another instance of the class.
# 20
> Although it would be trivial to break that example.
> You will need an instance of the class from
> which to obtain the "constants", and there's nothing
> to stop multiple instances being created, each with
> different values of the "constants". What you could
> do is create a sole instance at startup of the
> servlet, and put it in the application context,
> making sure you only ever access that and never
> create another instance of the class.
I give the class a private constructor and also made the init method synchronized. In the init I check a boolean flag called initialized to see if we have already initialized the class. If it has already been initialized then I throw IllegalStateException. In all the get methods I also check this flag and if it has not been initialized then I throw IllegalStateException. So I am using the singleton pattern here.
# 21
> > Although it would be trivial to break that
> example.
> > You will need an instance of the class from
> > which to obtain the "constants", and there's
> nothing
> > to stop multiple instances being created, each
> with
> > different values of the "constants". What you
> could
> > do is create a sole instance at startup of the
> > servlet, and put it in the application context,
> > making sure you only ever access that and never
> > create another instance of the class.
>
> I give the class a private constructor and also made
> the init method synchronized. In the init I check a
> boolean flag called initialized to see if we have
> already initialized the class. If it has already been
> initialized then I throw IllegalStateException. In
> all the get methods I also check this flag and if it
> has not been initialized then I throw
> IllegalStateException. So I am using the singleton
> pattern here.
A much simpler way to keep a singleton thread-safe, that actually works
public class MySingleton {
// Java loads classes lazily, this won't be instantiated until you ask for it
public static final MySingleton INSTANCE = new MySingleton();
private MySingleton() {}
// your methods here
}
# 22
My code works, why would it not? My get methods are static though since I cannot see any reason why this class would need to be subclassed.
# 23
> My code works, why would it not? My get methods are
> static though since I cannot see any reason why this
> class would need to be subclassed.
Without seeing it, no idea. But most implementations of the Singleton pattern in Java are inherently not thread-safe, despite first appearances. Overly-complex, too, with all that unnecessary check--if-instance-is-null, and synchronizing the unneccesary accessor method and what-not
Not sure what you mean by keeping the methods static because of subclassing, though. What do you mean?
