web stats
Code Review .. - Mirth Community

Go Back   Mirth Community > Mirth Connect > Support

Reply
 
Thread Tools Display Modes
  #1  
Old 09-27-2018, 06:02 PM
gojoshi gojoshi is offline
OBX.1 Kenobi
 
Join Date: Jul 2013
Posts: 42
gojoshi is on a distinguished road
Question Code Review ..

Hello Everyone,

I have the following code in my source transformer. The objective is to capture the eID from pid4 if it is populated. The eID is sometimes populated, and sometimes not. The eID always starts with an "E" and could be populated in any repeating position of PID4.1, if populated ..

for (var i = 0; i < msg['PID']['PID.4'].length(); i++)
{
if (msg['PID']['PID.4'][i]['PID.4.1'].toString().substring(0,1) == "E")
{
var eID = msg['PID']['PID.4'][i]['PID.4.1'].toString();
$c('eID',eID);
}
}

I have the following code in my destination transformer.. The objective is to place the eID in PID3.1 if it was populated..

var eID = $('eID');
if(eID != null || eID != "")
{
msg['PID']['PID.3']['PID.3.1'] = eID;
}


For some reasons, the above code in the destination always gets executed, even in the eID is not populated in the message. The above code works fine it the eID is populated, but if the eID is NOT populated, it places a blank value in PID3.1.. The channel map on the dashboard shows a blank value as the eID when the eID is NOT populated..

Any reason why the code is getting executed when it should not? I just want it to execute when the eID is populated and NOT when it's not..

Any help will be greatly appreciated. Thanks!!
Reply With Quote
  #2  
Old 09-27-2018, 10:50 PM
siddharth siddharth is offline
Mirth Guru
 
Join Date: Feb 2013
Posts: 835
siddharth is on a distinguished road
Default

Any good reason you are iterating over the PID.4 field?

I would first check whether PID.4.1 exist (check-1), then whether it begins with E or not (check-2). After that I would fetch and store the EID in a channelMap variable. You might want to use charAt(0) to make the check or indexOf (charAt(0)).

if the whole objective is to pick EID from PID.4 and place it in PID.3, then you are doing it wrong by doing it in destination. You would have to do everything in the source.

if(check-1) {

if(check-2) {

msg['PID']['PID.3']['PID.3.1']=msg['PID']['PID.4']['PID.4.1'].toString();
}
}
__________________
HL7v2.7 Certified Control Specialist!
Reply With Quote
  #3  
Old 09-28-2018, 09:07 AM
agermano agermano is offline
Mirth Guru
 
Join Date: Apr 2017
Location: Indiana, USA
Posts: 1,005
agermano is on a distinguished road
Default

I'm guessing the iteration over PID-4 is because it could be repeating, and only the value which starts with E is desired. It's not technically wrong to pull the value in the source transformer, stick it in a channelMap variable, and use it in the destination. I don't really see any value in doing it that way, though. If you do it all in the source transformer, the replaced value will appear in your inbound message for all of your destinations. If you only want the replacement to happen for a particular destination, then do it there. If your channel only has one destination, then it's really your preference.

This should work placed in the source or destination transformer, and does not require a channelMap variable. It uses charAt as siddharth suggested. The for each loop accounts for repeating PID-4 and performs siddharth's check-1 at the same time.

Code:
for each (var pid4 in msg['PID']['PID.4']) {
    var pid4_1 = pid4['PID.4.1'].toString();
    if (pid4_1.charAt(0) == 'E') {
        msg['PID']['PID.3']['PID.3.1'] = pid4_1;
        break; // stop looping if we found one
    }
}
This will throw an error if PID-3 also happens to be repeating.

If you want to retain your original solution, try
Code:
if(eID != null && eID != "")
// or
if(eID != null && eID.length > 0)
// or
if(org.apache.commons.lang3.StringUtils.isNotEmpty(eID))
Reply With Quote
  #4  
Old 09-30-2018, 08:20 AM
J-Flynn J-Flynn is offline
Mirth Newb
 
Join Date: Sep 2018
Posts: 9
J-Flynn is on a distinguished road
Default

