Skip to content

Commit 69caaa2

Browse files
JeremyCaneyJeremyCaney
committed
Merge branch 'feature/TopicFactory-Create-parameters' into develop
Previously, there were two overloads for the `TopicFactory.Create()` method which used competing parameter orders. Not only did the parameter order compete within the overloads, but it also competed with the corresponding parameter order of the `Topic` constructor. This is confusing. To mitigate this, I consolidated the two `Create()` overloads and, as part of that process, standardized the parameter order. This allows the logic between these methods to be centralized, and maintains consistency across the application. Technically, this is a breaking change. In practice, however, the vast majority of callers to `Create()` never pass an `Id` and, therefore, won't notice the change. The only production code that's expected to use this overload are concrete implementations of `ITopicRepository.Load()` which interact directly with a persistence store (i.e., not abstract base classes or decorators or test stubs). The only concrete implementation we have is `SqlTopicRepository`, and it actually loads the `parent` topic separately, and thus doesn't break with this change. That said, a lot of non-production code breaks with this change. Namely, it's common for unit tests—and test doubles—to generate topics using the `TopicFactory` that have both a `parent` (in order to simulate a topic graph) and an `id` (in order to simulate a saved entity, even though the number is arbitrary). As such, the unit tests needed to be updated to reflect this change. Outside of that, this isn't _expected_ to impact any third-party implementations.
2 parents fdd25e7 + 48a2ecc commit 69caaa2

File tree

6 files changed

+34
-90
lines changed

6 files changed

+34
-90
lines changed

