Major clean-up of code #1

daledupreezISC · 2017-09-20T19:38:20Z

Rename toJSON() to toDynamicObject()
Code formatting and consistency changes
Refactor population utilities
General code cleanup

- Rename toJSON() to toJSONObject() - Code formatting and consistency changes - Refactor population utilities - General code cleanup

daledupreezISC · 2017-09-20T19:40:44Z

WidgetsDirect/Data/PopulateWidgets.cls


+	set tSC = $$$ADDSC(tSC,..CreateWidget("Waterproof Widget", 10.99, 10, "This widget is waterproof to a depth of 100m for up to 7 hours"))


I refactored the population tools to split up the object creation from the object definition, as well as adding better error handling and more formatting consistency.

daledupreezISC · 2017-09-20T19:41:50Z

WidgetsDirect/REST/Dispatch.cls

+	set tSC = $$$OK
+	set widgetAry = []
+
+	&SQL(DECLARE WidgetCursor CURSOR FOR 


Use more descriptive cursor names

Check SQLCODE for errors

Avoid reuse of Id in both cursor loops

Handle errors from calls to %OpenId()

daledupreezISC · 2017-09-20T19:45:16Z

WidgetsDirect/REST/Dispatch.cls

-	QUIT tSC
+	set retObj = {}
+
+	if '##class(User.Widget).%ExistsId(WidgetId) {


Cleanly report case where the requested widget doesn't exist

Cleanly handle cases where we fail to open a widget that does exist

daledupreezISC · 2017-09-20T19:51:34Z

WidgetsDirect/REST/Dispatch.cls

+
+	set widgetClass = "User.Widget"
+	set internalWidgetId = WidgetId
+	if ($extract(WidgetId,1) = "W") {


I have not changed this, but I think we should have a dedicated widgetType property on the returned data.

daledupreezISC · 2017-09-20T19:53:25Z

WidgetsDirect/REST/Dispatch.cls

-	SET retObj = {}
+	set retObj = {}
+
+	set widgetClass = "User.Widget"


Clean up widget class detection

Cleanly handle widgets that don't exist

Improve error handling for multiple issues

daledupreezISC · 2017-09-20T19:55:41Z

WidgetsDirect/REST/Dispatch.cls

+	set retObj = {}
+	set tSC = $$$OK
+
+	if '##class(User.Widget).%ExistsId(WidgetId) {


Cleaner handling of widgets that don't exist

Clean up formatting

Clean up error handling

daledupreezISC · 2017-09-20T19:56:23Z

WidgetsDirect/REST/Widget.cls

-    	  do widgetAry.%Push(widgetObj.toJSON(1))
-	    }
-    &SQL(CLOSE WidgetCurs)
+	set %response.ContentType = "application/json"


Not clear why these methods are present here AND in the other class.

There's an issue in InterSystems IRIS handling the map route to send all /widgets/... requests to the sub class. So they were moved back into the top Dispatch class.

@ocorrea7, might it make more sense to have shell methods rather than duplicating the actual code?

A shell method in one of the classes that calls the class method in the other? Yeah, that's probably cleaner. Then add a comment saying this is temporary to support the current version of IRIS?

@ocorrea7, I added the shell methods in the base class and an XML comment to indicate that some versions of InterSystems Cache/IRIS have a bug that prevent the Map from working as expected.

daledupreezISC · 2017-09-20T19:56:53Z

WidgetsDirect/UnitTest/Transaction.cls

@@ -6,32 +6,34 @@ Class UnitTest.Transaction Extends UnitTest.Abstract
 Method OnBeforeAllTests() As %Status
 {
 	tstart
-	if ..Manager.Debug set %debug=1
+	if ..Manager.Debug {


Code formatting

daledupreezISC · 2017-09-20T20:01:39Z

WidgetsDirect/User/WWWidget.cls

@@ -14,7 +14,7 @@ Property Barcode As %String;

 Property Location As %String;

-Method toJSON(traverseRelationships As %Boolean = 0) As %String
+Method toJSONObject(traverseRelationships As %Boolean = 0) As %Library.DynamicObject


Change method name and signature to capture what this method is actually doing.

I think calling this "toDynamicObject" (here and elsewhere) might be clearer.

daledupreezISC · 2017-09-20T20:06:35Z

WidgetsDirect/User/WWWidget.cls

@@ -29,27 +29,25 @@ Method toJSON(traverseRelationships As %Boolean = 0) As %String
 	quit jsonReturn
 }

-Method fromJSON(json As %String) As %String
+Method fromJSON(json As %String) As %Status


Fix method signature to indicate what we were returning

Use classmethod directly instead of {}.%FromJSON()

daledupreezISC · 2017-09-20T20:07:59Z

WidgetsDirect/User/Widget.cls

@@ -14,7 +14,7 @@ Property Deleted as %Boolean;

 Relationship Accessories As User.WidgetAccessoryLink [ Cardinality = many, Inverse = Widget ];

-Method toJSON(traverseRelationships As %Boolean = 0) As %String
+Method toJSONObject(traverseRelationships As %Boolean = 0) As %Library.DynamicObject


Fix method name/signature

Clean up code and make it clearer

daledupreezISC · 2017-09-20T20:08:42Z

WidgetsDirect/User/Widget.cls

 	}

 	quit jsonReturn
 }

-Method fromJSON(json As %String) As %String
+Method fromJSON(json As %String) As %Status


Fix method signature

Avoid classmethod call on instance

Clean up formatting

daledupreezISC · 2017-09-20T20:09:53Z

WidgetsDirect/User/WidgetAccessory.cls

@@ -20,7 +20,7 @@ Property SKU As %String;

 Relationship Widgets As User.WidgetAccessoryLink [ Cardinality = many, Inverse = Accessory ];

