On 4 October 2013 21:09, Romain Manni-Bucau <rmannibu...@gmail.com> wrote: > Hi > > It should be done in the doc iirc. Having a constant would just mean > loosing a line with no benefit excepted needing to browse code instead of > reading it inline in this case cause the prop will not be used in the code > at all so id stay it like it...moreover thats really a detail for the > product imo
The advantage of using a field is that it can have Javadoc. Also there are code validation tools that can detect 'magic' constants when used inline; they expect constants to be defined as such. Most IDEs these days will show the value of the constant when hovering over it. What's important is that people reading the code see the explanation right by where the string is used. Back to the case in point: why is the string ".activate" and not anything else? > Le 4 oct. 2013 20:29, "sebb" <seb...@gmail.com> a écrit : > >> On 4 October 2013 18:04, Jean-Louis MONTEIRO <jeano...@gmail.com> wrote: >> > Apologize for the late answer. >> > Not sure to understand the purpose of the request. >> > Could you detail cause it's not used anywhere else? >> >> The code should not contain 'magic' strings (or numbers for that matter). >> >> All fixed strings should be documented as to their purpose and derivation. >> The conventional way to do this is via a constant with appropriate Javadoc. >> >> In other words, why is the suffix ".activated" ? >> Why not ".alive" or ".on" or ".randomString"? >> >> > JLouis >> > >> > >> > 2013/10/3 sebb <seb...@gmail.com> >> > >> >> On 2 October 2013 21:12, <jlmonte...@apache.org> wrote: >> >> > Author: jlmonteiro >> >> > Date: Wed Oct 2 20:12:29 2013 >> >> > New Revision: 1528612 >> >> > >> >> > URL: http://svn.apache.org/r1528612 >> >> > Log: >> >> > Fixing configuration property typo >> >> > >> >> > Modified: >> >> > >> >> >> commons/sandbox/monitoring/trunk/reporting/src/main/java/org/apache/commons/monitoring/reporting/web/plugin/PluginRepository.java >> >> > >> >> > Modified: >> >> >> commons/sandbox/monitoring/trunk/reporting/src/main/java/org/apache/commons/monitoring/reporting/web/plugin/PluginRepository.java >> >> > URL: >> >> >> http://svn.apache.org/viewvc/commons/sandbox/monitoring/trunk/reporting/src/main/java/org/apache/commons/monitoring/reporting/web/plugin/PluginRepository.java?rev=1528612&r1=1528611&r2=1528612&view=diff >> >> > >> >> >> ============================================================================== >> >> > --- >> >> >> commons/sandbox/monitoring/trunk/reporting/src/main/java/org/apache/commons/monitoring/reporting/web/plugin/PluginRepository.java >> >> (original) >> >> > +++ >> >> >> commons/sandbox/monitoring/trunk/reporting/src/main/java/org/apache/commons/monitoring/reporting/web/plugin/PluginRepository.java >> >> Wed Oct 2 20:12:29 2013 >> >> > @@ -35,7 +35,7 @@ public final class PluginRepository { >> >> > if (name == null) { >> >> > throw new IllegalArgumentException("plugin name can't >> >> be null"); >> >> > } >> >> > - if (!Configuration.is(name + "activated", true)) { >> >> > + if (!Configuration.is(name + ".activated", true)) { >> >> >> >> I assume that this string is used elsewhere within monitoring? >> >> If so, it should be defined once as a String constant (with Javadoc) >> >> and used throughout. >> >> Or there could be a method to convert a name by appending the suffix. >> >> >> >> > continue; >> >> > } >> >> > >> >> > >> >> > >> >> >> >> --------------------------------------------------------------------- >> >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> >> For additional commands, e-mail: dev-h...@commons.apache.org >> >> >> >> >> > >> > >> > -- >> > Jean-Louis >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> >> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
