Sure, makes sense to do this according to the boyscout principle and based on patches. I will start working on such a patch for the code area I'm working with, not for the whole project. As for an inconsistent state for the condition checking logic, it should not be an issue, as the Preconditions class throws the exact *same exceptions* as the ones that are thrown manually, so in fact it should behave exactly the same for a client of the class. About clustering and classification, I have not worked with that portion of the code yet - I'm focusing on recommendation algorithms for now. Thanks for the feedback, I'll make sure to open the JIRA issue. Eugen.
On Tue, Aug 10, 2010 at 11:00 PM, Ted Dunning <[email protected]> wrote: > Guava is a dependency that I added not so long ago. > > I think it is good practice to do this going forward, but it really will be > another one of those forever cleanup tasks to get it done completely. > > Certainly, it is fine to add checks in the new style, but I don't see an > issue leaving old checks in the old style unless somebody is actually > working on them. I don't think that makes our consistency all that much > worse than it is and it makes the modified code better. > > More important in my mind is the data-structure rationalization that we > need > to do for clustering and classification. > > On Tue, Aug 10, 2010 at 10:18 AM, Sean Owen <[email protected]> wrote: > > > Is Guava already a dependency somehow? maybe so, I never looked. > > > > If so I find this a decent idea. It's a big undertaking to convert all > > arg checking, and perhaps tighten up argument checking in other > > places. You'd be welcome to do so and file a JIRA with a patch. > > > > My concern would be a half-way patch. Then we'd be in a more > > inconsistent state and that's not good. > > > > On Tue, Aug 10, 2010 at 8:17 AM, Eugen Paraschiv <[email protected]> > > wrote: > > > Hi, > > > We are starting to use Mahout for a recommendation project at work - it > > has > > > been a good experience so far. We went through the integration process > > > without any notable hurdles, and we’ve kept some notes along the way. I > > will > > > start with a general observation and follow up with some more in the > > future, > > > so that we may get some feedback from the community and help with > patches > > > where needed. > > > One thing I’ve noticed is that there doesn’t seem to be a consistent > way > > of > > > validating method arguments. > > > There is a Preconditions utility class in *G*uava (the formed Google > > > Collections) for checking various stuff such as not nulls or boolean > > > conditions: > > > Preconditions.checkNotNull( object ); or > > > Preconditions.checkState( expression, “errorMessage” ); > > > This can be used instead of manual checks such as: > > > if( delimiterTwo < 0 ){ > > > throw new IllegalArgumentException( "Bad line: " + line ); > > > } > > > This kind of checking has many advantages: > > > - it ensures a consistent exception logic, making sure you always throw > > the > > > exact same exceptions in speciffic situations (such as checking an > > argument) > > > - it makes the code more readable and reduces the cognitive load for a > > > client going through it > > > - it keeps the method at a single level of abstraction - throwing an > > > exception when a parameter is invalid is a low level of abstraction > > whereas > > > the following logic is a higher level of abstraction - the two should > not > > be > > > mixed > > > Seeing how *Guava* is already a dependency, it makes sense to use it to > > it > > > full potential. Reducing the cognitive load of the code may not be that > > > relevant in some cases, but it is very important when dealing with very > > long > > > methods such as *FileDataModel* - *processLine*. > > > Is this a direction that makes sense for Mahout? If so, how should I go > > > forward with helping - is the standard route of opening JIRA issues and > > > submitting patches OK? > > > Thanks. > > > Eugen. > > > > > >