-Method toJSON(traverseRelationships As %Boolean = 0) As %String
+Method toJSONObject(traverseRelationships As %Boolean = 0) As %Library.DynamicObject


Change name/method signature

Code cleanup

daledupreezISC · 2017-09-20T20:10:14Z

WidgetsDirect/Util/Build.cls

@@ -2,7 +2,7 @@
 Class Util.Build {

 /// This is a build manifest. Here we describe build steps in XML format.
-XData BuildManifest
+XData BuildManifest [ XMLNamespace = INSTALLER ]


Minor code cleanup/formatting

daledupreezISC · 2017-09-20T20:11:16Z

I've added method-level comments as to what I did for each class.

timleavitt · 2017-09-20T20:10:37Z

WidgetsDirect/REST/Dispatch.cls

+		}
+		do widgetAry.%Push(widgetObj.toJSONObject(1))
+	}
+	if (SQLCODE && (SQLCODE '= 100)) {


I think if (SQLCODE < 0) would be clearer (here and elsewhere)

timleavitt · 2017-09-20T20:17:27Z

WidgetsDirect/User/WWWidget.cls

@@ -14,7 +14,7 @@ Property Barcode As %String;

 Property Location As %String;

-Method toJSON(traverseRelationships As %Boolean = 0) As %String
+Method toJSONObject(traverseRelationships As %Boolean = 0) As %Library.DynamicObject


I think calling this "toDynamicObject" (here and elsewhere) might be clearer.

timleavitt · 2017-09-20T20:18:46Z

WidgetsDirect/User/WWWidget.cls

@@ -29,27 +29,25 @@ Method toJSON(traverseRelationships As %Boolean = 0) As %String
 	quit jsonReturn
 }

-Method fromJSON(json As %String) As %String
+Method fromJSON(json As %String) As %Status


Perhaps highlight that json could also be a stream (because %FromJSON supports that - this would be handy in some of the REST APIs).

The current REST code is reading from the incoming content stream, so I've left that aspect of the code in place.

timleavitt · 2017-09-20T20:23:56Z

WidgetsDirect/REST/Dispatch.cls

@@ -4,184 +4,221 @@ Class REST.Dispatch Extends %CSP.REST

 XData UrlMap [ XMLNamespace = "http://www.widgetsdirect.com/urlmap" ]


This should probably be the standard UrlMap XML namespace, http://www.intersystems.com/urlmap (here and elsewhere) - with that in place Studio will suggest the right elements/attributes.

- Whitespace cleanup - Fix some typos

ocorrea7

Looks good. Thanks for cleaning things up

Major clean-up of code

deae506

- Rename toJSON() to toJSONObject() - Code formatting and consistency changes - Refactor population utilities - General code cleanup

daledupreezISC requested review from logist, ocorrea7, iscChris and isc-bspead September 20, 2017 19:38

daledupreezISC commented Sep 20, 2017

View reviewed changes

timleavitt reviewed Sep 20, 2017

View reviewed changes

daledupreezISC added 4 commits September 20, 2017 16:57

Fixes based on comments from Bill Sherman

98d8489

- Whitespace cleanup - Fix some typos

Use (SQLCODE < 0) for SQL checks

1444f6c

Rename toJSONObject() to toDynamicObject()

6559239

Fix XMLNamespace for URL maps

7acc18b

timleavitt approved these changes Sep 20, 2017

View reviewed changes

Refactor widget methods in REST.Dispatch to call APIs in REST.Widget

43d1ce9

ocorrea7 approved these changes Sep 21, 2017

View reviewed changes

iscChris approved these changes May 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major clean-up of code #1

Major clean-up of code #1

daledupreezISC commented Sep 20, 2017 •

edited

Loading

daledupreezISC Sep 20, 2017

daledupreezISC Sep 20, 2017 •

edited

Loading

daledupreezISC Sep 20, 2017

daledupreezISC Sep 20, 2017

daledupreezISC Sep 20, 2017

daledupreezISC Sep 20, 2017

daledupreezISC Sep 20, 2017

ocorrea7 Sep 20, 2017

daledupreezISC Sep 20, 2017

ocorrea7 Sep 20, 2017

daledupreezISC Sep 21, 2017

daledupreezISC Sep 20, 2017

daledupreezISC Sep 20, 2017

timleavitt Sep 20, 2017

daledupreezISC Sep 20, 2017

daledupreezISC Sep 20, 2017

daledupreezISC Sep 20, 2017

daledupreezISC Sep 20, 2017

daledupreezISC Sep 20, 2017

daledupreezISC Sep 20, 2017

daledupreezISC commented Sep 20, 2017

timleavitt Sep 20, 2017

daledupreezISC Sep 20, 2017

timleavitt Sep 20, 2017

timleavitt Sep 20, 2017

daledupreezISC Sep 20, 2017

timleavitt Sep 20, 2017

daledupreezISC Sep 20, 2017

ocorrea7 left a comment


		set tSC = $$$ADDSC(tSC,..CreateWidget("Waterproof Widget", 10.99, 10, "This widget is waterproof to a depth of 100m for up to 7 hours"))

		@@ -4,184 +4,221 @@ Class REST.Dispatch Extends %CSP.REST

		XData UrlMap [ XMLNamespace = "http://www.widgetsdirect.com/urlmap" ]

Major clean-up of code #1

Are you sure you want to change the base?

Major clean-up of code #1

Conversation

daledupreezISC commented Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

daledupreezISC Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daledupreezISC commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocorrea7 left a comment

Choose a reason for hiding this comment

daledupreezISC commented Sep 20, 2017 •

edited

Loading

daledupreezISC Sep 20, 2017 •

edited

Loading