• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Jeanne Boyarsky
  • Ron McLeod
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • paul wheaton
  • Rob Spoor
  • Devaka Cooray
Saloon Keepers:
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Frits Walraven
  • Tim Moores
Bartenders:
  • Mikalai Zaikin

please review criteriaFind -> good riddance from hateful "ANY"

 
Ranch Hand
Posts: 263
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
could you please review the criteriaFind. thougn note that this is an alternative of getting rid of hateful "ANY"


pls find code in the recent post with proper format, below.



does:-
1. the current implementation solves the "ANY" criteria.
i mean if its a wildcard the user wants he can enter "ORIGIN=,DESTINATION='SFO'", it works in my implementation
2. is it a breaching the contract of the requirements
but i think my assumptions are reasonable.
Looking forward to your kind review and comments.
Thanks you.
[ January 29, 2003: Message edited by: aadhi agathi ]
[ January 29, 2003: Message edited by: aadhi agathi ]
[ January 29, 2003: Message edited by: aadhi agathi ]
 
aadhi agathi
Ranch Hand
Posts: 263
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
kindly forgive the whitespaces ! i doesnt know how to remove it
 
aadhi agathi
Ranch Hand
Posts: 263
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

at last
[ January 29, 2003: Message edited by: aadhi agathi ]
[ January 30, 2003: Message edited by: aadhi agathi ]
 
Ranch Hand
Posts: 94
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
What about a delimiter like "=';" for your tokenizer.
 
aadhi agathi
Ranch Hand
Posts: 263
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Originally posted by Charles Dupin:
What about a delimiter like "=';" for your tokenizer.


Charles,
my main aim is to "not" to have any delimeter. though i appreciate your idea.
 
aadhi agathi
Ranch Hand
Posts: 263
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Peter,
could you please review the criteriaFind.
 
aadhi agathi
Ranch Hand
Posts: 263
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
any help on this???
 
