Skip to main content
Code Review

Return to Answer

fix "didactic" typo
Source Link
J_H
J_H
  • 30.8k
  • 3
  • 30
  • 119

backwhacks

SELECT ... '{"dir": "c:\\"}'

If your use case permits it, prefer c:/ forward slash. It avoids backwhacked quoting nonsense, where each level of evaluation cuts the number of slashes in half.

Also, for did didactic purposes, consider replacing this/path/not/exist with a simple some/path. As near as I can tell, the existence / non-existence of the path seems to be immaterial to the result produced, and to the teaching value of the example.

magic number

... , @PATH NVARCHAR(255), ...

I imagine there's a good reason for that number. Maybe it's part of a spec, or we want to avoid 16-bit lengths. Recommend you spell out the details, in case some hapless maintenance engineer revisits this code a year or two down the road.

I am reading the corresponding JSON_OBJECT function:

Description:

This function is used to add or update a text value at the specified JSON path within a JSON object. This function is more versatile, where it is possible to enter a Json Object, or Text, value. It also has support for including items in arrays.

In another context we might call this javadoc or a docstring. (Sorry, I don't know the proper name for it in a t-sql context.)

I truly do thank you for offering this helpful advice.

But what I'm really looking for is a promise, a description of what hoops caller must jump through, and what lovely result caller can rely on receiving. These sentences hint at that, without being explicit.

I appreciate the wonderfully explicit precondition we find here:

    -- VALIDATIONS
    IF ISJSON(@VALUE) <= 0 ...

I am slightly surprised that we're contemplating negative return values, given that the function spec says this boolean predicate shall

Returns 1 if the string contains valid JSON; otherwise, returns 0. Returns null if expression is null.

type stability

Or we might refer to this as meaning stability.

We pass in @JSON, which means one thing. And then we mutate and return @JSON, having an entirely different meaning. (It turns out that, due to deeply nested ELSE clauses, once we decide on a new value we never reconsider @JSON again.)

Recommend you invent a new variable, perhaps @RET, for the returned value.

I do thank you for helpful comments such as these:

       --RETURN 'PROPERTY'
       ...
          --RETURN 'OBJECT OR ARRAY'

extract helper

This function is starting to get Too Long, as one cannot read all of it without vertical scrolling. Consider breaking out the 'PATH NOT EXISTS' logic as a private helper function. That would give you an opportunity to offer a separate explanation about inputs and outputs for the helper.

Recommend you elide "obvious" comments such as -- UPDATE CURRENT.

nit, typo: EXTISTS

backwhacks

SELECT ... '{"dir": "c:\\"}'

If your use case permits it, prefer c:/ forward slash. It avoids backwhacked quoting nonsense, where each level of evaluation cuts the number of slashes in half.

Also, for did didactic purposes, consider replacing this/path/not/exist with a simple some/path. As near as I can tell, the existence / non-existence of the path seems to be immaterial to the result produced, and to the teaching value of the example.

magic number

... , @PATH NVARCHAR(255), ...

I imagine there's a good reason for that number. Maybe it's part of a spec, or we want to avoid 16-bit lengths. Recommend you spell out the details, in case some hapless maintenance engineer revisits this code a year or two down the road.

I am reading the corresponding JSON_OBJECT function:

Description:

This function is used to add or update a text value at the specified JSON path within a JSON object. This function is more versatile, where it is possible to enter a Json Object, or Text, value. It also has support for including items in arrays.

In another context we might call this javadoc or a docstring. (Sorry, I don't know the proper name for it in a t-sql context.)

I truly do thank you for offering this helpful advice.

But what I'm really looking for is a promise, a description of what hoops caller must jump through, and what lovely result caller can rely on receiving. These sentences hint at that, without being explicit.

I appreciate the wonderfully explicit precondition we find here:

    -- VALIDATIONS
    IF ISJSON(@VALUE) <= 0 ...

I am slightly surprised that we're contemplating negative return values, given that the function spec says this boolean predicate shall

Returns 1 if the string contains valid JSON; otherwise, returns 0. Returns null if expression is null.

type stability

Or we might refer to this as meaning stability.

We pass in @JSON, which means one thing. And then we mutate and return @JSON, having an entirely different meaning. (It turns out that, due to deeply nested ELSE clauses, once we decide on a new value we never reconsider @JSON again.)

Recommend you invent a new variable, perhaps @RET, for the returned value.

I do thank you for helpful comments such as these:

       --RETURN 'PROPERTY'
       ...
          --RETURN 'OBJECT OR ARRAY'

extract helper

This function is starting to get Too Long, as one cannot read all of it without vertical scrolling. Consider breaking out the 'PATH NOT EXISTS' logic as a private helper function. That would give you an opportunity to offer a separate explanation about inputs and outputs for the helper.

Recommend you elide "obvious" comments such as -- UPDATE CURRENT.

nit, typo: EXTISTS

backwhacks

SELECT ... '{"dir": "c:\\"}'

If your use case permits it, prefer c:/ forward slash. It avoids backwhacked quoting nonsense, where each level of evaluation cuts the number of slashes in half.

Also, for didactic purposes, consider replacing this/path/not/exist with a simple some/path. As near as I can tell, the existence / non-existence of the path seems to be immaterial to the result produced, and to the teaching value of the example.

magic number

... , @PATH NVARCHAR(255), ...

I imagine there's a good reason for that number. Maybe it's part of a spec, or we want to avoid 16-bit lengths. Recommend you spell out the details, in case some hapless maintenance engineer revisits this code a year or two down the road.

I am reading the corresponding JSON_OBJECT function:

Description:

This function is used to add or update a text value at the specified JSON path within a JSON object. This function is more versatile, where it is possible to enter a Json Object, or Text, value. It also has support for including items in arrays.

In another context we might call this javadoc or a docstring. (Sorry, I don't know the proper name for it in a t-sql context.)

I truly do thank you for offering this helpful advice.

But what I'm really looking for is a promise, a description of what hoops caller must jump through, and what lovely result caller can rely on receiving. These sentences hint at that, without being explicit.

I appreciate the wonderfully explicit precondition we find here:

    -- VALIDATIONS
    IF ISJSON(@VALUE) <= 0 ...

I am slightly surprised that we're contemplating negative return values, given that the function spec says this boolean predicate shall

Returns 1 if the string contains valid JSON; otherwise, returns 0. Returns null if expression is null.

type stability

Or we might refer to this as meaning stability.

We pass in @JSON, which means one thing. And then we mutate and return @JSON, having an entirely different meaning. (It turns out that, due to deeply nested ELSE clauses, once we decide on a new value we never reconsider @JSON again.)

Recommend you invent a new variable, perhaps @RET, for the returned value.

I do thank you for helpful comments such as these:

       --RETURN 'PROPERTY'
       ...
          --RETURN 'OBJECT OR ARRAY'

extract helper

This function is starting to get Too Long, as one cannot read all of it without vertical scrolling. Consider breaking out the 'PATH NOT EXISTS' logic as a private helper function. That would give you an opportunity to offer a separate explanation about inputs and outputs for the helper.

Recommend you elide "obvious" comments such as -- UPDATE CURRENT.

nit, typo: EXTISTS

Source Link
J_H
J_H
  • 30.8k
  • 3
  • 30
  • 119

backwhacks

SELECT ... '{"dir": "c:\\"}'

If your use case permits it, prefer c:/ forward slash. It avoids backwhacked quoting nonsense, where each level of evaluation cuts the number of slashes in half.

Also, for did didactic purposes, consider replacing this/path/not/exist with a simple some/path. As near as I can tell, the existence / non-existence of the path seems to be immaterial to the result produced, and to the teaching value of the example.

magic number

... , @PATH NVARCHAR(255), ...

I imagine there's a good reason for that number. Maybe it's part of a spec, or we want to avoid 16-bit lengths. Recommend you spell out the details, in case some hapless maintenance engineer revisits this code a year or two down the road.

I am reading the corresponding JSON_OBJECT function:

Description:

This function is used to add or update a text value at the specified JSON path within a JSON object. This function is more versatile, where it is possible to enter a Json Object, or Text, value. It also has support for including items in arrays.

In another context we might call this javadoc or a docstring. (Sorry, I don't know the proper name for it in a t-sql context.)

I truly do thank you for offering this helpful advice.

But what I'm really looking for is a promise, a description of what hoops caller must jump through, and what lovely result caller can rely on receiving. These sentences hint at that, without being explicit.

I appreciate the wonderfully explicit precondition we find here:

    -- VALIDATIONS
    IF ISJSON(@VALUE) <= 0 ...

I am slightly surprised that we're contemplating negative return values, given that the function spec says this boolean predicate shall

Returns 1 if the string contains valid JSON; otherwise, returns 0. Returns null if expression is null.

type stability

Or we might refer to this as meaning stability.

We pass in @JSON, which means one thing. And then we mutate and return @JSON, having an entirely different meaning. (It turns out that, due to deeply nested ELSE clauses, once we decide on a new value we never reconsider @JSON again.)

Recommend you invent a new variable, perhaps @RET, for the returned value.

I do thank you for helpful comments such as these:

       --RETURN 'PROPERTY'
       ...
          --RETURN 'OBJECT OR ARRAY'

extract helper

This function is starting to get Too Long, as one cannot read all of it without vertical scrolling. Consider breaking out the 'PATH NOT EXISTS' logic as a private helper function. That would give you an opportunity to offer a separate explanation about inputs and outputs for the helper.

Recommend you elide "obvious" comments such as -- UPDATE CURRENT.

nit, typo: EXTISTS