[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [IMP-dev] EM issues




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