criteriaFind review please...

[Rajesh So]

Hi,
I have written criteriaFind and it is working for my test conditions. I took a lot of inputs from this thread.
https://coderanch.com/t/182827/java-developer-SCJD/certification/please-review-criteriaFind-good-riddance
But the principle adopted is slightly different.So please see if it is eccentric and/or would reduce marks.
What is different are three things:
1) Parsing using 3 values = ' ,
2) Not using the word 'ANY' , but the implementation implicitly uses this.
3) I have added composed methods :
In Data.java,
boolean isdataFieldPresent(String key) = Checks if the field name is valid
boolean isLive() = Checks if the record is Live
List getAllRecordNumbers() = Gets all the Live Record numbers as Integers in a List.
In DataInfo.java,
String getValue(String fieldName) = Gets the value for a particular field Name

A small code snippet for parse is

        // Split the given data into index and value
Map resultMap = new HashMap(description.length);
for (StringTokenizer criteriaAsToken = new StringTokenizer(criteria); criteriaAsToken.hasMoreTokens(); ) {
try {
   String index = criteriaAsToken.nextToken(FIELDVALUE_DELIMITER)
   .replace(CRITERION_DELIMITER.toCharArray()[0],' ').trim();
    criteriaAsToken.nextToken(VALUE_ENCLOSER);
    String value = criteriaAsToken.nextToken(VALUE_ENCLOSER);
    criteriaAsToken.nextToken(CRITERION_DELIMITER);
    System.out.println("Index = " + index + " Value = " + value );
    if ( (dataFieldPresent(index)) && (!value.equals("")) )
     resultMap.put(index,value);
}catch(NoSuchElementException nsee) {
nsee.printStackTrace(); // can be removed after debugging
}
}

CRITERION_DELIMITER = ",";
FIELDVALUE_DELIMITER = "=";
VALUE_ENCLOSER = "'";
Not using the word 'ANY' in the program is by this algorithm.
First all nonMatching recordnumbers are collected in a List. Second, get all the Live recordnumbers in a List called matchingRecordNumbers. Third is removeAll() nonMatching recordNumbers from matchingRecordNumbers. This would give us the matching records. Fourthly, for all the matching recordNumbers , get DataInfo and pack them in a List.
Following is a code snippet

Map result = parse(criteria);
ArrayList nonMatchingRecordNumbers = new ArrayList(recordCount);
boolean nonMatching = false;
        for (int r = 1; r <= recordCount; r++) {
DataInfo record = getRecord(r);//seek(r);
for (Iterator resultIterator = result.keySet().iterator();resultIterator.hasNext()  :wink:   {
String resultIndex = (String) resultIterator.next();
String resultValue = (String) result.get(resultIndex);
if (!(record.getValue(resultIndex).trim().equals(resultValue))) {
nonMatching = true;
break;
}else {
nonMatching = false;
continue;
}
}
if (nonMatching) nonMatchingRecordNumbers.add(new Integer(record.getRecordNumber()));
nonMatching = false;
}
List matchingRecordNumbers = getAllRecordNumbers();
matchingRecordNumbers.removeAll(nonMatchingRecordNumbers);

ArrayList matchingRecords = new ArrayList(matchingRecordNumbers.size());
System.out.println("Number of Non Matching Records = "+ nonMatchingRecordNumbers.size());
System.out.println("Number of Matching Records = " + matchingRecordNumbers.size() );
for (int i=0;i<matchingRecordNumbers.size();i++) {
matchingRecords.add( getRecord(( (Integer) matchingRecordNumbers.get(i)).intValue()));
}

Following are my fears :
1) Getting a ALL record numbers in a List would give rise to memory errors.
2) Adding four composed methods and its validity.
3) Although 'Any' is implemented, it is not used explicitly.
Please tell if all the above are okay and all other errors.
Thanks in advance,
Rajesh
[ April 24, 2003: Message edited by: Rajesh Rajesh ]

[Andrew Monkhouse]

Hi Rajesh,

2) Not using the word 'ANY' , but the implementation implicitly uses this.

