about criteriaFind

[Bigwood Liu]

hi,
My criteriaFind is as following:

public synchronized DataInfo[] criteriaFind(String criteria) {
    1.parse the criteria with "," as delimiter
    2.add parsed strings into a crieraiList
    3.iterate the crieraiList, parse the parsed strings using substring(), find out the index of each fieldName: 
    if {fieldName exist) {
         add its index to a indexList;
         add its value to a valueList; 
         //thus they have the same index in the list.
    } else {
         didn't find;
    }
    4.go through the db, use the elements in  indexList and valueList to compare the record value with criteria value.
}

Regards,
Damu
[ June 07, 2003: Message edited by: damu liu ]

[Andrew Monkhouse]

Hi Damu,
Sorry, you have to code in Java - psuedocode is not allowed
Seriously though ....
It appears that if a column name is not found, then the bad column name is ignored, but the search continues with the remaining column names that are valid.
Do you have the instruction:

In the event of an invalid field name being provided as part of the criteria the behavior of this method is the same as if no records matched correctly specified criteria.

If so, I think you may end up with records that appear to match the invalid criteria.
Otherwise it appears that this will work.
Just some things you might want to think about:
Have you considered using a Map instead of your two lists?
Have you considered using StringTokenizer instead of the substring()/indexof() methods? Or the regular expression package (I dont recommend it for this assignment, but you might want to consider it).
Regards, Andrew

[Bigwood Liu]

Hi,
thank you for reply Andrew,

Have you considered using a Map instead of your two lists?

I had thought to put the indexes and the values into a map, but when I go through all the records and do the comparing, I don't know how to take them out from the map one by one without knowing what is in the map.

Have you considered using StringTokenizer instead of the substring()/indexof()

I had read a lot of articles here about using StringTokenizer, but I think substring()/indexof() can parse the string easly, while StringTokenizer have to deal with diffrent delimiter:"=","'".
My code is as following:

 public synchronized DataInfo[] criteriaFind(String criteria) {
invariant();
ArrayList criteriaList = new ArrayList();
Iterator iterator = null;
ArrayList fieldNameIndexes = new ArrayList();
ArrayList criteriaValues = new ArrayList();
StringTokenizer tokenizer = new StringTokenizer(criteria, ",");
List fieldNameList = new ArrayList();
int fieldNameIndex = 0;
String criteriaFieldName = null;
String criteriaValue = null;
String [] values = null;
String temp = null;
boolean found = false;
DataInfo rv = null;
DataInfo[] rvs = null;
while (tokenizer.hasMoreTokens()) {
    criteriaList.add(tokenizer.nextToken().trim());
 }
iterator = criteriaList.iterator();
for (int i = 0; i < description.length; i++) {
    fieldNameList.add(description[i].getName());
}
while (iterator.hasNext()) {
    temp = (String) iterator.next();
    criteriaFieldName = temp.substring(0, temp.indexOf("=")).trim();
    fieldNameIndex = fieldNameList.indexOf(criteriaFieldName);
    criteriaValue = temp.substring((temp.indexOf("'")+1), temp.lastIndexOf("'")).trim();
    if (fieldNameIndex != -1) {
      fieldNameIndexes.add(new Integer(fieldNameIndex));
criteriaValues.add(criteriaValue);
    } else {
return null;
    }
}
criteriaList.clear();
try {
    seek(1);
    
    for (int r = 1; r <= recordCount; r++) {
    values = readRecord();
for (int j = 0; (values != null) && (j <  fieldNameIndexes.size()); j++) {
    fieldNameIndex = ((Integer) fieldNameIndexes.get(j)).intValue();
    if (((String) criteriaValues.get(j)).equals(values[fieldNameIndex].trim())) {
found = true;
    } else {
found = false;
break;
    }
}
if (found == true) {
    rv = new DataInfo(r, description, values);
    criteriaList.add(rv);
}
    }
}  catch (IOException e) {
       //     throw new DatabaseException(UNEXPECTED + e);
        }
rvs = new DataInfo[criteriaList.size()];
for (int i = 0; i < rvs.length; i++) {
    rvs[i] = (DataInfo) criteriaList.get(i);
}
 System.out.println("ok"+rvs.length);
return (rvs.length != 0) ? rvs : null;   

    } 

Regards,
Damu

[Bigwood Liu]

hi Andrew,
Do you think this method is to big, and I should divide it into several little methods?
Regards,
Damu

[Andrew Monkhouse]

Hi Damu,

I had thought to put the indexes and the values into a map, but when I go through all the records and do the comparing, I don't know how to take them out from the map one by one without knowing what is in the map.