‎OnTopic.TestDoubles/StubTopicRepository.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ AttributeDescriptor addAttribute(
252252
container = TopicFactory.Create("Attributes", "List", contentType);
253253
container.Attributes.SetBoolean("IsHidden", true);
254254
}
255-
var attribute = (AttributeDescriptor)TopicFactory.Create(attributeKey, editorType, currentAttributeId++, container);
255+
var attribute = (AttributeDescriptor)TopicFactory.Create(attributeKey, editorType, container, currentAttributeId++);
256256
attribute.IsRequired = isRequired;
257257
attribute.IsExtendedAttribute = isExtended;
258258
return attribute;
@@ -272,7 +272,7 @@ AttributeDescriptor addAttribute(
272272
/*------------------------------------------------------------------------------------------------------------------------
273273
| Establish content
274274
\-----------------------------------------------------------------------------------------------------------------------*/
275-
var web = TopicFactory.Create("Web", "Page", 10000, rootTopic);
275+
var web = TopicFactory.Create("Web", "Page", rootTopic, 10000);
276276

277277
CreateFakeData(web, 2, 3);
278278

@@ -296,7 +296,7 @@ AttributeDescriptor addAttribute(
296296
/// </summary>
297297
private void CreateFakeData(Topic parent, int count = 3, int depth = 3) {
298298
for (var i = 0; i < count; i++) {
299-
var topic = TopicFactory.Create(parent.Key + "_" + i, "Page", parent.Id + (int)Math.Pow(10, depth) * i, parent);
299+
var topic = TopicFactory.Create(parent.Key + "_" + i, "Page", parent, parent.Id + (int)Math.Pow(10, depth) * i);
300300
topic.Attributes.SetValue("ParentKey", parent.Key);
301301
topic.Attributes.SetValue("DepthCount", (depth+i).ToString(CultureInfo.InvariantCulture));
302302
if (depth > 0) {

‎OnTopic.Tests/ITopicRepositoryTest.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ public void Load_Default_ReturnsTopicTopic() {
7474
public void Load_ValidUniqueKey_ReturnsCorrectTopic() {
7575

7676
var topic = _topicRepository.Load("Root:Configuration:ContentTypes:Page");
77-
var child = TopicFactory.Create("Child", "ContentType", Int32.MaxValue, topic);
77+
var child = TopicFactory.Create("Child", "ContentType", topic, Int32.MaxValue);
7878

7979
Assert.AreEqual<string>("Page", topic.Key);
8080

‎OnTopic.Tests/SqlTopicRepositoryTest.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -200,8 +200,8 @@ public void LoadTopicGraph_WithMissingReference_NotFullyLoaded() {
200200
public void LoadTopicGraph_WithDeletedRelationship_RemovesRelationship() {
201201

202202
var topic = TopicFactory.Create("Test", "Container", 1);
203-
var child = TopicFactory.Create("Child", "Container", 2, topic);
204-
var related = TopicFactory.Create("Related", "Container", 3, topic);
203+
var child = TopicFactory.Create("Child", "Container", topic, 2);
204+
var related = TopicFactory.Create("Related", "Container", topic, 3);
205205

206206
child.Relationships.SetTopic("Test", related);
207207

‎OnTopic.Tests/TopicMappingServiceTest.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -770,7 +770,7 @@ public async Task Map_MetadataLookup_ReturnsLookupItems() {
770770
public async Task Map_CircularReference_ReturnsCachedParent() {
771771

772772
var topic = TopicFactory.Create("Test", "Circular", 1);
773-
var childTopic = TopicFactory.Create("ChildTopic", "Circular", 2, topic);
773+
var childTopic = TopicFactory.Create("ChildTopic", "Circular", topic, 2);
774774

775775
var mappedTopic = await _mappingService.MapAsync<CircularTopicViewModel>(topic).ConfigureAwait(false);
776776

‎OnTopic.Tests/TopicQueryingTest.cs

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,10 @@ public TopicQueryingTest() {
5454
public void FindAllByAttribute_ReturnsCorrectTopics() {
5555

5656
var parentTopic = TopicFactory.Create("ParentTopic", "Page", 1);
57-
var childTopic = TopicFactory.Create("ChildTopic", "Page", 5, parentTopic);
58-
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", 20, childTopic);
59-
var grandNieceTopic = TopicFactory.Create("GrandNieceTopic", "Page", 3, childTopic);
60-
var greatGrandChildTopic = TopicFactory.Create("GreatGrandChildTopic", "Page", 7, grandChildTopic);
57+
var childTopic = TopicFactory.Create("ChildTopic", "Page", parentTopic, 5);
58+
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", childTopic, 20);
59+
var grandNieceTopic = TopicFactory.Create("GrandNieceTopic", "Page", childTopic, 3);
60+
var greatGrandChildTopic = TopicFactory.Create("GreatGrandChildTopic", "Page", grandChildTopic, 7);
6161

6262
grandChildTopic.Attributes.SetValue("Foo", "Baz");
6363
greatGrandChildTopic.Attributes.SetValue("Foo", "Bar");
@@ -79,9 +79,9 @@ public void FindAllByAttribute_ReturnsCorrectTopics() {
7979
public void FindFirstParent_ReturnsCorrectTopic() {
8080

8181
var parentTopic = TopicFactory.Create("ParentTopic", "Page", 1);
82-
var childTopic = TopicFactory.Create("ChildTopic", "Page", 5, parentTopic);
83-
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", 20, childTopic);
84-
var greatGrandChildTopic = TopicFactory.Create("GreatGrandChildTopic", "Page", 7, grandChildTopic);
82+
var childTopic = TopicFactory.Create("ChildTopic", "Page", parentTopic, 5);
83+
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", childTopic, 20);
84+
var greatGrandChildTopic = TopicFactory.Create("GreatGrandChildTopic", "Page", grandChildTopic, 7);
8585

8686
var foundTopic = greatGrandChildTopic.FindFirstParent(t => t.Id is 5);
8787

@@ -99,9 +99,9 @@ public void FindFirstParent_ReturnsCorrectTopic() {
9999
public void GetRootTopic_ReturnsRootTopic() {
100100

101101
var parentTopic = TopicFactory.Create("ParentTopic", "Page", 1);
102-
var childTopic = TopicFactory.Create("ChildTopic", "Page", 5, parentTopic);
103-
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", 20, childTopic);
104-
var greatGrandChildTopic = TopicFactory.Create("GreatGrandChildTopic", "Page", 7, grandChildTopic);
102+
var childTopic = TopicFactory.Create("ChildTopic", "Page", parentTopic, 5);
103+
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", childTopic, 20);
104+
var greatGrandChildTopic = TopicFactory.Create("GreatGrandChildTopic", "Page", grandChildTopic, 7);
105105

106106
var rootTopic = greatGrandChildTopic.GetRootTopic();
107107

@@ -119,7 +119,7 @@ public void GetRootTopic_ReturnsRootTopic() {
119119
public void GetByUniqueKey_RootKey_ReturnsRootTopic() {
120120

121121
var parentTopic = TopicFactory.Create("ParentTopic", "Page", 1);
122-
_ = TopicFactory.Create("ChildTopic", "Page", 2, parentTopic);
122+
_ = TopicFactory.Create("ChildTopic", "Page", parentTopic, 2);
123123

124124
var foundTopic = parentTopic.GetByUniqueKey("ParentTopic");
125125

@@ -138,10 +138,10 @@ public void GetByUniqueKey_RootKey_ReturnsRootTopic() {
138138
public void GetByUniqueKey_ValidKey_ReturnsTopic() {
139139

140140
var parentTopic = TopicFactory.Create("ParentTopic", "Page", 1);
141-
var childTopic = TopicFactory.Create("ChildTopic", "Page", 5, parentTopic);
142-
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", 20, childTopic);
143-
var greatGrandChildTopic1 = TopicFactory.Create("GreatGrandChildTopic1", "Page", 7, grandChildTopic);
144-
var greatGrandChildTopic2 = TopicFactory.Create("GreatGrandChildTopic2", "Page", 7, grandChildTopic);
141+
var childTopic = TopicFactory.Create("ChildTopic", "Page", parentTopic, 5);
142+
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", childTopic, 20);
143+
var greatGrandChildTopic1 = TopicFactory.Create("GreatGrandChildTopic1", "Page", grandChildTopic, 7);
144+
var greatGrandChildTopic2 = TopicFactory.Create("GreatGrandChildTopic2", "Page", grandChildTopic, 7);
145145

146146
var foundTopic = greatGrandChildTopic1.GetByUniqueKey("ParentTopic:ChildTopic:GrandChildTopic:GreatGrandChildTopic2");
147147

@@ -160,9 +160,9 @@ public void GetByUniqueKey_ValidKey_ReturnsTopic() {
160160
public void GetByUniqueKey_InvalidKey_ReturnsNull() {
161161

162162
var parentTopic = TopicFactory.Create("ParentTopic", "Page", 1);
163-
var childTopic = TopicFactory.Create("ChildTopic", "Page", 5, parentTopic);
164-
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", 20, childTopic);
165-
var greatGrandChildTopic = TopicFactory.Create("GreatGrandChildTopic", "Page", 7, grandChildTopic);
163+
var childTopic = TopicFactory.Create("ChildTopic", "Page", parentTopic, 5);
164+
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", childTopic, 20);
165+
var greatGrandChildTopic = TopicFactory.Create("GreatGrandChildTopic", "Page", grandChildTopic, 7);
166166

167167
var foundTopic = greatGrandChildTopic.GetByUniqueKey("ParentTopic:ChildTopic:GrandChildTopic:GreatGrandChildTopic2");
168168

‎OnTopic/TopicFactory.cs

Lines changed: 9 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -31,66 +31,6 @@ public static class TopicFactory {
3131
/*==========================================================================================================================
3232
| METHOD: CREATE
3333
\-------------------------------------------------------------------------------------------------------------------------*/
34-
/// <summary>
35-
/// Factory method for creating new strongly-typed instances of the topics class, assuming a strongly-typed subclass is
36-
/// available.
37-
/// </summary>
38-
/// <remarks>
39-
/// When creating new attributes, the <see cref="AttributeValue"/>s for both <see cref="Topic.Key"/> and <see
40-
/// cref="Topic.ContentType"/> will be set to <see cref="AttributeValue.IsDirty"/>, which is required in order to
41-
/// correctly save new topics to the database.
42-
/// </remarks>
43-
/// <param name="key">A string representing the key for the new topic instance.</param>
44-
/// <param name="contentType">A string representing the key of the target content type.</param>
45-
/// <param name="parent">Optional topic to set as the new topic's parent.</param>
46-
/// <exception cref="ArgumentException">
47-
/// Thrown when the class representing the content type is found, but doesn't derive from <see cref="Topic"/>.
48-
/// </exception>
49-
/// <returns>A strongly-typed instance of the <see cref="Topic"/> class based on the target content type.</returns>
50-
/// <requires description="The topic key must be specified." exception="T:System.ArgumentNullException">
51-
/// !String.IsNullOrWhiteSpace(key)
52-
/// </requires>
53-
/// <requires
54-
/// decription="The key should be an alphanumeric sequence; it should not contain spaces or symbols."
55-
/// exception="T:System.ArgumentException">
56-
/// !key.Contains(" ")
57-
/// </requires>
58-
/// <requires description="The content type key must be specified." exception="T:System.ArgumentNullException">
59-
/// !String.IsNullOrWhiteSpace(contentType)
60-
/// </requires>
61-
/// <requires
62-
/// decription="The contentType should be an alphanumeric sequence; it should not contain spaces or symbols."
63-
/// exception="T:System.ArgumentException">
64-
/// !contentType.Contains(" ")
65-
/// </requires>
66-
public static Topic Create(string key, string contentType, Topic? parent = null) {
67-
68-
/*------------------------------------------------------------------------------------------------------------------------
69-
| Validate contracts
70-
\-----------------------------------------------------------------------------------------------------------------------*/
71-
Contract.Requires<ArgumentNullException>(!String.IsNullOrWhiteSpace(key), nameof(key));
72-
Contract.Requires<ArgumentNullException>(!String.IsNullOrWhiteSpace(contentType), nameof(contentType));
73-
TopicFactory.ValidateKey(key);
74-
TopicFactory.ValidateKey(contentType);
75-
76-
/*------------------------------------------------------------------------------------------------------------------------
77-
| Determine target type
78-
\-----------------------------------------------------------------------------------------------------------------------*/
79-
var targetType = TypeLookupService.Lookup(contentType);
80-
81-
Contract.Assume(
82-
targetType,
83-
$"The content type {contentType} could not be located in the ITypeLookupService, and no fallback could be " +
84-
$"identified."
85-
);
86-
87-
/*------------------------------------------------------------------------------------------------------------------------
88-
| Identify the appropriate topic
89-
\-----------------------------------------------------------------------------------------------------------------------*/
90-
return (Topic)Activator.CreateInstance(targetType, key, contentType, parent, -1)!;
91-
92-
}
93-
9434
/// <summary>
9535
/// Factory method for creating new strongly-typed instances of the topics class, assuming a strongly-typed subclass is
9636
/// available. Used for cases where a <see cref="Topic"/> is being deserialized from an existing instance, as indicated
@@ -103,22 +43,22 @@ public static Topic Create(string key, string contentType, Topic? parent = null)
10343
/// </remarks>
10444
/// <param name="key">A string representing the key for the new topic instance.</param>
10545
/// <param name="contentType">A string representing the key of the target content type.</param>
106-
/// <param name="id">The unique identifier assigned by the data store for an existing topic.</param>
10746
/// <param name="parent">Optional topic to set as the new topic's parent.</param>
47+
/// <param name="id">The unique identifier assigned by the data store for an existing topic.</param>
10848
/// <exception cref="ArgumentException">
10949
/// Thrown when the class representing the content type is found, but doesn't derive from <see cref="Topic"/>.
11050
/// </exception>
11151
/// <returns>A strongly-typed instance of the <see cref="Topic"/> class based on the target content type.</returns>
112-
public static Topic Create(string key, string contentType, int id, Topic? parent = null) {
52+
public static Topic Create(string key, string contentType, Topic? parent = null, int id = -1) {
11353

11454
/*------------------------------------------------------------------------------------------------------------------------
11555
| Validate input
11656
\-----------------------------------------------------------------------------------------------------------------------*/
11757
Contract.Requires<ArgumentNullException>(!String.IsNullOrWhiteSpace(key), nameof(key));
11858
Contract.Requires<ArgumentNullException>(!String.IsNullOrWhiteSpace(contentType), nameof(contentType));
119-
Contract.Requires<ArgumentOutOfRangeException>(id > 0, nameof(id));
120-
TopicFactory.ValidateKey(key);
121-
TopicFactory.ValidateKey(contentType);
59+
60+
ValidateKey(key);
61+
ValidateKey(contentType);
12262

12363
/*------------------------------------------------------------------------------------------------------------------------
12464
| Determine target type
@@ -138,6 +78,10 @@ public static Topic Create(string key, string contentType, int id, Topic? parent
13878

13979
}
14080

81+
/// <inheritdoc cref="Create(String, String, Topic?, Int32)"/>
82+
public static Topic Create(string key, string contentType, int id) =>
83+
Create(key, contentType, null, id);
84+
14185
/*==========================================================================================================================
14286
| METHOD: VALIDATE KEY
14387
\-------------------------------------------------------------------------------------------------------------------------*/

0 commit comments

Comments
 (0)