I also disagree with the use of the word "any" (ANY is the origin / destination code for Anthony Municipal Airport, KS, USA), however the specifications do require it, so I think it has to be allowed for - I am specifically using the lowercase "any" as written in the documentation to try and distinguish it from real codes.
I think we have to write the server to meet the criteria specified for the client, not for how we write the client. Subtle difference. The specifications are written in such a way that I should be able to use your server with my client or you mine. In fact, this could be a way of testing the server: the examiner plugs in a known client to perform standard tests against the server. If you think about them doing this, then since the spec allows a client to send "any", if you dont handle it, you could fail that test.
I havent really looked at the code itself yet - maybe later today.
Regards, Andrew
[ April 24, 2003: Message edited by: Andrew Monkhouse ]

[Rajesh So]

Thanks for pointing out Andrew, I have added 'ANY' , but during parsing, I remove the 'Any' field out of the search criteria.

    if ( (dataFieldPresent(index)) && (!value.equals("")) && (!value.toLowerCase().equals(ANY)) )
     resultMap.put(index,value);

I welcome/request all experts to clear my doubts, pleasseee....
Thanks in advance,
Rajesh

[aadhi agathi]

please correct your previous posts. it's very difficult to read your code!

[Rajesh So]

public synchronized List criteriaFind(String criteria) throws IOException, DatabaseException {
invariant();
boolean criteriaPresent;
List matchingRecords, nonMatchingRecordNumbers,matchingRecordNumbers;
Map result = parse(criteria);
if (result.size() == 0) { 
criteriaPresent = false;  
}else {
criteriaPresent = true;
}
if (criteriaPresent) {
nonMatchingRecordNumbers = new ArrayList(recordCount);
boolean nonMatching = false;
        for (int r = 1; r <= recordCount; r++) { // run for every record
DataInfo record = getRecord(r);
for (Iterator resultIterator = result.keySet().iterator();
resultIterator.hasNext();
) { // run for every criterion given
String resultIndex = (String) resultIterator.next();
String resultValue = (String) result.get(resultIndex);
if (!(record.getValue(resultIndex).trim().equals(resultValue))) {
nonMatching = true;
continue; //Not matching.Continue the loop to check next criterion
}else {
nonMatching = false;
break; // Matching. Break, as we don't want this record.
}
}
// Collect all non Matching records
if (nonMatching) nonMatchingRecordNumbers.add(new Integer(record.getRecordNumber()));
}
matchingRecordNumbers = getAllRecordNumbers();// All Live record Numbers 
// Remove all non Matching records from 'All records'.The result are the matching ones!!
matchingRecordNumbers.removeAll(nonMatchingRecordNumbers);

    // Package the matching records as a List of DataInfo's 
matchingRecords = new ArrayList(matchingRecordNumbers.size());
for (int i=0;i<matchingRecordNumbers.size();i++) {
matchingRecords.add( getRecord(( (Integer) matchingRecordNumbers.get(i)).intValue()));
}
}else {
matchingRecords = new ArrayList(); // Empty matching records as there are no criterion 
}
    return matchingRecords;
}

This is the parse method.

   /**
    *  Parses the given String into a Map of DatabaseFieldNames and values.
   *  Carrier='SpeedyAir',Origin='SFO'Uses encloser ' for values
*  This method parses the given criteria into field name and value as key value pairs.
*  It discards invalid, empty fieldNames and values. And also the values with 'ANY'
*  There are chances that the given Map may be empty as the entire input is not
*  properly formatted or contains invalid/empty field names/values.    
   */
private Map parse(String criteria) {
        // Split the given data into index and value
Map resultMap = new HashMap(description.length);
for (StringTokenizer criteriaAsToken = new StringTokenizer(criteria); 
criteriaAsToken.hasMoreTokens(); ) {
try {
   String index = criteriaAsToken.nextToken(FIELDVALUE_DELIMITER)
   .replace(CRITERION_DELIMITER.toCharArray()[0],' ').trim();
    criteriaAsToken.nextToken(VALUE_ENCLOSER);
    String value = criteriaAsToken.nextToken(VALUE_ENCLOSER).trim();
    criteriaAsToken.nextToken(CRITERION_DELIMITER);
    if ( (isDataFieldPresent(index))
     && (!value.equals(""))
     && (!value.toLowerCase().equals(ANY)) )
     resultMap.put(index,value);
}catch(NoSuchElementException nsee) {
nsee.printStackTrace(); // can be removed after debugging
}
}
return resultMap;
}

