> I can't build EM and so was poking through the code. > > - EM{noise, resample, etc} should be just be {noise, resample, etc} to be > consistent with other names in other modules and the core (all access are > already prefixed with via the paths and namespaces). > > Only the files have that name. Functions don't have the prefix.
> > - in general it is less error prone to use an enum for modes rather than a > string (in noise, for example) as they get documented better and can only > exist in correct values > >
> > - calling the arguments to add_noise op1 and op2 is horrible. They should > have descriptive names. > > add_noise uses different functions with different meanings for op1 and op2, thus they can't be described properly. They are 2 arguments.
> > - given we have a rotation class, "project" should use that rather than its > own convention for defining rotations > > project is a hard function to develop, and the math behind it is far better implemented with a given convention. Hence the definition.
> > - in general we avoid passing return return values as arguments > ("project"). At the very least, if you do that you need to make sure that > the non-return arguments are const refs, rather than refs and the last > arguments. > > reasonable
> > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev > > >
> Only the files have that name. Functions don't have the prefix. Yes, but the headers shouldn't either (everyone using C++ sees the header names). Easily fixed.
> >> >> - calling the arguments to add_noise op1 and op2 is horrible. >> They should have descriptive names. > > add_noise uses different functions with different meanings for op1 and > op2, thus they can't be described properly. They are 2 arguments. As basic good coding guidelines, such things should be separate functions then with descriptive names. You can always share implementation internally, but having the meanings of arguments change based on other arguments is even worse than having generic names on the arguments.
>> > project is a hard function to develop, and the math behind it is far > better implemented with a given convention. Hence the definition. It is probably pretty easy to compute the desired euler angles from a quaternion (but I haven't done so, so I am not entirely sure). If it is, you wouldn't have to change project/
participants (2)
-
Daniel Russel
-
Javier Ángel Velázquez Muriel