Quote:
Originally Posted by gojoshi View Post
var eID = $('eID');
if(eID != null || eID != "")
{
msg['PID']['PID.3']['PID.3.1'] = eID;
}


For some reasons, the above code in the destination always gets executed, even in the eID is not populated in the message.
Be sure to put code in code blocks. Your problem is very simple - you're using an || (or) instead of an and (&&).

Code:
var eID = $('eID');
if(eID != null && eID != "")
{
	msg['PID']['PID.3']['PID.3.1'] = eID;
}
A shorter/easier way to write a check for undefined/nullness is a double-bang:

Code:
var SomethingNull = null;

if(!!SomethingNull && SomethingNull != "")
{
//Null/undefined returns 'false'
}
JSFiddle example of double-bang behaviour on undefined/null:
https://jsfiddle.net/o1ufcj3d/1/

If your incoming data has a defined length or format, it's advisable to check against the intended format, as there may be rare cases where an incoming dataset might be non-null and non-blank (EG "AHJGHAJ"). If the length is variable, then work out the minimum length, so for example:

Code:
var SomeString = "182"; //ID should be 6 digits

if(!!SomeString && SomeString != "")
{
if(SomeString.length > 5)
{
//Do some stuff here
}
else
{
//Throw an error
logger.error("SomeString has a length less than 5");
}

}

Word of warning: an issue with Rhino on strings is there's two types: JavaScript strings and Java strings.

Java strings use .length()
JavaScript strings use .length

(Notice the brackets)

If you're not sure which type of string you'll be handling, you'll need to create a 'UniLength' function that tests for which type of length function it is (a similar issue exists between certain types of array using size and size()).

Here's my UniLength function (MIT):

Code:
function UniLength(TargetObject)
{
	if(TargetObject.length != undefined)
	{
		try
		{
			return (IsNumber(TargetObject.length())) ? TargetObject.length() : TargetObject.length;
		}
		catch(err)
		{
			return TargetObject.length;
		}
	}
	else if(TargetObject.size != undefined)
	{
		try
		{
			return (IsNumber(TargetObject.size())) ? TargetObject.size() : TargetObject.size;
		}
		catch(err)
		{
			return TargetObject.size;
		}
	}
	else
	{
		throw {name:"UniLength error",message:"UniLength: TargetObject does not have a .length or .size property"};
		return null;
	}
}
Be aware due to try-catch handling it incurs overhead, so if you know what specific type of string it is, use the normal length call.

Last edited by J-Flynn; 09-30-2018 at 08:27 AM. Reason: Added JSFiddle demo
Reply With Quote
  #5  
Old 10-01-2018, 09:40 AM
agermano agermano is offline
Mirth Guru
 
Join Date: Apr 2017
Location: Indiana, USA
Posts: 1,005
agermano is on a distinguished road
Default

I would lean toward the solution I posted, and siddharth hinted at to not split the code between two transformers.

As far as the reason why the proposed solution wasn't working, I did already point out the use of (or) instead of (and)

I made a mistake in my second suggestion using length due to the differences in string types like J-Flynn mentioned, but a UniLength function is not needed in this case. When you pull a string out of a java Map, it will always be converted to an instance of java.lang.String, regardless of which type of string you stuck in there.

The commons StringUtils function is the most readable way to check for empty strings that could be null.
Reply With Quote
  #6  
Old 10-03-2018, 03:10 AM
J-Flynn J-Flynn is offline
Mirth Newb
 
Join Date: Sep 2018
Posts: 9
J-Flynn is on a distinguished road
Default

Quote:
Originally Posted by agermano View Post
IWhen you pull a string out of a java Map, it will always be converted to an instance of java.lang.String, regardless of which type of string you stuck in there.
I find the map object (such as global maps) can hold any type of object, including custom JavaScript constructs (I've not encountered any sort of conversion from JavaScript string to Java string by placing text data in a global map). I also find that default implicit string conversion, such as:

Code:
var SomeInt = 22;
var Temp = ""+SomeInt;
Results in the string being a type of JavaScript string, such is the nature of Rhino (which behaves more like a hybrid between JavaScript and Java). There's other quirks within Rhino but this is the most pertinent (and the length issue was fairly difficult to trace in my case as the error message isn't exactly clear).