Hope this is clear. Do you feel the code is not readable? Please tell me if the code communicates its operation.
Rajesh

[Ta Ri Ki Sun]

just want to see what the code tag does after I format the code, some of your comments may be on the next line.

 
 public synchronized List criteriaFind(String criteria) throws IOException,
            DatabaseException {
        invariant();
        boolean criteriaPresent;
        List matchingRecords, nonMatchingRecordNumbers,matchingRecordNumbers;
        Map result = parse(criteria);
        if (result.size() == 0) {
            criteriaPresent = false;
        } else {
            criteriaPresent = true;
        }
        if (criteriaPresent) {
            nonMatchingRecordNumbers = new ArrayList(recordCount);
            boolean nonMatching = false;
            for (int r = 1; r <= recordCount; r++) { // run for every record
                DataInfo record = getRecord(r);
                for (Iterator resultIterator = result.keySet().iterator();
                     resultIterator.hasNext() ;)  {
                    // run for every criterion given
                    String resultIndex = (String) resultIterator.next();
                    String resultValue = (String) result.get(resultIndex);
                    if (!(record.getValue(resultIndex)
                            .trim().equals(resultValue))) {
                        nonMatching = true;
                        continue;
                        //Not matching.Continue the loop to check next criterion
                    } else {
                        nonMatching = false;
                        break; // Matching. Break, as we don't want this record.
                    }
                }// Collect all non Matching records
                if (nonMatching)
                    nonMatchingRecordNumbers.
                            add(new Integer(record.getRecordNumber()));
            }
            matchingRecordNumbers = getAllRecordNumbers();
            // All Live record Numbers
            // Remove all non Matching records from 'All records'.
            //The result are the matching ones!!
            matchingRecordNumbers.removeAll(nonMatchingRecordNumbers);
            // Package the matching records as a List of DataInfo's
            matchingRecords = new ArrayList(matchingRecordNumbers.size());
            for (int i = 0; i < matchingRecordNumbers.size(); i++) {
                matchingRecords.add(getRecord(((Integer) matchingRecordNumbers.
                        get(i)).intValue()));
            }
        } else {
            matchingRecords = new ArrayList();
            // Empty matching records as there are no criterion
        }
        return matchingRecords;
    }

 
private Map parse(String criteria) {
        // Split the given data into index and value
        Map resultMap = new HashMap(description.length);
        for (
                StringTokenizer criteriaAsToken = new StringTokenizer(criteria);
                criteriaAsToken.hasMoreTokens() ;)  {
            try {
                String index = criteriaAsToken.nextToken(FIELDVALUE_DELIMITER)
                        .replace(CRITERION_DELIMITER
                        .toCharArray()[0], ' ').trim();
                criteriaAsToken.nextToken(VALUE_ENCLOSER);
                String value = criteriaAsToken.nextToken(VALUE_ENCLOSER).trim();
                criteriaAsToken.nextToken(CRITERION_DELIMITER);
                if ((isDataFieldPresent(index)) && (!value.equals(""))
                        && (!value.toLowerCase().equals(ANY)))
                    resultMap.put(index, value);
            } catch (NoSuchElementException nsee) {
                nsee.printStackTrace(); // can be removed after debugging
            }
        }
        return resultMap;
    }

[ April 25, 2003: Message edited by: Ta Ri Ki Sun ]

[Andrew Monkhouse]

Hi Rajesh,
I think this is going to be a picky post, so please turn away now if you are feeling squeemish. Also - these are my comments, not Sun's, not the examiners ... so if I say something you disagree with, feel free to ignore it.

