Is grouping preconditions acceptable for a DRY stay?

Question

When writing prerequisites for different functions with similar parameters, I want to group statements or exceptions into a static method, rather than writing them explicitly. For example, instead of

GetFooForUser(User user) { assert(null != user); assert(user.ID > 0); // db ids always start at 1 assert(something else that defines a valid user); ... // do some foo work } GetBarForUser(User user) { assert(null != user); assert(user.ID > 0); // db ids always start at 1 assert(something else that defines a valid user); ... // do some bar work } 

I would rather write

 GetFooForUser(User user) { CheckUserIsValid(user); // do some foo work } GetBarForUser(User user) { CheckUserIsValid(user); // do some bar work } static CheckUserIsValid(User user) { assert(null != user); assert(user.ID > 0); // db ids always start at 1 assert(something else that defines a valid user); ... } 

This seems more natural and helps reduce the code I need to write (and may even reduce the number of errors in my statements!), But it seems to contradict the idea that the preconditions should help document the exact intent of the method.

Is this a common anti-pattern or is there any significant reason not to do this?

Also, would the answer be different if I were to use exceptions?

Answer 1

Solid answer

This is perfectly acceptable: .NET source code wraps the terms.

For example, the StringBuilder source code has a method called VerifyClassInvariant() , which it calls 18 times. The method just checks for correctness.

 private void VerifyClassInvariant() { BCLDebug.Correctness((uint)(m_ChunkOffset + m_ChunkChars.Length) >= m_ChunkOffset, "Integer Overflow"); StringBuilder currentBlock = this; int maxCapacity = this.m_MaxCapacity; for (; ; ) { // All blocks have copy of the maxCapacity. Contract.Assert(currentBlock.m_MaxCapacity == maxCapacity, "Bad maxCapacity"); Contract.Assert(currentBlock.m_ChunkChars != null, "Empty Buffer"); Contract.Assert(currentBlock.m_ChunkLength <= currentBlock.m_ChunkChars.Length, "Out of range length"); Contract.Assert(currentBlock.m_ChunkLength >= 0, "Negative length"); Contract.Assert(currentBlock.m_ChunkOffset >= 0, "Negative offset"); StringBuilder prevBlock = currentBlock.m_ChunkPrevious; if (prevBlock == null) { Contract.Assert(currentBlock.m_ChunkOffset == 0, "First chunk offset is not 0"); break; } // There are no gaps in the blocks. Contract.Assert(currentBlock.m_ChunkOffset == prevBlock.m_ChunkOffset + prevBlock.m_ChunkLength, "There is a gap between chunks!"); currentBlock = prevBlock; } } 

Dialog

Is it possible to group statements or exceptions into a static method and not write them explicitly?

Yes. Everything is good.

... seems to run counter to the idea that preconditions should help document the exact intent of the method.

The name of the method that wraps the assert or Exception assert should indicate the intent of the shell. In addition, if the reader wants to know the specifics, then it can view the implementation of the method (unless it is closed-source.)

Is this good or bad practice and why?

It is good practice to transfer a set of related or commonly reused asserts to another method, as it can improve readability and facilitate maintenance.

Is this a common anti-pattern?

On the contrary, vice versa. You can see something like this, and it is really useful and recommended, because it communicates well.

 private void IfNotValidInputForPaymentFormThenThrow(string input) { if(input.Length < 10 || input.Length) { throw new ArgumentException("Wrong length"); } // other conditions omitted } 

It is a good idea to add ThenThrow to the end of the method so that the caller knows what you are throwing. Otherwise, you may need to return bool , and let the caller decide what to do if the condition fails.

 private bool IsValidInputForPaymentForm(string input) { if(input.Length < 10 || input.Length) { return true; } else { return false; } // other conditions omitted } 

Then the calling code can call:

 if(!IsValidInputForPaymentForm(someStringInput) { throw new ArgumentException(); } 

Or here's another example from a .NET source that throws an exception if some conditions fail ( ThrowHelper just throws an exception.)

 // Allow nulls for reference types and Nullable<U>, // but not for value types. internal static void IfNullAndNullsAreIllegalThenThrow<T>( object value, ExceptionArgument argName) { // Note that default(T) is not equal to null // for value types except when T is Nullable<U>. if (value == null && !(default(T) == null)) ThrowHelper.ThrowArgumentNullException(argName); } 

Are there any significant reasons not to do this?

I might think that if the method name does not explain what you are checking, and there is no easy way to view the source, then you should probably avoid wrapping conditions.