It is fairly easy, you create an iterator over either the key or the value, and extract the other item:

Map values = new HashMap();
values.add(key, value);
...
Iterator i = values.keySet().iterator();
while (i.hasNext()) {
  String key = (String) i.next();
  String value = (String) values.get(key)
  ...
}

It could be argued that the extra work is not worth the effort. Your decision.
I had a separate method which converted the string supplied by the user into a map which was used by the criteriaFind method. So in my case having the data in a single map which was returned by the conversion method made my code easier to use.
I am not trying to convince you of one way over the other - your way seems fine to me - I am just giving you things to think about
And now for some more things to think about (to stop you from getting bored )
You have catered for the criteria being enclosed in quotation marks, but you have not catered for the column name being enclosed in quotation marks.
I think your quotation handling could cause problems if the column name contains a quotation mark (e.g. Bill'sAirline) but the entire string is not enclosed in quotation marks - does this concern you?
I think if record 'x' matches the criteria, and record 'x+1' is deleted, then the deleted record will be added to your criteriaList ArrayList.
I am not sure whether you should be reusing CriteriaList for two different purposes - I think it would be better (although slightly wasteful) to have a separate ArrayList for this function.
Think about the scope of your variables - I think you could reduce scope on a lot of them.
Why aren't you using the toArray() method instead of

rvs = new DataInfo[criteriaList.size()];
for (int i = 0; i < rvs.length; i++) {
    rvs[i] = (DataInfo) criteriaList.get(i);
}

Do you think this method is to big, and I should divide it into several little methods?

I dont think it is too big in terms of number of lines, however I think that clarity might be improved by breaking it into functional areas. At the moment, your documentation might read "converts user criteria from string format to lists, AND then finds matching records." Anywhere that an AND appears in documentation is a possible indication that you are violating the One responsibility rule. But this is subjective - you have to decide whether refactoring will make it more confusing.
Regards, Andrew
[ June 07, 2003: Message edited by: Andrew Monkhouse ]

[Bigwood Liu]

Hi Andrew,
Thank you very much.
Your answer is great valuable to me. I will rethink this method.
Best Regards,
Damu
[ June 08, 2003: Message edited by: damu liu ]

[Bigwood Liu]

when I do toArray():

 
rvs =  (DataInfo[]) criteriaList.toArray(); 

I get a runtime ClassCastException .
in addition, I add a row of code as this to avoid adding a 'x+1' deleted record into the criteriaList:

 if (found == true) {
    rv = new DataInfo(r, description, values);
    criteriaList.add(rv);
    found == false;//newly added
}
 

[ June 08, 2003: Message edited by: damu liu ]
[ June 08, 2003: Message edited by: damu liu ]

[Mark Spritzler]

while (tokenizer.hasMoreTokens()) { criteriaList.add(tokenizer.nextToken().trim()); }
To me that looks like you are just putting the Strings into the ArrayList. Whereas, you are trying to cast it to a DataInfo[]. String[] <> DataInfo[].
I hope that is it.
I would argue that your method is too big, it can be much more readible and manageable if it is broken into multiple methods, each with the one responsibility rule.
Mark

[Andrew Monkhouse]

Sorry Mark, no prizes for you today You got caught by the fact that Damu is using the criteriaList variable for two different functions, holding two different types of objects.
Damu: this is why I recommend that you do not reuse variables in this manner ... it is too easy for someone reading your code to be confused.
The following code worked for me:

        rvs =  (DataInfo[]) criteriaList.toArray(new DataInfo[0]);

Sorry I cannot currently remember why the normal toArray() without any parameters does not work, but specifying the type of data as a parameter does work.
Regards, Andrew

[Bigwood Liu]

Hi ,
Thank you both.
I am trying to divide this method to make it more readable.

it is too easy for someone reading your code to be confused.

You are right, Andrew! I already made Mark confused .I will correct it.

  rvs =  (DataInfo[]) criteriaList.toArray(new DataInfo[0]);

this does work!
Thank you once more, Andrew
Regards,
Damu

[Bigwood Liu]

Hi,
this is my new version criteriaFind:

  public synchronized DataInfo[] criteriaFind(String criteria) {
invariant();
ArrayList dataInfoList = new ArrayList();
Map criteriaMap = new HashMap();
Iterator iterator = null;
String [] values = null;
int fieldNameIndex = 0;
boolean found = false;
DataInfo rv = null;
DataInfo[] rvs = null;
criteriaMap = CriteriaParser.parse(criteria, description);
try {
    seek(1);
    
    for (int r = 1; r <= recordCount; r++) {
    values = readRecord();
iterator = criteriaMap.keySet().iterator();
while((values != null) && iterator.hasNext()) {
    fieldNameIndex = ((Integer) iterator.next()).intValue();
    
    if (((String) criteriaMap.get(iterator.next())).equals(values[fieldNameIndex].trim())) {
found = true;
    } else {
found = false;
break;
    }
}
if (found == true) {
    rv = new DataInfo(r, description, values);
    dataInfoList.add(rv);
    found = false;
}
    }
}  catch (IOException e) {
       //     throw new DatabaseException(UNEXPECTED + e);
        }
rvs =  (DataInfo[]) dataInfoList.toArray(new DataInfo[0]);
return (rvs.length != 0) ? rvs : null;   

    } 

[Andrew Monkhouse]

Hi Damu
You call iterator.next() twice in this bit of code:

while((values != null) && iterator.hasNext()) {
    fieldNameIndex = ((Integer) iterator.next()).intValue();
   
    if (((String) criteriaMap.get(iterator.next())).equals(values[fieldNameIndex].trim())) {
found = true;
    } else {
found = false;
break;
    }
}

This is almost certainly wrong.
Something else to think about ... do you prefer this line of code:

    if (((String) criteriaMap.get(iterator.next())).equals(values[fieldNameIndex].trim()))

or these two lines:

    Object desiredValue = criteriaMap.get(iterator.next());
    if (values[fieldNameIndex].trim().equals(desiredValue))

It can certainly be argued either way - it is up to you how you want to code this.
I didnt bother casting the desiredValue back into a String - I left it to the String equals() method to handle that. Once again, you could argue for or against the explicit cast.
I presume you are going to do something with that Exception handling routine?
Regards, Andrew

[S. Ganapathy]

I really suggest Damu not to use so huge code in methods. Try to refracter your code damu.
I too implemented search functionality. Please review my code too.
Delimiter I used is either ',' OR ', '
Usage: name='someName', location='someLocation', etc..

    private String[] parseCriteria(String toParse) throws PatternSyntaxException {
        String regex = "(\\w+(?: ?\\w+)?)='([^']+)'[, ]*";
        Pattern pattern = Pattern.compile(regex);
        Matcher matcher = pattern.matcher(toParse);
        Map map = new HashMap(6);
        while (matcher.find()) {
            if (matcher.groupCount() == 2) {
                map.put(matcher.group(1).toLowerCase(),
                        matcher.group(2).toLowerCase());
            }
        }
        String[] data = new String[6];
        //did some stuff here which converts search query to data array
        return data;
    }
    public long[] findByCriteria(String[] criteria) {
        long[] activeRecordList = null;
        List returnList = new ArrayList(10);
        try {
            activeRecordList = getActiveRecords();
        } catch (IOException ioe) {
        }
        String[] data = null;
        for (int i = 0; i < activeRecordList.length; i++) {
            try {
                data = readRecord(activeRecordList[i]);
                if (match(data, criteria)) {
                    returnList.add(new Long(activeRecordList[i]));
                }
            } catch (RecordNotFoundException rnfex) {
            }
        }
        Iterator iter = returnList.iterator();
        long[] out = new long[returnList.size()];
        int i = 0;
        while (iter.hasNext()) {
            out[i++] = ((Long) iter.next()).longValue();
        }
        return out;
    }
    private boolean match(String[] data, String[] criteria) {
        boolean found = true;
        //ignore case sensitiveness while searching
        for (int i = 0; i < data.length; i++) {
            if ((data[i].toLowerCase()).startsWith(criteria[i].toLowerCase())) {
                found &= true;
            } else {
                found &= false;
            }
        }
        return found;
    }

Please note that the above code is not final one. This only gives an idea of search functionality
Please review my code too and let me know your comments.
Thanks in advance.
Regards,
Ganapathy.
[ June 08, 2003: Message edited by: S. Ganapathy ]

[Bigwood Liu]

Hi Andrew,
thank you!
Is this ok?

 while((values != null) && iterator.hasNext()) {
    Object key = iterator.next();
    fieldNameIndex = ((Integer) key).intValue();
    
    if (((String) criteriaMap.get(key)).equals(values[fieldNameIndex].trim())) {
found = true;
    } else {
found = false;
break;
    }
}
 

[Andrew Monkhouse]

Hi Ganapathy,
Wow - are you trying to make me engage my brain by using regex?
I assume that where you put the comment:

        //did some stuff here which converts search query to data array

You are converting column names to column indexes. Is this correct?
Assuming the above, your regex allows the column name to have up to two words seperated by spaces. This will fail if the column names have three words. Could this be a potential problem? (I dont have your assignment so I dont know)
Your regex will fail if either the column name or the criteria to match contains a single quote (e.g. "O'briensContractors") - does this concern you?
Some general comments ... I strongly recommend against swallowing exceptions like this:

        } catch (IOException ioe) {
        }

