Hi, I completely agree with almost all of your comments, and hope to improve for the 4.1.4 release, so updates are forthcoming.
As for 'class level members almost always declared in the top of class', it in fact had me thinking as well while authoring JavascriptManagerImpl. So, to lend some perspective, let me just write down my thought process - i'm interested in your comments (more valuable than just strict rules) -) I almost always write members at the very top, but then -) somehow the code of several tapestry components came to my mind... many declare the abstract getters/setters at the end - this forced another thinking, i.e. what to do in this case? -) then i realized that users pass Strings configurations to this class which get instantly converted to IAssets - the IAssets is what gets stored in the members -) so, it seemed more valuable to first present the setters (which accept Strings) than the members (which are IAssets) because the former are what the users are going to use -) unfortunately, I then forgot (!) all about this and moved the interface implementations at the top. So, the current ordering is interface implementation methods, setters, members. Anyway, that's it (btw, nice to write down one's thoughts!) However, I still value the setters more than the members for that particular class, so my instinct would be to bend the rules :D On Nov 14, 2007 10:38 PM, Jesse Kuhnert <[EMAIL PROTECTED]> wrote: > Awesome to see this stuff getting added in! > > On a slightly less positive side, I think we need to look more > closely at coding style and naming conventions of things in T4 in > order to maintain an easy to read / understand code base for the > system as a whole. I've been guilty of straying myself a lot, so > take this with a grain of salt... > > JavascriptManager / impl in particular had a couple little things that > we might want to change before release: > > -) Method names probably don't need to have "Js" embedded in all of > them - such as getJs<Foo> - as that should hopefully be implied by the > class name of the object already. > > -) Some of the method names are either confusing or the javadoc > doesn't have enough information. (again, I'm just as guilty ) For > instance, I have absolutely no idea what getMainJsWidgetAsset / > getJsTapestryAsset means just looking at the interface and javadocs > provided. > > -) class level members are almost always declared in the top of class > files in the system - not the bottom. > > -) JavascriptManagerImpl - Sometimes the curly braces (for > constructor / method declarations ) are on new lines and sometimes the > same line. The system has thus far been putting them on a new line. > (though some things may have escaped this) > > -) Need lots more new lines and whitespace in general. > org.apache.tapestry.pageload.PageLoader might be a good example. Or > anything else that has mostly been written by Howard. He writes very > visually clean / easy to read code. I usually switch between being > slightly more verbose / tight when writing javascript code (as it's > interpreted and whitespace / new lines do ultimately hurt you) and > trying to use lots more whitespace / new lines in java as there is > really no reason not to give everything plenty of space since it will > be compiled down to byte code that doesn't care about spacing. > > Again, please don't take it the wrong way as we both know I'm > obviously screwing up plenty here and there with the same stuff / > other things as well. Just want to be sure that we try and keep T4 > up to par even if it isn't the latest and greatest stuff in the > tapestry dev world. > > > On Nov 13, 2007 3:22 PM, <[EMAIL PROTECTED]> wrote: > > Author: andyhot > > Date: Tue Nov 13 12:22:54 2007 > > New Revision: 594630 > > > > URL: http://svn.apache.org/viewvc?rev=594630&view=rev > > Log: > > test for jsmanager > > > > Added: > > > > tapestry/tapestry4/trunk/tapestry-framework/src/test/org/apache/tapestry/javascript/ > > > > tapestry/tapestry4/trunk/tapestry-framework/src/test/org/apache/tapestry/javascript/TestJavascriptManagerImpl.java > > Modified: > > > > tapestry/tapestry4/trunk/tapestry-framework/src/java/org/apache/tapestry/javascript/JavascriptManagerImpl.java > > > > Modified: > > tapestry/tapestry4/trunk/tapestry-framework/src/java/org/apache/tapestry/javascript/JavascriptManagerImpl.java > > URL: > > http://svn.apache.org/viewvc/tapestry/tapestry4/trunk/tapestry-framework/src/java/org/apache/tapestry/javascript/JavascriptManagerImpl.java?rev=594630&r1=594629&r2=594630&view=diff > > ============================================================================== > > --- > > tapestry/tapestry4/trunk/tapestry-framework/src/java/org/apache/tapestry/javascript/JavascriptManagerImpl.java > > (original) > > +++ > > tapestry/tapestry4/trunk/tapestry-framework/src/java/org/apache/tapestry/javascript/JavascriptManagerImpl.java > > Tue Nov 13 12:22:54 2007 > > @@ -14,9 +14,10 @@ > > > > package org.apache.tapestry.javascript; > > > > -import java.util.List; > > import java.util.ArrayList; > > +import java.util.List; > > > > +import org.apache.hivemind.HiveMind; > > import org.apache.hivemind.Location; > > import org.apache.hivemind.util.URLResource; > > import org.apache.tapestry.IAsset; > > @@ -25,11 +26,20 @@ > > import org.apache.tapestry.util.DescribedLocation; > > > > /** > > + * An implementation that accepts a comma separated String for > > + * files, formFiles and widgetFiles. > > + * > > * @author Andreas Andreou > > * @since 4.1.4 > > */ > > public class JavascriptManagerImpl implements JavascriptManager { > > > > + public JavascriptManagerImpl() { > > + _files = new ArrayList(); > > + _formFiles = new ArrayList(); > > + _widgetFiles = new ArrayList(); > > + } > > + > > public IAsset getMainJsAsset() > > { > > return findFirst(_files); > > @@ -122,7 +132,7 @@ > > private IAsset findAsset(String path, String description) > > { > > IAsset asset = null; > > - if (path!=null) > > + if ( !HiveMind.isBlank(path) ) > > { > > Location location = new DescribedLocation(new > > URLResource(path), description); > > asset = _assetSource.findAsset(null, path, null, location); > > @@ -131,7 +141,7 @@ > > } > > > > private IAsset findFirst(List list) { > > - if (list ==null || list.isEmpty()) > > + if (list == null || list.isEmpty()) > > return null; > > else > > return (IAsset) list.get(0); > > > > Added: > > tapestry/tapestry4/trunk/tapestry-framework/src/test/org/apache/tapestry/javascript/TestJavascriptManagerImpl.java > > URL: > > http://svn.apache.org/viewvc/tapestry/tapestry4/trunk/tapestry-framework/src/test/org/apache/tapestry/javascript/TestJavascriptManagerImpl.java?rev=594630&view=auto > > ============================================================================== > > --- > > tapestry/tapestry4/trunk/tapestry-framework/src/test/org/apache/tapestry/javascript/TestJavascriptManagerImpl.java > > (added) > > +++ > > tapestry/tapestry4/trunk/tapestry-framework/src/test/org/apache/tapestry/javascript/TestJavascriptManagerImpl.java > > Tue Nov 13 12:22:54 2007 > > @@ -0,0 +1,94 @@ > > +package org.apache.tapestry.javascript; > > + > > +import java.util.Locale; > > + > > +import org.apache.tapestry.IAsset; > > +import org.apache.tapestry.TestBase; > > +import org.apache.tapestry.util.DescribedLocation; > > +import org.apache.tapestry.asset.AssetSource; > > +import org.apache.hivemind.Resource; > > +import org.testng.annotations.Test; > > + > > +import static org.easymock.EasyMock.*; > > + > > [EMAIL PROTECTED] > > +public class TestJavascriptManagerImpl extends TestBase > > +{ > > + public void test_null_manager() > > + { > > + JavascriptManagerImpl impl = createImpl(); > > + replay(); > > + > > + assertNullAndEmpty(impl); > > + > > + verify(); > > + } > > + > > + public void test_empty_manager() > > + { > > + JavascriptManagerImpl impl = createImpl("", "", "", "", "", ""); > > + replay(); > > + > > + assertNullAndEmpty(impl); > > + > > + verify(); > > + } > > + > > + public void test_several_files() > > + { > > + AssetSource source = newMock(AssetSource.class); > > + expectFile(source, "a.js"); > > + expectFile(source, "b.js"); > > + expectFile(source, "tap"); > > + > > + replay(); > > + > > + JavascriptManagerImpl impl = createImpl(source, "a.js, b.js", "", > > "", "", "tap", ""); > > + assertEquals(impl.getJsAssets().size(), 2); > > + assertNotNull(impl.getMainJsAsset()); > > + assertNotNull(impl.getJsTapestryAsset()); > > + > > + verify(); > > + } > > + > > + private void expectFile(AssetSource source, String file) { > > + expect(source.findAsset((Resource) isNull(), eq(file), > > + (Locale) isNull(), isA(DescribedLocation.class))) > > + .andReturn(newMock(IAsset.class)); > > + } > > + > > + private void assertNullAndEmpty(JavascriptManagerImpl impl) { > > + assertNull(impl.getJsPath()); > > + assertNull(impl.getJsTapestryAsset()); > > + assertNull(impl.getJsTapestryPath()); > > + assertNull(impl.getMainJsAsset()); > > + assertNull(impl.getMainJsFormAsset()); > > + assertNull(impl.getMainJsWidgetAsset()); > > + assertTrue(impl.getJsAssets().isEmpty()); > > + assertTrue(impl.getJsFormAssets().isEmpty()); > > + assertTrue(impl.getJsWidgetAssets().isEmpty()); > > + } > > + > > + private JavascriptManagerImpl createImpl(String...params) { > > + return createImpl(newMock(AssetSource.class), params); > > + } > > + > > + private JavascriptManagerImpl createImpl(AssetSource source, String... > > params) { > > + JavascriptManagerImpl impl = new JavascriptManagerImpl(); > > + impl.setAssetSource(source); > > + > > + if (params.length>0) > > + impl.setFiles(params[0]); > > + if (params.length>1) > > + impl.setFormFiles(params[1]); > > + if (params.length>2) > > + impl.setWidgetFiles(params[2]); > > + if (params.length>3) > > + impl.setPath(params[3]); > > + if (params.length>4) > > + impl.setTapestryFile(params[4]); > > + if (params.length>5) > > + impl.setTapestryPath(params[5]); > > + return impl; > > + } > > +} > > > > > > > > > > -- > Jesse Kuhnert > Tapestry/Dojo team member/developer > > Open source based consulting work centered around > dojo/tapestry/tacos/hivemind. http://blog.opencomponentry.com > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [EMAIL PROTECTED] > For additional commands, e-mail: [EMAIL PROTECTED] > > -- Andreas Andreou - [EMAIL PROTECTED] - http://blog.andyhot.gr Tapestry / Tacos developer Open Source / JEE Consulting --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
