On 27 July 2013 17:14, Dishara Wijewardana <[email protected]> wrote:
> On Sat, Jul 27, 2013 at 11:37 AM, Ian Boston <[email protected]> wrote: > > > Hi Dishara, > > You asked me offlist if I could to a code review. I think its better if I > > do it on list so others in the community can tell me if I am being too > > harsh or lenient. One of the benefits of being in a community; support > for > > both of us ;) > > > > Here is the review: > > > > Hi Ian, > Thank you for going through all the code and providing the review. I have > commented on couple of them. All the other stuff, I have refactored and > commited including, refactored all tests classes to use junit 4 and all > classes to use slf4j. > Hi Dishara, Excellent, well done, in hindsight I might have gone a bit over the top with the review, but you took it in your stride! Best Regards Ian > > > > Best Regards > > Ian > > > > > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > > cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** > > CassandraConstants.java< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraConstants.java > > > > > File > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraConstants.java > > (right): > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > > cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** > > CassandraConstants.java#**newcode23< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraConstants.java#newcode23 > > > > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraConstants.java:23: > > public class CassandraConstants { > > Needs indentation and Javadoc please. > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > > cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** > > CassandraIterable.java< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraIterable.java > > > > > File > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraIterable.java > > (right): > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > > cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** > > CassandraIterable.java#**newcode1< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraIterable.java#newcode1 > > > > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraIterable.java:1: > > package org.apache.sling.cassandra.**resource.provider; > > ASF License header needed here please. > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > > cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** > > CassandraIterable.java#**newcode7< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraIterable.java#newcode7 > > > > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraIterable.java:7: > > public class CassandraIterable implements Iterable { > > Some javadoc, even if its just going to say wraps an iterator. > > I wonder if something like IterableIterator wouldn't be a better name as > > there is nothing specific to cassandra about this class. ? > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > > cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** > > CassandraResource.java< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResource.java > > > > > File > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraResource.java > > (right): > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > > cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** > > CassandraResource.java#**newcode68< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResource.java#newcode68 > > > > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraResource.java:68: > > } catch (Exception e) { > > In general, try not to catch (Exception) as although its quick and easy > > it will also tend to hide what its going on. Also its helpful where you > > do catch anything and not rethrow it, to log the stack trace at at least > > debug level. eg log.debug(e.getMessage(),e); > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > > cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** > > CassandraResource.java#**newcode81< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResource.java#newcode81 > > > > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraResource.java:81: > > }else if(column.getName().equals("**resourceType")) { > > Looks like there are some formatting issues here. I'll share with you my > > formatting setup that should put all of these right. It wont correct > > trailing spaces in the code, but you can do that with a regex find > > replace " $" as the regex "" as the replacement. > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > > cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** > > CassandraResourceProvider.java< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceProvider.java > > > > > File > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraResourceProvider.java > > (right): > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > > cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** > > CassandraResourceProvider.**java#newcode80< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceProvider.java#newcode80 > > > > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraResourceProvider.**java:80: > > } catch (Exception ignore){ > > even if you ignore an exception, log it at debug level. It might be the > > one thing that helps a deployer work out what went wrong. If the > > exception is expected, then say so in the log message. > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > > cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** > > CassandraResourceProvider.**java#newcode98< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceProvider.java#newcode98 > > > > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraResourceProvider.**java:98: > > log.error("Error at Provider "+e.getMessage()); > > log the stack trace at info or debug, however if this is a rare error > > then think about logging a stack trace at error. > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > > cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** > > CassandraResourceProvider.**java#newcode104< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceProvider.java#newcode104 > > > > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraResourceProvider.**java:104: > > return resource.listChildren(); > > This might generate a cyclic recursion depending on how > > resource.listChildren is implemented. I think with many resources it > > goes back to the resource resolver which arrives here. Please verify if > > I am correct or not. > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > > cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** > > CassandraResourceResolver.java< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceResolver.java > > > > > File > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraResourceResolver.java > > (right): > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > > cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** > > CassandraResourceResolver.**java#newcode31< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceResolver.java#newcode31 > > > > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraResourceResolver.**java:31: > > public class CassandraResourceResolver implements ResourceResolver { > > This class can be removed, you dont need to implement a > > ResourceResolver. > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > cassandra/src/main/java/org/**apache/sling/cassandra/** > > resource/provider/mapper/**CassandraMapper.java< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/mapper/CassandraMapper.java > > > > > File > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/mapper/**CassandraMapper.java > > (right): > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > cassandra/src/main/java/org/**apache/sling/cassandra/** > > resource/provider/mapper/**CassandraMapper.java#newcode22< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/mapper/CassandraMapper.java#newcode22 > > > > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/mapper/**CassandraMapper.java:22: > > public interface CassandraMapper { > > Javadoc on the interface would be good. > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > cassandra/src/main/java/org/**apache/sling/cassandra/** > > resource/provider/mapper/**CassandraMapperException.java< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/mapper/CassandraMapperException.java > > > > > File > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/mapper/**CassandraMapperException.java > > (right): > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > cassandra/src/main/java/org/**apache/sling/cassandra/** > > resource/provider/mapper/**CassandraMapperException.java#**newcode5< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/mapper/CassandraMapperException.java#newcode5 > > > > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/mapper/**CassandraMapperException.java:**5: > > public CassandraMapperException(**String s) { > > Think about implementing the other constructors so you can set the > > cause. > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > cassandra/src/main/java/org/**apache/sling/cassandra/** > > resource/provider/mapper/**DefaultCassandraMapperImpl.**java< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/mapper/DefaultCassandraMapperImpl.java > > > > > File > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/mapper/**DefaultCassandraMapperImpl.**java > > (right): > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > cassandra/src/main/java/org/**apache/sling/cassandra/** > > resource/provider/mapper/**DefaultCassandraMapperImpl.**java#newcode29< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/mapper/DefaultCassandraMapperImpl.java#newcode29 > > > > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/mapper/**DefaultCassandraMapperImpl.**java:29: > > public class DefaultCassandraMapperImpl implements CassandraMapper { > > Needs javadoc to say what the class does. In particular you might > > mention what the SHA1 is about. > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > cassandra/src/main/java/org/**apache/sling/cassandra/** > > resource/provider/util/**CassandraResourceProviderUtil.**java< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/util/CassandraResourceProviderUtil.java > > > > > File > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/util/**CassandraResourceProviderUtil.**java > > (right): > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > cassandra/src/main/java/org/**apache/sling/cassandra/** > > resource/provider/util/**CassandraResourceProviderUtil.**java#newcode43< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/util/CassandraResourceProviderUtil.java#newcode43 > > > > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/util/**CassandraResourceProviderUtil.**java:43: > > public class CassandraResourceProviderUtil { > > Be wary of putting code that is really service code in a utility class. > > A good test if if code interacts with or > > modifies an external resource it should not be in a utility class. > > Utility static methods should have no side > > effects. COmmunicating with cassandra causes cassandra to log things, so > > is a side effect. > > > > This is not a hard and fast rule, since commons-io is littered with > > really useful static utility methods that read from streams etc, so just > > think, is the code service or utility. > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > cassandra/src/main/java/org/**apache/sling/cassandra/** > > resource/provider/util/**CassandraResourceProviderUtil.**java#newcode45< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/util/CassandraResourceProviderUtil.java#newcode45 > > > > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/util/**CassandraResourceProviderUtil.**java:45: > > private static Log log = > > LogFactory.getLog(**CassandraResourceProviderUtil.**class); > > Dont use commons logging use slf4j Logger e.g.: > > Logger log = > LogFactory.getLogger(**CassandraResourceProviderUtil.**class); > > > > Also, I prefer LOGGER since its a static constant, but thats just being > > pedantic. > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > cassandra/src/main/java/org/**apache/sling/cassandra/** > > resource/provider/util/**CassandraResourceProviderUtil.**java#newcode90< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/util/CassandraResourceProviderUtil.java#newcode90 > > > > > main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/util/**CassandraResourceProviderUtil.**java:90: > > System.out.println(column.**getValue().toString()); > > Avoid System.out and System.err please > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > > > > cassandra/src/test/java/org/**apache/sling/cassandra/test/**data/populator/* > > *CassandraDataAddTest.java< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/test/java/org/apache/sling/cassandra/test/data/populator/CassandraDataAddTest.java > > > > > File > > main/cassandra/src/test/java/**org/apache/sling/cassandra/**test > > /data/populator/**CassandraDataAddTest.java > > (right): > > > > https://codereview.appspot.**com/11960043/diff/1/main/** > > > > > cassandra/src/test/java/org/**apache/sling/cassandra/test/**data/populator/* > > *CassandraDataAddTest.java#**newcode25< > > > https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/test/java/org/apache/sling/cassandra/test/data/populator/CassandraDataAddTest.java#newcode25 > > > > > main/cassandra/src/test/java/**org/apache/sling/cassandra/**test > > /data/populator/**CassandraDataAddTest.java:25: > > import junit.framework.TestCase; > > you should be using JUnit 4 ie org.junit and not junit.framework which > > IIRC is 3. > > > > Then you dont have to extend classes and the correct test runner is > > used. > > > > Description: > > Sling GSoC2013 Code Review > > > > Please review this at > > https://codereview.appspot.**com/11960043/< > > https://codereview.appspot.com/11960043/> > > > > Affected files: > > A main/cassandra/pom.xml > > A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraConstants.java > > A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraIterable.java > > A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraResource.java > > A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraResourceProvider.java > > A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/**CassandraResourceResolver.java > > A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/mapper/**CassandraMapper.java > > A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/mapper/**CassandraMapperException.java > > A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/mapper/**DefaultCassandraMapperImpl.**java > > A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso > > urce/provider/util/**CassandraResourceProviderUtil.**java > > A main/cassandra/src/test/java/**org/apache/sling/cassandra/**test > > /data/populator/**CassandraDataAddTest.java > > A main/cassandra/src/test/java/**org/apache/sling/cassandra/**test > > /data/populator/**CassandraDataChildNodeIterable**Test.java > > A main/cassandra/src/test/java/**org/apache/sling/cassandra/**test > > /data/populator/**CassandraDataChildNodeTest.**java > > A main/cassandra/src/test/java/**org/apache/sling/cassandra/**test > > /data/populator/**CassandraDataParentNodeTest.**java > > A main/cassandra/src/test/java/**org/apache/sling/cassandra/**test > > /data/populator/**CassandraDataReadTest.java > > A scratch/test/hector-examples-**master/LICENSE > > A scratch/test/hector-examples-**master/README.mdown > > A scratch/test/hector-examples-**master/pom.xml > > A scratch/test/hector-examples-**master/src/main/java/org/test/**m > > e/BasicTest.java > > A scratch/test/hector-examples-**master/src/main/java/org/test/**m > > e/CqlQueryTest.java > > > > > > -- > Thanks > /Dishara >