public synchronized List criteriaFind(....

According to my instructions, the signature should be:

public DataInfo[] criteriaFind(String criteria)

I dont think the synchronized causes a problem but returning a different data type certainly will. IMHO You need to return the data type specified.

List matchingRecords, nonMatchingRecordNumbers,matchingRecordNumbers;

This goes against the Sun coding standards,:

6 - Declarations
6.1 Number Per Line
One declaration per line is recommended since it encourages commenting. In other words,
int level; // indentation level
int size; // size of table
is preferred over
int level, size;

For the complete coding conventions, see: Sun's coding conventions

                    if (! (record.getValue(resultIndex).trim().equals(resultValue)))

How does this handle an invalid field name?

                String value = criteriaAsToken.nextToken(VALUE_ENCLOSER).trim();

Sun coding standards suggest that variables should be declared at the start of a block, not just prior to its use.

                    && (!value.toLowerCase().equals(ANY)))

I have given the user a choice of "any" (in lower case) or any of the values present in the database. This means that if they do choose "any" it will be in lowercase already. If the option "ANY" comes through, it will be because it is in the database, in which case I want it. For my logic, the "toLowerCase" above would negate this. YMMV.
Sorry again for being so critical. Hopefully you will have noted that all my objections are style based, not really problems with the code as such, and therefore can be ignored since no 2 programmers are ever going to agree 100% with each other in matters of style.
Regards, Andrew
[ April 25, 2003: Message edited by: Andrew Monkhouse ]
[ April 25, 2003: Message edited by: Andrew Monkhouse ]

[Rajesh So]

How does this handle an invalid field name?

If an invalid field name is given, it will be filtered in the parse method and will not be searched for. Hence the result will be empty.
I agree with Andrew's suggestions.
Please tell me if my logic in criteriaFind is correct. I am worried of the memory usage as I create a List full of all record numbers of the database.
Also, tell if the entire algorithm is okay.
Thanks in advance,
Rajesh

[Rajesh So]

Any one to help me out here??? Your suggestions regarding my having a list of ALL records and subtracting the NONMatching records would be very useful.
In nut shell, it is
ALL Records - NONMATCHING Records = MATCHING Records.
Please comment on that.
Thanking you,
Rajesh

[Andrew Monkhouse]

Hi Rajesh,
As I understand it, you are stating that your code works, and are just asking for a critique of your coding style / algorithm use. So that is all I will be commenting on.
Can you please confirm that your code does work?
You said: Your suggestions regarding my having a list of ALL records and subtracting the NONMatching records would be very useful
I am wondering why you are doing it this way. I would have thought it easier just to track the matching records.
At the end of your "for ..." loop stepping over fields in the record, you should be able to know whether the record matches the criteria or not. To me, it would be more logical at this point to save the record you know is correct, rather than save the record you know is incorrect.
I think you should reconsider some of your variable names to make it easier to read your code. For example:
Map result = parse(criteria);
to me, result is something you are sending back to the calling method, not an internal variable name. Perhaps criteriaMap would be more meaningful?

Andrew: How does this handle an invalid field name?
Rajesh: If an invalid field name is given, it will be filtered in the parse method and will not be searched for. Hence the result will be empty.

result as in the name of the map you use to hold the criteria?
I didnt see that in your parse method - it appeared to throw the exception, then continue with the next criteria, which would mean the bad field name is just ignored.
Regards, Andrew

[Rajesh So]

Can you please confirm that your code does work?

Yes. If the input is given in the correct format, the output will be recieved. If the input is given in wrong format, it will not complain. (which is how the assignment stated). Also, if the input is in the wrong format, it will be filtered in the parse() method and no searching will be done.

You said: Your suggestions regarding my having a list of ALL records and subtracting the NONMatching records would be very useful
I am wondering why you are doing it this way. I would have thought it easier just to track the matching records.

The intention is that there should not be any cycles dedicated for "ANY". "ANY" Field is discarded in the parse() method and no searching/checking is done for this.If we collect a matching records, as you said, we would have to consider searching/checking for "ANY". Hence this way of removing all non matching records from the total records.

I think you should reconsider some of your variable names to make it easier to read your code. For example:
Map result = parse(criteria);
to me, result is something you are sending back to the calling method, not an internal variable name. Perhaps criteriaMap would be more meaningful?

