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.
> 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
