Skip to content

Commit e93d1f6

Browse files
JeremyCaneyJeremyCaney
committed
Remove guard clauses for parameters on internal, private methods
With C# 8's nullable reference types in place, and subsequent improvements in C# 10 to code analysis, it is unnecessary to place guard clauses in `private` or `internal` methods for non-nullable reference types. This is because the values passed to them will already be validated at their source location. Impressively, Roslyn will even identify if a public method doesn't have a guard clause but relies on a guard clause in e.g. a `private` or `internal` overload; as such, removing these doesn't open us up to exposure from unprotected `public` passthroughs. (Or, rather, it did—notably in `TopicRelationshipMultiMap`—but Roslyn's flow analysis caught this, and so I was able to reintroduce the guard clauses on the `private` overloads. In addition to reducing unnecessary code, this might have a _slight_ performance impact. These checks are fast, but there's also a lot of them, and on commonly references functions that could add up.
1 parent eec0d42 commit e93d1f6

10 files changed

Lines changed: 2 additions & 177 deletions

File tree

‎OnTopic.Data.Sql/SqlCommandExtensions.cs‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -123,12 +123,6 @@ private static void AddParameter(
123123
ParameterDirection paramDirection = ParameterDirection.Input
124124
) {
125125

126-
/*------------------------------------------------------------------------------------------------------------------------
127-
| Validate input
128-
\-----------------------------------------------------------------------------------------------------------------------*/
129-
Contract.Requires(command, "The SQL command object must be specified.");
130-
Contract.Requires(command.Parameters, "The SQL command object's parameters collection must be available");
131-
132126
/*------------------------------------------------------------------------------------------------------------------------
133127
| Establish basic parameter
134128
\-----------------------------------------------------------------------------------------------------------------------*/

‎OnTopic/Collections/Specialized/TrackedRecordCollection{TItem,TValue,TAttribute}.cs‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,6 @@ public abstract class TrackedRecordCollection<TItem, TValue, TAttribute> :
4545
/// <param name="parentTopic">A reference to the topic that the current collection is bound to.</param>
4646
internal TrackedRecordCollection(Topic parentTopic) : base(StringComparer.OrdinalIgnoreCase) {
4747

48-
/*------------------------------------------------------------------------------------------------------------------------
49-
| Validate parameters
50-
\-----------------------------------------------------------------------------------------------------------------------*/
51-
Contract.Requires(parentTopic, nameof(parentTopic));
52-
5348
/*------------------------------------------------------------------------------------------------------------------------
5449
| Set properties
5550
\-----------------------------------------------------------------------------------------------------------------------*/

‎OnTopic/Internal/Reflection/MemberAccessor.cs‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@ internal MemberAccessor(MemberInfo memberInfo) {
4040
/*------------------------------------------------------------------------------------------------------------------------
4141
| Validate parameters
4242
\-----------------------------------------------------------------------------------------------------------------------*/
43-
Contract.Requires(memberInfo, nameof(memberInfo));
4443
Contract.Requires(
4544
memberInfo is PropertyInfo or MethodInfo,
4645
$"The {nameof(memberInfo)} parameter must be of type {nameof(PropertyInfo)} or {nameof(MethodInfo)}."
@@ -163,8 +162,6 @@ internal bool IsSettable(Type? sourceType = null, bool allowConversion = false)
163162
/*------------------------------------------------------------------------------------------------------------------------
164163
| Validate parameters
165164
\-----------------------------------------------------------------------------------------------------------------------*/
166-
Contract.Requires(source, nameof(source));
167-
168165
if (MemberInfo.DeclaringType != source.GetType() && !MemberInfo.DeclaringType.IsAssignableFrom(source.GetType())) {
169166
throw new ArgumentException(
170167
$"The {nameof(MemberAccessor)} for {MemberInfo.DeclaringType} cannot be used to access a member of {source.GetType()}",
@@ -214,8 +211,6 @@ internal void SetValue(object target, object? value, bool allowConversion = fals
214211
/*------------------------------------------------------------------------------------------------------------------------
215212
| Validate parameters
216213
\-----------------------------------------------------------------------------------------------------------------------*/
217-
Contract.Requires(target, nameof(target));
218-
219214
if (MemberInfo.DeclaringType != target.GetType() && !MemberInfo.DeclaringType.IsAssignableFrom(target.GetType())) {
220215
throw new ArgumentException(
221216
$"The {nameof(MemberAccessor)} for {MemberInfo.DeclaringType} cannot be used to set a member of {target.GetType()}",

‎OnTopic/Internal/Reflection/TypeAccessor.cs‎

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -62,11 +62,6 @@ internal class TypeAccessor {
6262
/// <param name="type"></param>
6363
internal TypeAccessor(Type type) {
6464

65-
/*------------------------------------------------------------------------------------------------------------------------
66-
| Validate parameters
67-
\-----------------------------------------------------------------------------------------------------------------------*/
68-
Contract.Requires(type, nameof(type));
69-
7065
/*------------------------------------------------------------------------------------------------------------------------
7166
| Initialize fields, properties
7267
\-----------------------------------------------------------------------------------------------------------------------*/
@@ -213,12 +208,6 @@ internal bool HasGettableMethod<T>(string name, Type? targetType = null) where T
213208
/// <returns>The value returned from the member.</returns>
214209
internal object? GetValue(object source, string memberName) {
215210

216-
/*------------------------------------------------------------------------------------------------------------------------
217-
| Validate parameters
218-
\-----------------------------------------------------------------------------------------------------------------------*/
219-
Contract.Requires(source, nameof(source));
220-
Contract.Requires(memberName, nameof(memberName));
221-
222211
/*------------------------------------------------------------------------------------------------------------------------
223212
| Retrieve member
224213
\-----------------------------------------------------------------------------------------------------------------------*/
@@ -335,12 +324,6 @@ internal bool HasSettableMethod<T>(string methodName, Type? targetType = null) w
335324
/// </param>
336325
internal void SetValue(object target, string memberName, object? value, bool allowConversion = false) {
337326

338-
/*------------------------------------------------------------------------------------------------------------------------
339-
| Validate parameters
340-
\-----------------------------------------------------------------------------------------------------------------------*/
341-
Contract.Requires(target, nameof(target));
342-
Contract.Requires(memberName, nameof(memberName));
343-
344327
/*------------------------------------------------------------------------------------------------------------------------
345328
| Validate dependencies
346329
\-----------------------------------------------------------------------------------------------------------------------*/

‎OnTopic/Internal/Reflection/TypeAccessorCache.cs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@ internal static class TypeAccessorCache {
3434
/// <param name="type">The <see cref="Type"/> that needs to be dynamically accessed.</param>
3535
/// <returns>A <see cref="TypeAccessor"/> for dynamically accessing the supplied <paramref name="type"/>.</returns>
3636
internal static TypeAccessor GetTypeAccessor(Type type) {
37-
Contract.Requires(type, nameof(type));
3837
return _cache.GetOrAdd(type, t => new(t));
3938
}
4039

‎OnTopic/Mapping/Internal/ItemConfiguration.cs‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,12 +51,6 @@ internal class ItemConfiguration {
5151
/// <param name="attributePrefix">The prefix to apply to the attributes.</param>
5252
internal ItemConfiguration(ICustomAttributeProvider source, string name, string? attributePrefix = "") {
5353

54-
/*------------------------------------------------------------------------------------------------------------------------
55-
| Validate parameters
56-
\-----------------------------------------------------------------------------------------------------------------------*/
57-
Contract.Requires(source, nameof(source));
58-
Contract.Requires(name, nameof(name));
59-
6054
/*------------------------------------------------------------------------------------------------------------------------
6155
| Set backing property
6256
\-----------------------------------------------------------------------------------------------------------------------*/

‎OnTopic/Mapping/Internal/MappedTopicCache.cs‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -54,13 +54,6 @@ internal bool TryGetValue(int topicId, Type type, [NotNullWhen(true)] out Mapped
5454
/// <param name="viewModel">The mapped view model associated with the <paramref name="topicId"/>.</param>
5555
internal void Register(int topicId, AssociationTypes associations, object viewModel) {
5656

57-
/*------------------------------------------------------------------------------------------------------------------------
58-
| Validate input
59-
\-----------------------------------------------------------------------------------------------------------------------*/
60-
Contract.Requires(topicId, nameof(topicId));
61-
Contract.Requires(associations, nameof(associations));
62-
Contract.Requires(viewModel, nameof(viewModel));
63-
6457
/*------------------------------------------------------------------------------------------------------------------------
6558
| Construct cache entry
6659
\-----------------------------------------------------------------------------------------------------------------------*/
@@ -96,12 +89,6 @@ internal void Register(int topicId, AssociationTypes associations, object viewMo
9689
/// <param name="type">The <see cref="Type"/> that the <see cref="Topic"/> is being mapped to.</param>
9790
internal MappedTopicCacheEntry Preregister(int topicId, Type type) {
9891

99-
/*------------------------------------------------------------------------------------------------------------------------
100-
| Validate input
101-
\-----------------------------------------------------------------------------------------------------------------------*/
102-
Contract.Requires(topicId, nameof(topicId));
103-
Contract.Requires(type, nameof(type));
104-
10592
/*------------------------------------------------------------------------------------------------------------------------
10693
| Construct cache entry
10794
\-----------------------------------------------------------------------------------------------------------------------*/

‎OnTopic/Mapping/Reverse/BindingModelValidator.cs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,6 @@ [DisallowNull]Type listType
310310
Contract.Requires(sourceType, nameof(sourceType));
311311
Contract.Requires(configuration, nameof(configuration));
312312
Contract.Requires(attributeDescriptor, nameof(attributeDescriptor));
313-
//Contract.Requires(listType, nameof(listType));
314313

315314
/*------------------------------------------------------------------------------------------------------------------------
316315
| Define variables

‎OnTopic/Mapping/Reverse/ReverseTopicMappingService.cs‎

Lines changed: 2 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -192,12 +192,6 @@ public ReverseTopicMappingService(ITopicRepository topicRepository) {
192192
\-----------------------------------------------------------------------------------------------------------------------*/
193193
if (source is null) return target;
194194

195-
/*------------------------------------------------------------------------------------------------------------------------
196-
| Validate input
197-
\-----------------------------------------------------------------------------------------------------------------------*/
198-
Contract.Requires(target, nameof(target));
199-
Contract.Assume(target.ContentType, nameof(target.ContentType));
200-
201195
/*------------------------------------------------------------------------------------------------------------------------
202196
| Validate model
203197
\-----------------------------------------------------------------------------------------------------------------------*/
@@ -244,13 +238,6 @@ private async Task SetPropertyAsync(
244238
string? attributePrefix = null
245239
) {
246240

247-
/*------------------------------------------------------------------------------------------------------------------------
248-
| Validate parameters
249-
\-----------------------------------------------------------------------------------------------------------------------*/
250-
Contract.Requires(source, nameof(source));
251-
Contract.Requires(target, nameof(target));
252-
Contract.Requires(property, nameof(property));
253-
254241
/*------------------------------------------------------------------------------------------------------------------------
255242
| Establish per-property variables
256243
\-----------------------------------------------------------------------------------------------------------------------*/
@@ -349,15 +336,7 @@ private static void SetScalarValue(
349336
object source,
350337
Topic target,
351338
PropertyConfiguration configuration
352-
)
353-
{
354-
355-
/*------------------------------------------------------------------------------------------------------------------------
356-
| Validate parameters
357-
\-----------------------------------------------------------------------------------------------------------------------*/
358-
Contract.Requires(source, nameof(source));
359-
Contract.Requires(target, nameof(target));
360-
Contract.Requires(configuration, nameof(configuration));
339+
) {
361340

362341
/*------------------------------------------------------------------------------------------------------------------------
363342
| Attempt to retrieve value from the binding model property
@@ -408,15 +387,7 @@ private void SetRelationships(
408387
object source,
409388
Topic target,
410389
PropertyConfiguration configuration
411-
)
412-
{
413-
414-
/*------------------------------------------------------------------------------------------------------------------------
415-
| Validate parameters
416-
\-----------------------------------------------------------------------------------------------------------------------*/
417-
Contract.Requires(source, nameof(source));
418-
Contract.Requires(target, nameof(target));
419-
Contract.Requires(configuration, nameof(configuration));
390+
) {
420391

421392
/*------------------------------------------------------------------------------------------------------------------------
422393
| Retrieve source list
@@ -468,13 +439,6 @@ private async Task SetNestedTopicsAsync(
468439
PropertyConfiguration configuration
469440
) {
470441

471-
/*------------------------------------------------------------------------------------------------------------------------
472-
| Validate parameters
473-
\-----------------------------------------------------------------------------------------------------------------------*/
474-
Contract.Requires(source, nameof(source));
475-
Contract.Requires(target, nameof(target));
476-
Contract.Requires(configuration, nameof(configuration));
477-
478442
/*------------------------------------------------------------------------------------------------------------------------
479443
| Retrieve source list
480444
\-----------------------------------------------------------------------------------------------------------------------*/
@@ -516,13 +480,6 @@ private void SetReference(
516480
PropertyConfiguration configuration
517481
) {
518482

519-
/*------------------------------------------------------------------------------------------------------------------------
520-
| Validate parameters
521-
\-----------------------------------------------------------------------------------------------------------------------*/
522-
Contract.Requires(source, nameof(source));
523-
Contract.Requires(target, nameof(target));
524-
Contract.Requires(configuration, nameof(configuration));
525-
526483
/*------------------------------------------------------------------------------------------------------------------------
527484
| Retrieve source value
528485
\-----------------------------------------------------------------------------------------------------------------------*/
@@ -578,12 +535,6 @@ private async Task PopulateTargetCollectionAsync(
578535
KeyedTopicCollection targetList
579536
) {
580537

581-
/*------------------------------------------------------------------------------------------------------------------------
582-
| Validate parameters
583-
\-----------------------------------------------------------------------------------------------------------------------*/
584-
Contract.Requires(sourceList, nameof(sourceList));
585-
Contract.Requires(targetList, nameof(targetList));
586-
587538
/*------------------------------------------------------------------------------------------------------------------------
588539
| Queue up mapping tasks
589540
\-----------------------------------------------------------------------------------------------------------------------*/

0 commit comments

Comments
 (0)