If you want to ignore the exception while developing and come back to it later, then put a comment in there to remind yourself - preferably using a notation that makes it easy to find later. Personally I do this by using my initials in the comment (and I dont use them elsewhere), so I can always search for the magic string ADM to see where I have something left to fix:

        } catch (IOException ioe) {
            // ADM handle exception
        }

This also helps when you put code up for review - the reviewer then knows what your intention is.
Your findByCriteria method works on an array of activeRecords. However there is nothing to stop additional records being added, or existing records being deleted while you are inside this method. Is this OK for you? Are you commenting this?
As with my comment to Damu - why are you iterating through the returnList to put the items it contains into an array, when the toArray() method will do this for you?
In you match() method - nice use of &= (last time I did that, my team spent 15 minutes reviewing 200 lines of code, and 45 minutes reviewing my use of &= ) but I wonder if it is worthwhile? If you are willing to break the concept of having only one "return" statement in a method, then you can return false the moment that you know that the data does not match the criteria, and return true if you get through the loop otherwise. With such a short block of code and appropriate comments, you can get away with this. However if you wish to stay "pure" then this is fine.
Regards, Andrew

[Andrew Monkhouse]

Hi Damu,
Your modification looks fine.
Regards, Andrew

[S. Ganapathy]

Hi Andrew,
I am not swallowing exceptions. I need to handle them. I will provide comments as you suggests, and later I will handle them.
I have not tested regular expression properly. I will update the expression, if I am able to makeout one, or else I use StringTokenizer to split the name value pairs. Thanks for pointingout the parseing bug.
I am converting column names to column indexes. You are right.

