You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ant.apache.org by Kevin Jackson <ke...@it.fts-vn.com> on 2004/12/02 10:47:15 UTC
[Patch] style of UpToDate
Just removed the C++ style private variables
Why set dir to null here? Is there a problem with the size of the
object and the GC not collecting it in time? if not it doesn't seem to
be relevant
protected boolean scanDir(File srcDir, String[] files) {
SourceFileScanner sfs = new SourceFileScanner(this);
FileNameMapper mapper = null;
File dir = srcDir;
if (mapperElement == null) {
MergingMapper mm = new MergingMapper();
mm.setTo(this.targetFile.getAbsolutePath());
mapper = mm;
dir = null;
} else {
mapper = mapperElement.getImplementation();
}
return sfs.restrict(files, srcDir, dir, mapper).length == 0;
}
Kev
Re: final keyword WAS [Patch] style of UpToDate
Posted by Matt Benson <gu...@yahoo.com>.
--- Phil Weighill-Smith
<ph...@volantis.com> wrote:
> I personally think the use of "final" for method
> parameters is "a good
> thing" - it reminds me of that great C++ feature
> "const" (which was
> actually far more sophisticated and useful, and
> strangely is a Java
> reserved keyword).
I read the sample chapter Kevin pointed out, and I
understood the points made. But if you look at the
issue from a different perspective, the basic
take-away message is that "final" should be used as a
crutch. The obvious response to that statement is
then "what's wrong with that?" Nothing, I suppose,
but there is unit testing to help avoid the kinds of
mistakes warned against in the chapter. I suppose
it's all a matter of taste. The
compiler-optimization-based conditional compilation
was interesting, but personally I would rather be able
to throw switches without touching java code.
$0.02
-Matt
>
> Phil :n.
__________________________________
Do you Yahoo!?
Meet the all-new My Yahoo! - Try it today!
http://my.yahoo.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: [Patch] style of UpToDate
Posted by Phil Weighill-Smith <ph...@volantis.com>.
I personally think the use of "final" for method parameters is "a good
thing" - it reminds me of that great C++ feature "const" (which was
actually far more sophisticated and useful, and strangely is a Java
reserved keyword).
Just because people haven't previously adopted a practice doesn't mean
that it can't become best practice; this is a definite candidate for the
latter.
Phil :n.
On Thu, 2004-12-02 at 15:54, Peter Reilly wrote:
> Kevin Jackson wrote:
>
> > Peter Reilly wrote:
> >
> >> Kevin Jackson wrote:
> >>
> >>> Just removed the C++ style private variables
> >>
> >>
> >>
> >> cool, but...
> >> 1) why place a "final" in the arguments
> >> 2) you should only use the "this." in the setter
> >
> >
> > 1) I was reading hardcore java - www.oreilly.com/catalog/*hardcore*jv/
> >
> > The arguments made in favour of declaring variables to methods as
> > final far outweighed (in my mind), the hassle of typing a few extra
> > characters. The chapter on final being used in this (and a variety of
> > other manners) is available as a pdf and I thoroughly recommend
> > reading it, just to see what you think, but I couldn't disagree with
> > his reasoning.
>
> Mmmm.....
> declaring method parameters as final may reduce a number of silly bugs,
> however I cannot think of any
> bugs in the ant code that has been caused by setting method parameters.
> It is not normal java style to use
> final method parameters and it
> is not normal ant coding style (although some of Magesh's code does do
> this).**
>
> >
> > 2) yeah, sorry, I got carried away I think - in other methods there
> > should be no need to use this - doh!
> >
> > The main reason I did this was that I've been trying to contribute for
> > the last few days and I haven't found anything that I felt I had the
> > time to start on and this looked like a quick contribution,
>
> An ongoing project is making the ant source code pass a checkstyle
> audit. There is check.xml at the top level in the ant source code.
> The top offending files are the moment are:
>
> taskdefs/Rmic.java
> taskdefs/Zip.java
> taskdefs/optional/vss/MSVSS.java
> taskdefs/optional/javacc/JavaCC.java
> taskdefs/optional/NetRexxC.java
> taskdefs/optional/junit/JUnitTestRunner.java
> types/Path.java
> taskdefs/repository/Library.java
> taskdefs/optional/jsp/JspC.java
>
> Peter
> <ci...@apache.org>
>
> >
> > Kev
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
> > For additional commands, e-mail: dev-help@ant.apache.org
> >
> >
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
> For additional commands, e-mail: dev-help@ant.apache.org
--
Phil Weighill-Smith <ph...@volantis.com>
Volantis Systems
Re: [Patch] style of UpToDate
Posted by Peter Reilly <pe...@apache.org>.
Kevin Jackson wrote:
> Peter Reilly wrote:
>
>> Kevin Jackson wrote:
>>
>>> Just removed the C++ style private variables
>>
>>
>>
>> cool, but...
>> 1) why place a "final" in the arguments
>> 2) you should only use the "this." in the setter
>
>
> 1) I was reading hardcore java - www.oreilly.com/catalog/*hardcore*jv/
>
> The arguments made in favour of declaring variables to methods as
> final far outweighed (in my mind), the hassle of typing a few extra
> characters. The chapter on final being used in this (and a variety of
> other manners) is available as a pdf and I thoroughly recommend
> reading it, just to see what you think, but I couldn't disagree with
> his reasoning.
Mmmm.....
declaring method parameters as final may reduce a number of silly bugs,
however I cannot think of any
bugs in the ant code that has been caused by setting method parameters.
It is not normal java style to use
final method parameters and it
is not normal ant coding style (although some of Magesh's code does do
this).**
>
> 2) yeah, sorry, I got carried away I think - in other methods there
> should be no need to use this - doh!
>
> The main reason I did this was that I've been trying to contribute for
> the last few days and I haven't found anything that I felt I had the
> time to start on and this looked like a quick contribution,
An ongoing project is making the ant source code pass a checkstyle
audit. There is check.xml at the top level in the ant source code.
The top offending files are the moment are:
taskdefs/Rmic.java
taskdefs/Zip.java
taskdefs/optional/vss/MSVSS.java
taskdefs/optional/javacc/JavaCC.java
taskdefs/optional/NetRexxC.java
taskdefs/optional/junit/JUnitTestRunner.java
types/Path.java
taskdefs/repository/Library.java
taskdefs/optional/jsp/JspC.java
Peter
<ci...@apache.org>
>
> Kev
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
> For additional commands, e-mail: dev-help@ant.apache.org
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: [Patch] style of UpToDate
Posted by Kevin Jackson <ke...@it.fts-vn.com>.
Peter Reilly wrote:
> Kevin Jackson wrote:
>
>> Just removed the C++ style private variables
>
>
> cool, but...
> 1) why place a "final" in the arguments
> 2) you should only use the "this." in the setter
1) I was reading hardcore java - www.oreilly.com/catalog/*hardcore*jv/
The arguments made in favour of declaring variables to methods as final
far outweighed (in my mind), the hassle of typing a few extra
characters. The chapter on final being used in this (and a variety of
other manners) is available as a pdf and I thoroughly recommend reading
it, just to see what you think, but I couldn't disagree with his reasoning.
2) yeah, sorry, I got carried away I think - in other methods there
should be no need to use this - doh!
The main reason I did this was that I've been trying to contribute for
the last few days and I haven't found anything that I felt I had the
time to start on and this looked like a quick contribution,
Kev
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: [Patch] style of UpToDate
Posted by Peter Reilly <pe...@apache.org>.
Kevin Jackson wrote:
> Just removed the C++ style private variables
cool, but...
1) why place a "final" in the arguments
2) you should only use the "this." in the setter
>
> Why set dir to null here? Is there a problem with the size of the
> object and the GC not collecting it in time? if not it doesn't seem
> to be relevant
I do not know the code, but the "dir" is use in the sfs.restrict() call,
so this may be the reason that it is set to null.
>
> protected boolean scanDir(File srcDir, String[] files) {
> SourceFileScanner sfs = new SourceFileScanner(this);
> FileNameMapper mapper = null;
> File dir = srcDir;
> if (mapperElement == null) {
> MergingMapper mm = new MergingMapper();
> mm.setTo(this.targetFile.getAbsolutePath());
> mapper = mm;
> dir = null;
> } else {
> mapper = mapperElement.getImplementation();
> }
> return sfs.restrict(files, srcDir, dir, mapper).length == 0;
> }
>
> Kev
>
>------------------------------------------------------------------------
>
>Index: UpToDate.java
>===================================================================
>RCS file: /home/cvspublic/ant/src/main/org/apache/tools/ant/taskdefs/UpToDate.java,v
>retrieving revision 1.36
>diff -u -r1.36 UpToDate.java
>--- UpToDate.java 9 Mar 2004 16:48:06 -0000 1.36
>+++ UpToDate.java 2 Dec 2004 09:44:05 -0000
>@@ -42,10 +42,10 @@
>
> public class UpToDate extends Task implements Condition {
>
>- private String _property;
>- private String _value;
>- private File _sourceFile;
>- private File _targetFile;
>+ private String property;
>+ private String value;
>+ private File sourceFile;
>+ private File targetFile;
> private Vector sourceFileSets = new Vector();
>
> protected Mapper mapperElement = null;
>@@ -56,8 +56,8 @@
> *
> * @param property the name of the property to set if Target is up-to-date.
> */
>- public void setProperty(String property) {
>- _property = property;
>+ public void setProperty(final String property) {
>+ this.property = property;
> }
>
> /**
>@@ -66,15 +66,15 @@
> *
> * @param value the value to set the property to if Target is up-to-date
> */
>- public void setValue(String value) {
>- _value = value;
>+ public void setValue(final String value) {
>+ this.value = value;
> }
>
> /**
> * Returns the value, or "true" if a specific value wasn't provided.
> */
> private String getValue() {
>- return (_value != null) ? _value : "true";
>+ return (this.value != null) ? this.value : "true";
> }
>
> /**
>@@ -83,8 +83,8 @@
> *
> * @param file the file we are checking against.
> */
>- public void setTargetFile(File file) {
>- _targetFile = file;
>+ public void setTargetFile(final File file) {
>+ this.targetFile = file;
> }
>
> /**
>@@ -93,14 +93,14 @@
> *
> * @param file the file we are checking against the target file.
> */
>- public void setSrcfile(File file) {
>- _sourceFile = file;
>+ public void setSrcfile(final File file) {
>+ this.sourceFile = file;
> }
>
> /**
> * Nested <srcfiles> element.
> */
>- public void addSrcfiles(FileSet fs) {
>+ public void addSrcfiles(final FileSet fs) {
> sourceFileSets.addElement(fs);
> }
>
>@@ -121,32 +121,32 @@
> * see if the target(s) is/are up-to-date.
> */
> public boolean eval() {
>- if (sourceFileSets.size() == 0 && _sourceFile == null) {
>+ if (sourceFileSets.size() == 0 && this.sourceFile == null) {
> throw new BuildException("At least one srcfile or a nested "
> + "<srcfiles> element must be set.");
> }
>
>- if (sourceFileSets.size() > 0 && _sourceFile != null) {
>+ if (sourceFileSets.size() > 0 && this.sourceFile != null) {
> throw new BuildException("Cannot specify both the srcfile "
> + "attribute and a nested <srcfiles> "
> + "element.");
> }
>
>- if (_targetFile == null && mapperElement == null) {
>+ if (this.targetFile == null && mapperElement == null) {
> throw new BuildException("The targetfile attribute or a nested "
> + "mapper element must be set.");
> }
>
> // if the target file is not there, then it can't be up-to-date
>- if (_targetFile != null && !_targetFile.exists()) {
>- log("The targetfile \"" + _targetFile.getAbsolutePath()
>+ if (this.targetFile != null && !this.targetFile.exists()) {
>+ log("The targetfile \"" + this.targetFile.getAbsolutePath()
> + "\" does not exist.", Project.MSG_VERBOSE);
> return false;
> }
>
> // if the source file isn't there, throw an exception
>- if (_sourceFile != null && !_sourceFile.exists()) {
>- throw new BuildException(_sourceFile.getAbsolutePath()
>+ if (this.sourceFile != null && !this.sourceFile.exists()) {
>+ throw new BuildException(this.sourceFile.getAbsolutePath()
> + " not found.");
> }
>
>@@ -159,14 +159,14 @@
> ds.getIncludedFiles());
> }
>
>- if (_sourceFile != null) {
>+ if (this.sourceFile != null) {
> if (mapperElement == null) {
> upToDate = upToDate
>- && (_targetFile.lastModified() >= _sourceFile.lastModified());
>+ && (this.targetFile.lastModified() >= this.sourceFile.lastModified());
> } else {
> SourceFileScanner sfs = new SourceFileScanner(this);
> upToDate = upToDate
>- && (sfs.restrict(new String[] {_sourceFile.getAbsolutePath()},
>+ && (sfs.restrict(new String[] {this.sourceFile.getAbsolutePath()},
> null, null,
> mapperElement.getImplementation()).length == 0);
> }
>@@ -180,15 +180,15 @@
> * than (each of) the corresponding source file(s).
> */
> public void execute() throws BuildException {
>- if (_property == null) {
>+ if (this.property == null) {
> throw new BuildException("property attribute is required.",
> getLocation());
> }
> boolean upToDate = eval();
> if (upToDate) {
>- this.getProject().setNewProperty(_property, getValue());
>+ this.getProject().setNewProperty(this.property, getValue());
> if (mapperElement == null) {
>- log("File \"" + _targetFile.getAbsolutePath()
>+ log("File \"" + this.targetFile.getAbsolutePath()
> + "\" is up-to-date.", Project.MSG_VERBOSE);
> } else {
> log("All target files are up-to-date.",
>@@ -203,7 +203,7 @@
> File dir = srcDir;
> if (mapperElement == null) {
> MergingMapper mm = new MergingMapper();
>- mm.setTo(_targetFile.getAbsolutePath());
>+ mm.setTo(this.targetFile.getAbsolutePath());
> mapper = mm;
> dir = null;
> } else {
>
>
>
>------------------------------------------------------------------------
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
>For additional commands, e-mail: dev-help@ant.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org