UniLength works if you can't guarantee the incoming type (such as in a code template function's arguments) as being a specific type of subset of a class (note UniLength works on any class with a length property, not merely strings).

Best way to guarantee a specific type of string is to convert it prior to operations. There's no way to explicitly convert to a JavaScript string (that I'm aware of), just implicitly via ""+. Java string is straight forward - class call with the arguments passed to the constructor.


My personal preference is JavaScript strings, simply because I don't have to import or declare the class type.

Quote:
The commons StringUtils function is the most readable way to check for empty strings that could be null.
Double-bang is the most portable (C++, JavaScript, Rhino, Java), and writing out != null and != undefined (or org.apache.commons.lang3.StringUtils.isNotEmpty) for multiple checks quickly becomes tedious (especially if you're doing clinical data integrity checks).

If you do feel the need to use StringUtils, it might be advisable to build a wrapper function (in perhaps a code template) like:

Code:
function IsStringValid(Incoming){ return org.apache.commons.lang3.StringUtils.isNotEmpty(Incoming); }
To shorten the write out. Importing an entire library for one function might be overkill for smaller channels.

Similarly, a wrapper function can be used to simplify the != undefined, != null and != "" checks. I'm still a fan of double-bang though.
Reply With Quote
  #7  
Old 10-03-2018, 11:20 AM
agermano agermano is offline
Mirth Guru
 
Join Date: Apr 2017
Location: Indiana, USA
Posts: 1,005
agermano is on a distinguished road
Default

You are correct that the maps can hold any type of object. They are instances of java.util.HashMap<String, Object>. But, in Rhino, any string, whether java or javascript, when stored in a Map (or any Collection) is always retrieved as a java.lang.String.

You are also correct that string concatenation in Rhino always results in a javascript string. This is even true when concatenating two java.lang.Strings. Any non-string object being concatenated (javascript or java) will have its toString() method called prior to concatenation. Calling the javascript String() function (without new) is how to explicitly convert to a javascript string. new String() will actually returned a string wrapped in an object instead of the plain string.

I will concede that the double-bang is both portable and concise, but I don't think the intent is clear for those looking at the channel that are not programmers. You also have the issue that the numbers 0 and NaN and the empty string will all return false, which may or may not be desired if you only mean to check for not undefined and not null.

You can create a reference to the JavaClass in Rhino that acts similar to a Java import without needing to import the entire package or create a wrapper function. StringUtils will throw an exception if passed undefined as a value, but there is likely a logical error in your code if undefined values are treated the same way as true values or null (indeed, your UniLength function would also throw an error.)

I placed the below code into the deploy script of a brand new channel that doesn't do anything else to illustrate. I think we are sufficiently off-topic from the original post now, but hopefully someone will find this back-and-forth useful

Code:
var s = 'test';
logger.info('typeof before map: ' + (typeof s));
logger.info('instance of java string before map: ' + (s instanceof java.lang.String));
$gc('s',s);
s = $gc('s');
logger.info('typeof after map: ' + (typeof s));
logger.info('instance of java string after map: ' + (s instanceof java.lang.String));
s = String(s);
logger.info('typeof after explicit conversion: ' + (typeof s));
logger.info('instance of java string after explicit conversion: ' + (s instanceof java.lang.String));


var StringUtils = org.apache.commons.lang3.StringUtils;
logger.info('isNotEmpty (null): ' + StringUtils.isNotEmpty(null));
logger.info("isNotEmpty (''): " + StringUtils.isNotEmpty(''));
logger.info("isNotEmpty ('asdf'): " + StringUtils.isNotEmpty('asfd'));


var js = new java.lang.String('a');
logger.info('typeof java string: ' + (typeof js));
logger.info('typeof concatenated java strings: ' + (typeof (js + js)));
Reply With Quote
Reply

Tags
code, empty, javascript, null

Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off

Forum Jump


All times are GMT -8. The time now is 10:10 AM.


Powered by vBulletin® Version 3.8.7
Copyright ©2000 - 2019, vBulletin Solutions, Inc.
Mirth Corporation