Your findByCriteria method works on an array of activeRecords. However there is nothing to stop additional records being added, or existing records being deleted while you are inside this method. Is this OK for you? Are you commenting this?

I am making suitable assumptions like there are no new records are added while searching. But if the records are deleted, after collecting list of active records, while reading the record with record number, it will throw RNF exception. Only adding records is a problem but I am writing a note in choices.txt.
So, is this match() OK?
There is no much performance variation because array has only 6 elements, and readability is good in previous case, as there is only one return statement.

    private boolean match(String[] data, String[] criteria) {
        boolean found = true;
        //ignore case sensitiveness while searching
        for (int i = 0; i < data.length; i++) {
            if ((data[i].toLowerCase()).startsWith(criteria[i].toLowerCase())) {
                //found &= true;
            } else {
                //found &= false;
                return false;
            }
        }
        //return found;
        return true;
    }

I am sure, I have to look at parseCriteria() seriously.

As with my comment to Damu - why are you iterating through the returnList to put the items it contains into an array, when the toArray() method will do this for you?

I can only add Object types in Collection objects.

List list = new ArrayList(10);
for (int i = 0; i < 10; i++) {
    list.add(new Integer(i));
}
//this will not work.
int[] intArr = (int[]) list.toArray(new int[0]);
//this will work.
Integer[] intArray = (Integer[]) list.toArray(new Integer[0]);

That is the reason, I am using Iterator.
Your comments are most welcome.
Regards,
Ganapathy.

[Andrew Monkhouse]

Hi Ganapathy

But if the records are deleted, after collecting list of active records, while reading the record with record number, it will throw RNF exception.

OK - I didnt see that in your code.
I wonder if throwing an exception is a good thing or not? I think this is one case where you could just handle the deleted record as you go along.
Your new match looks OK to me. The old one did too, I just like to play devil's advocate As for the performance, it could make a difference if you had to itterate over 1000 records multiplied by all 6 elements, compared to the new code which only has to go over the elements until the point where it knows whether the record matches or not. However perfomance is not an issue in this assignment, so I wouldnt let that worry you.
I missed that you were converting from an array of Long to an array of long. Ooops, my bad.
Regards, Andrew

[S. Ganapathy]

Hi Andrew,
I have to swallow RNF Exception, while collecting elements. So I have to be very careful to handle and swallow exceptions. This is a big exercis to me.
Thanks once again Andrew for your express replies.
Regards,
Ganapathy.

[Vitaly Zhuravlyov]

Hi Andrew and Ganapathy,