ACCEPTED

I didnt see that in your parse method - it appeared to throw the exception, then continue with the next criteria, which would mean the bad field name is just ignored.

Yes. We are not going complain to the caller of this method, that they have given an invalid field name. Because, our requirement asks us to ignore if the input criteria is in the bad format.
Please tell me if my thinking is all CORRECT.
Besides, I have also used the record numbers for collecting the ALL Records and Non Matching records. Later, only the matching records will be converted to a List of DataInfo's . You suggested me to use the same signature, but I feel we could pack the output and give it to the caller. Is using Integer of record numbers elegant???
Thanks in advance,
Rajesh
[ May 08, 2003: Message edited by: Rajesh Rajesh ]

[Rajesh So]

Hi,
Can you review this logic?? Your suggestions will be most appreciated and welcomed. This would help me to move on to other parts of the assignment.
Thanks in advance,
Rajesh

[Andrew Monkhouse]

Hi Rajesh,
If I send through the criteria: Carrier='SpeedyAir',Origin airport='any', then you will discard the Origin Airport field from the fields to match on. (This sounds fine to me). Your parse method will return back a map containing:

{"Carrier", "SpeedyAir"}

But what about the criteria: Carrier='SpeedyAir',Origin airport='any' Date="20030512"? You will discard the Origin Airport field and the Date field from the fields to match on wont you? Your parse method will return back the same map as before.
Or am I wrong?
For both searches, you will return (using my data) 4 records.
My instructions state:

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.

So for my instructions, the second search above should return zero records - do you have different instructions?
Regards, Andrew

[Rajesh So]

For the following input,

Carrier='SpeedyAir',Origin airport='any'

The output is
{"Carrier", "SpeedyAir"}
For the following input,

Carrier='SpeedyAir',Origin airport='any' Date="20030512"

The output is {"Carrier", "SpeedyAir"}

But what about the criteria: Carrier='SpeedyAir',Origin airport='any' Date="20030512"? You will discard the Origin Airport field and the Date field from the fields to match on wont you? Your parse method will return back the same map as before. Or am I wrong?

No.You are perfectly correct. Date is discarded (at the parse method)because it was not preceeded by the delimiter "," and its value not enclosed by "'". The discarding comes before it is checked for the validity of the field name.

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.
So for my instructions, the second search above should return zero records - do you have different instructions?

No the second search does return 1 key/value {"Carrier", "SpeedyAir"}. I hope your second try is Carrier='SpeedyAir',Origin airport='any' Date="20030512" .However if the format was
Date="20030512" Carrier='Speedy Air', Origin Airport='SFO' , the parse method would return zero records, as the format is wrong; lack of delimiter between date and carrier. If the format was Date='20030512', Carrier='Speedy Air', Origin Airport='SFO' , then it would be {Carrier="Speedy Air", Origin Airport="SFO"}.
The format matters much for this logic. If the format is wrong, then our way of String tokenizing traps NoSuchElementException. If the format is right, but given invalid field names, there is no problem.
However there are some unpredictable behaviors in handling wrong formats: If the specified format is wrong after the first one or two elements, the first one or two right ones are considered. If the format in the first key/value only is specified wrong, then the rest may be considered NoSuchElement or invalid field names.
Hope I have communicated the strength and weaknesses clearly. Please tell me your suggestions...
Thank you so much Andrew,
Rajesh
[ May 11, 2003: Message edited by: Rajesh Rajesh ]

[Rajesh So]

Hi!!! Please help me out...... Let me know how simpler I can communicate to you, so that you can guide me easier.
Rajesh

[Pander Musubi]

Why don't people read the instuctions properly?
In the GUI requirement, the user can specify any for one of the criteria.
In the criteriaFind requiments, nothing is said on the wildcard "any". Only that there is a comma separated list of criteria.
So in my opinion there is no need or criteriaFind to be able to be aware of "any" as a wildcard at all.

[Andrew Monkhouse]

Hi Pander,
Welcome to JavaRanch.
Good point.
Regards, Andrew