commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Romain Manni-Bucau <rmannibu...@gmail.com>
Subject Re: svn commit: r1528612 - /commons/sandbox/monitoring/trunk/reporting/src/main/java/org/apache/commons/monitoring/reporting/web/plugin/PluginRepository.java
Date Fri, 04 Oct 2013 20:09:22 GMT
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
Le 4 oct. 2013 20:29, "sebb" <sebbaz@gmail.com> a écrit :

> On 4 October 2013 18:04, Jean-Louis MONTEIRO <jeanouii@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 <sebbaz@gmail.com>
> >
> >> On 2 October 2013 21:12,  <jlmonteiro@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-unsubscribe@commons.apache.org
> >> For additional commands, e-mail: dev-help@commons.apache.org
> >>
> >>
> >
> >
> > --
> > Jean-Louis
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message