Originally posted by Andrew Monkhouse:
In you match() method - nice use of &= (last time I did that, my team spent 15 minutes reviewing 200 lines of code, and 45 minutes reviewing my use of &= ) but I wonder if it is worthwhile?

It certainly is not worthwhile

found &= true  

is always found and

found &= false 

is always false
Vitaly

[Andrew Monkhouse]

Hi Vitaly
Ooops, I was busy being impressed that someone else used &= and ignored the fact that it was operating on a boolean. Thanks for spotting it and bringing it to our attention.
Just had to go back and check the code that I wrote a while back that also used bitwise assignment operators. Fortuanately my usage was valid however it was written in a C program, so I wont post it here.
Regards, Andrew

[Vitaly Zhuravlyov]

You are welcome Andrew!
It's not often that people reply to my messages so promptly Because of the time zones
Vitaly

[S. Ganapathy]

Hi Andrew,
I again started concentrating on criteriaFind. I modified my regular expression. Please go through te regex once again.
Usage: name="someName", location="someLocation", etc..

    private String[] parseCriteria(String toParse) throws PatternSyntaxException {
        String regex = "(\\w+(?: ?\\w+)?)=\"([^\"]+)\"[, ]*";
        Pattern pattern = Pattern.compile(regex);
        Matcher matcher = pattern.matcher(toParse);
        Map map = new HashMap(6);
        while (matcher.find()) {
            if (matcher.groupCount() == 2) {
                map.put(matcher.group(1).toLowerCase(),
                        matcher.group(2).toLowerCase());
            }
        }
        String[] data = new String[6];
        //process data
        return data;
    }

I am assuming that the double quote never occur in the value of name-value pair. I am documenting this.
I am not interested to use StringTokenizer, as I need to use twice go split name-value pairs. Regular expression will make the code simple.
Please give your openions and comments.
Regards,
Ganapathy.

[Andrew Monkhouse]

Hi Ganapathy,
The only difference is that you are now using double quotes instead of single quotes. This solves the issue I raised: Your regex will fail if either the column name or the criteria to match contains a single quote (e.g. "O'briensContractors") - does this concern you?
So, this should be fine. I assume you are also documenting the other issue I raised.
By the way, do you have a requirement that the value to be searched for should be between quotation marks?

Regular expression will make the code simple.

I like regular expressions, and use them a lot. And I like the fact that some people here are using them in the assignment. But simple? I would argue that a junior programmer could read and understand larger code that uses StringTokenizer faster than they could read and understand regex. I think the same statment could even be applied to most non junior programmers. But keep using regex if you are comfortable with it.

Regards, Andrew

[S. Ganapathy]

Hi Andrew,

The only difference is that you are now using double quotes instead of single quotes. This solves the issue I raised: Your regex will fail if either the column name or the criteria to match contains a single quote (e.g. "O'briensContractors") - does this concern you?

I tried using single quotes, and it did not fail. Earlier, I forgot to consider using single quote. Now it is fine. e.g. name="Monster's Website" will work fine.
I tried StringTokenizer, and String.split(regex), and I found using regular expression is good.
Ofcourse, for junior programmers, regular expression is difficult. But they need not understand regular expression extensively. This will be documented in the method. So there won't be any problem.
Regards,
Ganapathy.

[Eugene Sun]

Hi all,
A couple of questions here about FBN,
1. Has anyone used StringTokenizer to parse the criteria String instead of the java.util.regx package and passed the exam without losing points on criteriaFind method implementation ?
2. I have both the "Origin City" and "Destination City" as input TextFields, has anyone implemented them this way and passed without being penalized, or do I have to implement a ComboBox with a selection of cities for these two input fields ?

Thanks,
Eugene

[Andrew Monkhouse]

Hi Eugene

1. Has anyone used StringTokenizer to parse the criteria String instead of the java.util.regx package and passed the exam without losing points on criteriaFind method implementation ?

Up until recently it was unclear whether JDK 1.4 could even be used for Fly By Night Services. Sun eventually declared that it was allowable though. It has only been since then that people have even started looking at regex.
Which is a long way of saying that there are lots of people who have passed using StringTokenizer.

2. I have both the "Origin City" and "Destination City" as input TextFields, has anyone implemented them this way and passed without being penalized, or do I have to implement a ComboBox with a selection of cities for these two input fields ?

I don't know if anyone has passed that way.
I do know that at least one person failed, and received the comment from the examiner that they had not used JComboBoxes (caused quite a bit of concern at the time, and I think that person later passed on appeal).
Personally I only used JComboBoxes and got 100% for my GUI, so I guess they liked it
Regards, Andrew