You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by Ian Boston <ie...@tfd.co.uk> on 2013/07/27 08:07:25 UTC
"[GSoC2013] Code Review"
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:
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
Re: "[GSoC2013] Code Review"
Posted by Ian Boston <ie...@tfd.co.uk>.
On 27 July 2013 17:14, Dishara Wijewardana <dd...@gmail.com> wrote:
> On Sat, Jul 27, 2013 at 11:37 AM, Ian Boston <ie...@tfd.co.uk> 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
>
Re: "[GSoC2013] Code Review"
Posted by Dishara Wijewardana <dd...@gmail.com>.
On Sat, Jul 27, 2013 at 11:37 AM, Ian Boston <ie...@tfd.co.uk> 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