author
Posts: 3252
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Looking at the code from an arm's length, I notice the following code smells:
  • A method that is too big, mostly because it does two things: it parses the criteria string, and it performs the search. It's a composed method in need of some decomposition.
  • It starts with a whole raft of local variables, all method-scoped. There's another batch halfway through. Having so many in-scope variables makes code very hard to read.
  • Too many comments. Seriously. If you have to explain what your code does, the code isn't expressive enough. Comments, where necessary, should explain what the code wants to achieve, not how it goes about doing it.
  • A few specific points -- many of them are discussed at length in other posts on criteriaFind! --
    This is a minor performance optimisation. The requirements ask you to favour clarity over performance.
    Code to interface, not implementation. Use Map.
    Both are used only within nested blocks; please move their declaration to the narrowest scope you can get away with. There are no performance penalties associated with this, and a lot of readability bonuses. BTW, a single tokenizer will do if you use nextToken(String) and have it return tokens.
    This should be a public static final field. In any case, this wildcard arguably belongs in the front end rather than the back end. It doesn't add anything to the criteriaFind API (if you want to match any value, just drop the field from your criteria). It is tied to the English language; another strong hint that it's a user interface thing rather than a back-end thing.
    Rather than using an integer loop variable, use the StringTokenizer as an iterator:
    for (StringTokenizer fieldCriteriaTokenizer = new StringTokenizer(criteria,","); fieldCriteriaTokenizer.hasMoreTokens(); ) {
    If you don't like the for( ; ; ) idiom here, use a while loop. But do get rid of that unnecessary integer loop variable.
    You chose not to require quotation marks around the value to search for. This means your criteriaFind breaks on any search string that contains, for example, commas.
    How does a database developer find an empty string?
    There's a bug here anyway. "Origin=SFA,Destination=" will return all records. Surely that's not right.
    Surely the way to handle improperly formed criteria strings (as opposed to unknown field names) is an exception of some kind, probably a DatabaseException?
    You chose to write a non-synchronized method that uses getRecordCount() and getRecord(). What is the reason for the inconsistency of approach between the criteriaFind() method and the existing find() method? They both basically do the very same thing.
    Urgh, Vector is obsolete, uselessly synchronized and has a bloated API. Also, here and elsewhere, code to interface not implementation. List results = new ArrayList() would do fine.
    To illustrate a few points touched upon above... This is the first use of returnValueAsDataInfo (surely that reads like a method name rather than a variable name), yet it has been declared half a method before. You seem to prefer oldfashioned integer loop variables to iterators. And in any case, there's a method in the Java Collections API to achieve this stuff:Will do the same job.
    Have a browse through the Wiki links above. If you can get hold of a copy of Fowler's Refactoring, your world will not just expand, it will explode . Many of the points I also stole from McConnell'sCode Complete and Hunt, Thomas and Cunningham's Pragmatic Programmer. Have a trawl through the Collections trail if you haven't already done so.
    If you want to finish the assignment this year, you probably shouldn't try to read all of that at once but do read all of those books sometime if you can.
    Have fun
    - Peter
    [ January 30, 2003: Message edited by: Peter den Haan ]
     
    Ranch Hand
    Posts: 3404
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Peter,
    A very comprehensive list.
    The only Object Orientated - Programmer book I've had referred to me was Thinking in Java by Bruce Eckel.
    Very good to start with !
    How early would you recommend learning Patterns . I know they are in the API but making the connection between language and pattern can be difficult without some effort.I found the GoF book difficult to start with as I don't have a C/C++ background but it seems to get easier over time.
    Are there any books/resources you'd recommend ?
    regards
     
    author
    Posts: 11962
    5
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Originally posted by HS Thomas:
    Are there any books/resources you'd recommend ?
    regards


    Hi,
    I would recommend "Java Patterns Applied" (or was it "Applied Java Patterns", hmm). It contains descriptions of all the fundamental patterns in UML/Java (including MVC, observer, facade, proxy, etc.)
     
    Lasse Koskela
    author
    Posts: 11962
    5
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Once we're at it, I figured I should ask some opinions about my Map-based approach for criteriaFind()...
     
    HS Thomas
    Ranch Hand
    Posts: 3404
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Thanks Lasse,
    I'll look out for the book you mentioned.
    Taking an initial cursory look, it seems you have set up a query of values to match on (criteriaSet) , then searching each record and selecting those that have the set of query values(candidateSet), but regardless of the column they are in.
    So a query for Origin 'SFO' and destination 'LUX' can return , in addition, records that have origin 'LUX' and destination 'SFO'.
    As mentioned , this is a cursory look. I could be wrong . Apologies if I am.
    regards
     
    Peter den Haan
    author
    Posts: 3252
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Originally posted by HS Thomas:
    How early would you recommend learning Patterns. I know they are in the API but making the connection between language and pattern can be difficult without some effort.

    You have already learnt many patterns, and probably invented a few. Many you just have to learn to recognise
    Personally, I'm a GoF man. I read GoF when I did my SCJD assignment and I haven't read any other book that comes even close to it. Between them, Design Patterns and Refactoring taught me what little I know about software design. But what works for me does not necessarily work for you. If you liked Thinking in Java, have a look at the current draft of Thinking in Patterns. It discusses all the important patterns, uses Java examples that are fairly practical and down to earth, and talks about unit testing and refactoring as well.
    - Peter
     
    Lasse Koskela
    author
    Posts: 11962
    5
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Originally posted by HS Thomas:
    ...selecting those that have the set of query values(candidateSet), but regardless of the column they are in...


    Actually, the Set(s) contain instances of java.util.Map.Entry (an inner class), which do contain both a key and the value (in this case, column name and value).
    In other words, this Map/Set approach relies on the implementation of java.util.Map and java.util.Set to perform the comparison. What my code is doing is
    1) creating a Map ("A") based on a given query string, and
    2) creating another Map ("B") based on a DataInfo, a record read from the database.
    3) saying, "Map B, do you contain Map A?"
    It works. The question here is more about whether there are some "non-functional" defects in this approach.
    Thanks for the effort of reading it through, though
    [ February 01, 2003: Message edited by: Lasse Koskela ]
     
    HS Thomas
    Ranch Hand
    Posts: 3404
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    That's clever. I'm surprised it's not in the API - a MapSet or SetMap.
    I thought there might be some additional hidden
    objects in createCriteriaMap(query).entrySet() that could make it work.
    But I couldn't think what (It was late and I still had to fit some socializing in.)
    Also my understanding of Collections is rusty and very likely out of date.


    The only other question I can think of is:
    How is synchronisation achieved around this block of code ?
    I am assuming that the Collection Sets are synchronized sets.
    You have implemented criteriaFind in the Data class and the methods are synchronized.
    If you have implemented the LockManager then I can't see a problem that there is no sync in your implementation of criteriaFind.
    Does the LockManager lock for reads and writes ?
    [ February 02, 2003: Message edited by: HS Thomas ]
    [ February 02, 2003: Message edited by: HS Thomas ]
     
    HS Thomas
    Ranch Hand
    Posts: 3404
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    quoted from Peter den Haan's post:
    Between them, Design Patterns and Refactoring taught me what little I know about software design.


    Surely, Peter , that must be the understatement of the year?
    How did you start applying that knowledge?
    After SCJD , what tiny steps took you to the next levels so that you can be the mentor that you are.
    Books can give one a lot of knowledge. What is it that makes the knowledge come 'alive' so that you can apply what you have learnt confidently?
    It all just seems rather daunting.
    Thanks. I've added Refactoring to my list of books to buy.
    regards
    [ February 02, 2003: Message edited by: HS Thomas ]
    [ February 02, 2003: Message edited by: HS Thomas ]
     
    Lasse Koskela
    author
    Posts: 11962
    5
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Originally posted by HS Thomas:

    How is synchronisation achieved around this block of code ?
    I am assuming that the Collection Sets are synchronized sets.
    You have implemented criteriaFind in the Data class and the methods are synchronized.
    If you have implemented the LockManager then I can't see a problem that there is no sync in your implementation of criteriaFind.
    Does the LockManager lock for reads and writes ?


    The criteriaFind() is a read-only method so I figured synchronization is not an issue. But, now that you mentioned it, I noticed that I need to add a null-check inside the loop -- otherwise someone could mark a record deleted and I would get a NullPointerException... Thank you. A lot.
     
    HS Thomas
    Ranch Hand
    Posts: 3404
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Glad to have been of help.
    regards
     
    aadhi agathi
    Ranch Hand
    Posts: 263
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Hi Peter,
    please find the implementation of the criteriaFind with refactoring( i'm
    afraid to use the word) applied. i don't know whether it is up to the mark!!
    and also please let me know your kind suggestions.thanks for such a wonderful
    review and the great links too. i remeber something like this by Fowler.


    any fool(myself ) can write a code that a computer understands . but only a
    good programmer can write a code that the other prorammers can uderstand.






    ', is treated to be the criteria filter
    =' is treated to be the field value splitter


    1)currently it is not working for widlcard "ANY". i am assuming that
    i will do a searchString.indexOf("'ANY'") != -1 in the Controller
    and if it is yes the pass null to the Model
    2) i have an issue with MyStringTokenizer (please suggest a valid name for this),
    thats is if the criteria is say, will i be penalized for this. :roll:
    this is only due to a ugly limitation of StringTokenizer
    consider the search string
    a='aaa',b='bbbb',c='cccc' then
    then the last criteria token will be taken split as
    field = c and value is cccc' (note quotes at end).
    reason being ', will be the criteria parser and the last record
    is not having any ', so it returs the value as it is and when
    the field-value is parsed (i.e, =' ;the value becomes cccc').
    if i am trimming the quotes for all values then the criteria
    "a='''" this will be treated an empty string instead
    of a single quotes value. the current implementation supports
    single quotes , equals or any special character without any issue.


    the only way i can think of is have one more constructor for
    MyStringTokenizer ( the last param will delimit the last token
    in order) means if parser is ', and if the last token has '
    then it will be removed. if the delimiter is ';, then
    the last token will be removed if contains '; . will this
    be reasonable. adding comma will be one way with all the search
    criteria but i will be relying on the client to supply this
    please help me what to choose.




















    every time, i'm impressed with your posts, a star falls!
    so if i look above and find the sky empty, it's not your falt.
    i am greatful to the 3 amigos of yourself,Mark and Eugene.
    i like it here!!!


    [ February 03, 2003: Message edited by: aadhi agathi ]
     
    Peter den Haan
    author
    Posts: 3252
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Originally posted by Lasse Koskela:
    Once we're at it, I figured I should ask some opinions about my Map-based approach for criteriaFind()...

    It's brilliant
    I've seen a variant before that starts with a criteriaMap. For each record, it creates a candidateMap with only those fields that are in criteriaMap.keySet(). Then you can compare candidate with criteria using a simple equals().
    There is an efficiency price to pay, but the instructions clearly state that clarity takes precedence over efficiency, and what is clearer thanBesides, if you're concerned about efficiency -- which for this assignment you shouldn't really be, so ignore this! -- you can easily create a DataInfoMap class that wraps around a DataInfo object and implements Map. You create one instance before you start searching through the records and re-use it for every single record.
    - Peter
    [ February 03, 2003: Message edited by: Peter den Haan ]
     
    aadhi agathi
    Ranch Hand
    Posts: 263
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator


    1)currently it is not working for widlcard "ANY". i am assuming that
    i will do a searchString.indexOf("'ANY'") != -1 in the Controller
    and if it is yes the pass null to the Model


    just a thougt, what if the user gives
    origin='ANY''' means the value is ANY'' then will it also be "'ANY'"

     
    Peter den Haan
    author
    Posts: 3252
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Originally posted by aadhi agathi:
    [...] i don't know whether it is up to the mark!!

    Don't let me make you insecure. I'm a pedantic perfectionist and proud of it; take on board the things you agree with, and simply ignore the rest

    1)currently it is not working for widlcard "ANY". i am assuming that i will do a searchString.indexOf("'ANY'") != -1 in the Controller and if it is yes the pass null to the Model

    You don't expect your users to enter quotes, do you? I'd simply suggest something based on simple equality, along the lines ofOr something more elegant as appropriate.

    consider the search string
    a='aaa',b='bbbb',c='cccc' then
    then the last criteria token will be taken split as
    field = c and value is cccc' (note quotes at end).

    I used a StringTokenizer(criteria, ",='", true). The bit that handled the criterion value, including the quotes, wasWould I do it again in exactly the same way? Probably not. It looks a bit too much like C(++). Besides, today you can use regular expressions; the expression matches a criteria expression and captures fields and values in its groups (see Matcher.group()); what more can you want?... if you're comfortable with regexps, that is
    Anyway, to your code.That quote makes me suspicious. What's it doing in here? I expected the highest-level tokenizer to tokenize on commas only.
    You're initializing this variable, but the first thing you do next is assign another value to it?
    This "='" string should never occur because "'" is a delimiter. Please keep in mind that each character individually in the delimiter string is a delimiter for your tokenizer.
    I personally never use "else" after throwing an exception. To a reader, the "else" signals merely an alternative code path where two or more different paths can legitimately be taken. The exception is not a legitimate code path -- it's an exceptional situation that should actually never happen. So in my code I like to express this by letting further code continue as if the error condition didn't exist:Can you feel the difference?
    In checkValidFieldsA small method that starts with no fewer than six local variables, all of which have a scope as large as the method itself. As I said before, so many variables make code hard to read. The reason is that there can be so many different interactions between them.
    You really, really don't need them! Let me take a stab at rewriting your method without them.It's no beauty, with that labeled continue, and some go apoplectic over return statements other than at the very end of a function; not all gurus agree with this if used with care.
    Note a couple of things. Some of your temporary variables were simple optimisations, like fields and fieldNames[]. Don't; remember the first rule of optimization. Others simply had too much scope, like currentField which is used only inside the loop. Don't, always choose the narrowest scope possible. Yet others had no other purpose than to break up lines, like fieldIterator. Don't, and if you do, at least ensure that the relevant code sits right next to each other. Finally, and perhaps arguably, there was a lot of stuff that kept track of state, like allFieldsValid, that could be expressed using program flow.
    But this revised version has one remaining problem: it doesn't make good use of existing Java API. Let's radically rewrite it a second time: Note that the method signature has been changed to Collection; even in private methods I generally try to make as few assumptions as possible.
    Even so I would seek to eliminate this method, and indeed eliminate this check altogether. After all, everything eventually boils down toWell, if validCriteria contains a field that is not part of the database, this equals() will always be false and you will end up with an empty result set which is exactly what's required. No need for explicit checks.
    For the very same reason, you should be able to eliminate getAllRecords(). If validCriteria is empty, then currentRecordWithMatchingCriteria will be empty as well, equals() will return true and every single record will be added to the results. In other words, you can implement the required behaviour there, as well, without making any kind of explicit check.
    With all that's been said above, go through matchRecords yourself. You will probably spot quite a few more things that can use polishing.
    A final word. In a process like this, I like to have a top-level method that outlines the algorithm on a high level. Eg, in your case:See what I mean? I'm not saying that this is the best possible implementation (heck, I'm not even saying that it compiles), but you can look at this method for one minute and come away with a clear understanding of how the algorithm actually works.
    - Peter
    [ February 03, 2003: Message edited by: Peter den Haan ]
     
    aadhi agathi
    Ranch Hand
    Posts: 263
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator


    You don't expect your users to enter quotes,i doesnt do you?


    yes i do expect the users to enter quotes because i do support quotes , quotes within
    quotes. (even the value can be equal to =' or ''''@#='''(in simple, anything within
    ' (quote) and ',(quote commad)). consider for example, a='=\'' is the
    representation equivalent of =' as value for field a.

    say the user can enter a='\'' to indicate a vaue equal to quotes.
    a='\'\'' to indicate a value with two quotes.


    my only assumptions are ', will be the criteria terminator &
    =' will be the field termiantor.



    considering my affinity towards the support for quotes ( ),
    i cant do contains => indexOf('ANY') . reason being the value can
    be 'ANY\'' meaning the value is ANY' which is not equal to 'ANY'.

    the only way i can think of doing is like in the Data or the controller


    // if in Data
    1) single criteria and multiple criteria
    criteria.endsWith("='"+WILDCARD"'") || criteria.indexOf("='"+WILDCARD+"',") !=-1

    // if in Controller/facade
    2) single criteria and multiple criteria
    if (criteria.endsWith("'ANY'") || criteria.indexOf("='ANY',") !=-1){
    pass null for search


    i feel option 2 will be appropriate , which will not force me the
    burden of hardcoding anything in the Data class ( ).



    I used a StringTokenizer(criteria, ",='", true). The bit that handled the criterion value,
    including the quotes


    i can't use a StringTokenizer at all reason being


    =' will be the field value separator and
    =. will be the criteria delimeter as i support quotes



    Besides, today you can use regular expressions; the expression


    yes , this will elminate my implementation of MyStringTokenizer.
    WOW, it's great




    That quote makes me suspicious. What's it doing in here? I expected the highest-level tokenizer to tokenize on commas only.


    the reason is i expect the user to give the values in quotes.





    You're initializing this variable, but the first thing you do next is assign another value to it?


    yup, you told it , unthoughtfull.




    This "='" string should never occur because "'" is a delimiter.


    in my implementation ' can be a value too. so =' will become the field-value
    delimeter.


    I personally never use "else" after throwing an exception. To a reader, the "else"
    signals merely an alternative code path where two or more different paths can
    legitimately be taken.Can you feel the difference?


    great observation!. it's an eye opener for me. i always look for an else for a if.
    ( ) , i always feel uncomfortable at declaring varibales in the
    loop scope or if scope, how unthoughtfull i am .


    In checkValidFields ..A small method that starts with no fewer than six local variables
    ome of your temporary variables were simple optimisations, like fields and fieldNames[].
    Don't; remember the first rule of optimization. currentField which is used only inside
    the loop. Don't, always choose the narrowest scope possible...


    i agree!! i am slowly realizing the wastage of
    variables, scoping and pitfalls of "start with optimation" approach.
    yes, believe me, you made me think...and think in the right way too...

    wow!!


    For the very same reason, you should be able to eliminate getAllRecords().
    A final word. In a process like this, I like to have a top-level method that outlines the algorithm on a high level. Eg, in your case:


    but one thing, this is what i wanted to write( )


    see what I mean? I'm not saying that this is the best possible implementation
    (heck, I'm not even saying that it compiles)


    the most understatement of the centrury! a big salute to you
    [ February 03, 2003: Message edited by: aadhi agathi ]
    reply
      Bookmark Topic Watch Topic
    • New Topic