[Java] Redundante code

[|orion]

Ik ben voor een school project bezig met het coden van een website in java Servlets en JSP's. Alles gaat goed en aardig, maar ik heb een stuk redundante code waarmee ik niet weet wat ik er nou mee moet.

Het gaat om dit stuk code:

code:

 public boolean authenticateUser( User user ) {
     int userCount = database.getUserCount( user );

     if ( userCount>0 )
         return true;
     else
         return false;
 }

 public boolean userExists( User user ) {
     int userCount = database.getUserCount( user );

     if ( userCount>0 )
         return true;
     else
         return false;
 }

De methode authenticateUser vraagt aan het database object hoeveel users er zijn met de username+password combinatie die in het User object wordt meegegeven. Als dit groter dan 0 is, dan bestaat zo'n combinatie en heeft de user (in mijn ogen) bewezen te bestaan in het systeem. De methode userExists gebruikt dezelfde code maar heeft als doel het checken of een user niet al toevallig bestaat in de database.

Beide methoden gebruiken dezelfde code met een ander doel. Is het nou verstandig om van die code een methode te extracten of is het slimmer om het zo te laten staan, met het oog op eventuele toekomstige uitbreidingen.

Dit staat trouwens los van dat ik de code eventueel nog moet veranderen om meer feedback te kunnen geven (in dit geval kan je alleen true of false terug geven. Het is lastig/onmogelijk om onderscheid te maken tussen 'false' van dat een user niet bestaat en 'false' mocht de database server plat liggen.).

edit:
commentaar moet er trouwens ook nog bij

[ Voor 4% gewijzigd door |orion op 25-09-2004 12:58 ]

[dominic]

Kun je misschien laten zien wat er in de getUserCount() functie zit?

Zowieso lijkt dit me beter, maar dat terzijde:

code:

public boolean userExists( User user ) {
     return database.getUserCount( user ) == 0 ? false:true;
 }

Dit staat trouwens los van dat ik de code eventueel nog moet veranderen om meer feedback te kunnen geven (in dit geval kan je alleen true of false terug geven. Het is lastig/onmogelijk om onderscheid te maken tussen 'false' van dat een user niet bestaat en 'false' mocht de database server plat liggen.).

Dit zou je met een simpele try/catch kunnen afvangen..

[Creepy]

dominic schreef op 25 september 2004 @ 13:02:
Kun je misschien laten zien wat er in de getUserCount() functie zit?

Zowieso lijkt dit me beter, maar dat terzijde:

code:

public boolean userExists( User user ) {
     return database.getUserCount( user ) == 0 ? false:true;
 }

[...]


Dit zou je met een simpele try/catch kunnen afvangen..

Als je dan gaat zitten nitpicken over code style doe het dan meteen goed, aangezien een vergelijking meteen een boolean oplevert

code:

return (database.getUserCount( User ) > 0);

Bij authenticateUser zou ik nog een eventuele passwd check verwachten. Zo niet dan is in mijn ogen heel die authenticateUser overbodig en dekt de naam van userExist perfect de lading van wat die method doet.

[ Voor 5% gewijzigd door Creepy op 25-09-2004 13:49 ]

[wizzkizz]

Creepy schreef op 25 september 2004 @ 13:48:
Bij authenticateUser zou ik nog een eventuele passwd check verwachten.

|orion schreef op 25 september 2004 @ 12:57:
(....)
De methode authenticateUser vraagt aan het database object hoeveel users er zijn met de username+password combinatie die in het User object wordt meegegeven.
(....)

[ Voor 24% gewijzigd door wizzkizz op 25-09-2004 13:57 ]

[|orion]

dominic schreef op 25 september 2004 @ 13:02:
Kun je misschien laten zien wat er in de getUserCount() functie zit?

code:

public int getUserCount( User user ) {
   try {
      NumericResult result = (NumericResult) sqlMap.queryForObject("getUserCount", user);
      
      int userCount = result.getResult();
      return userCount;
   } catch (SQLException e) {
      Debug.printWarning( "Error (SQLException) while getting usercount:");
      Debug.printSubWarning( e.toString() );
   }
   
   return -1;
}

Hier wordt gebruik gemaakt van een SQL map waarvan een een mooi java boontje terug krijg.

Zowieso lijkt dit me beter, maar dat terzijde:

code:

public boolean userExists( User user ) {
     return database.getUserCount( user ) == 0 ? false:true;
 }

Opzich kan dat prima, maar dan zit ik nog steeds met twee keer hetzelfde stuk code. Het oogt niet zo netjes en ik vraag me dus af wat (sommige/meeste/whatever) mensen daaraan doen.

[...]
Dit zou je met een simpele try/catch kunnen afvangen..

Daar had ik nog niet aan gedacht. Danku

[Grijze Vos]

Ik neem aan dat bij de userExist het password niet van belang is, toch?
Anders kun je 2x dezelfde username hebben met verschillende passwords, en dat is niet handig.

(Als mijn bovenstaande aanname klopt, dan zou je code dus wel verschillend moeten zijn. )

[FendtVario]

Java:

 public boolean authenticateUser( User user ) {
    return (userExists(user));
 }

 public boolean userExists( User user ) {
     int userCount = database.getUserCount( user );

     if ( userCount>0 )
         return true;
     else
         return false;
 }

Zoiets misschien?

[ Voor 28% gewijzigd door FendtVario op 25-09-2004 15:08 ]

[Alarmnummer]

Waarom plaatsen jullie in godsnaam extra haakjes om die expressie bij het return statement?? Dat is totaal zinloos.. het maakt de code alleen maar slechter te lezen.

[FendtVario]

Persoonlijke smaak denk ik . Bij 'return (userExists(user))' is er in de return dus geen vergelijking, maar in return (database.getUserCount( User ) > 0);' wel. IK vind de code hier zeker wel duidelijk omdatje je precies ziet wat de return is.

[Alarmnummer]

FendtVario schreef op 25 september 2004 @ 15:55:
Persoonlijke smaak denk ik . Bij 'return (userExists(user))' is er in de return dus geen vergelijking, maar in return (database.getUserCount( User ) > 0);' wel. IK vind de code hier zeker wel duidelijk omdatje je precies ziet wat de return is.

Ja.. dat is dat ding achter return Daarvoor ben je geen extra haakjes nodig.

[|orion]

Grijze Vos schreef op 25 september 2004 @ 15:04:
Ik neem aan dat bij de userExist het password niet van belang is, toch?
Anders kun je 2x dezelfde username hebben met verschillende passwords, en dat is niet handig.

(Als mijn bovenstaande aanname klopt, dan zou je code dus wel verschillend moeten zijn. )

Aanname correct, conclusie alleen niet helemaal. Ik maak gebruik van SQL maps en daarmee mogelijke dynamische/conditionele query (weet niet precies welke term het heeft). Als in de user bean de password string ongelijk is aan "" (en er dus een wachtwoord gespecificeerd is, zoals in het geval van authenticateUser) dan wordt deze upgenomen in de query met een AND ervoor, als er geen password is opgegeven wordt alleen gezocht op username.

Mijn Java code blijft daarmee dus gelijk. Al het 'moeilijke' werk wordt door de SQL Maps afgehandeld.

Ter ilustratie staat hieronder hoe de query zegmaar bekend is bij het SQL Maps object:

code:

<select id="getUserCount" resultClass="model.NumericResult">
    select count(*) as result
    from account
    where username=#username#
    <isNotEmpty prepend="AND" property="password">
        password=#password#
    </isNotEmpty>
</select>

FendtVario schreef op 25 september 2004 @ 15:05:

Java:

 public boolean authenticateUser( User user ) {
    return (userExists(user));
 }

 public boolean userExists( User user ) {
     int userCount = database.getUserCount( user );

     if ( userCount>0 )
         return true;
     else
         return false;
 }

Zoiets misschien?

Dat is opzich een goede, maar is dit een nette manier van code met oog op de toekomst waarin ik misschien besluit userExists te veranderen/uitbreiden en vergeet dat authenticateUser anders gaat reageren?

Hoe meer ik hiermee bezig ben hoe meer ik bang ben dat ik nu de 'beste' optie gebruik.

edit:
Het anders reageren van de code is misschien op te lossen met een of meer Unit tests, doe ik daar ook weer ervaring mee op

[ Voor 7% gewijzigd door |orion op 25-09-2004 16:36 ]

[FendtVario]

Of dit voor de toekomst ook toereikend is zal jij moeten beslissen. Omdat de implementatie in beide procedures gelijk was is dit een mogelijke oplossing. Als een procedure wordt uitgebreid of veranderd kan het zijn dat je de aanroepende functoe (hier authenticateUser) moet veranderen. Wil je dit niet vergeten dan kun je natuurlijk nog altijd een commentaar in userExists zetten.

[Verwijderd]

Je 'probleem' wordt feitelijk veroorzaakt doordat je twee functioneel verschillende zaken in 1 functie afhandelt ( de getUserCount() ).
Daar is op zich niks mis mee, maar maak dan gewoon 1 functie die zowel het authenticeren als het opvragen of de user bestaat afhandelt. En noem die dan queryUser bijvoorbeeld. Dat dekt de lading beter dan alleen userExists of alleen authenticateUser...

[zneek]

ik denk dat je 2 verschillende methodes nodig bent. Als er een nieuwe user aangemaakt wordt kijk je niet naar dezelfde data als bij inloggen. Authenticatie gebeurt op loginnaam en wachtwoord, terwijl bij het aanmaken van een user je eigenlijk al niet wilt dat dezelfde loginnaam als een andere user gebruikt wordt. En daarmee heb je geen probleem meer met dubbele code, omdat de code per functie moet verschillen.

[pjotrk]

|orion schreef op 25 september 2004 @ 16:32:
[...]
Aanname correct, conclusie alleen niet helemaal. Ik maak gebruik van SQL maps en daarmee mogelijke dynamische/conditionele query (weet niet precies welke term het heeft). Als in de user bean de password string ongelijk is aan "" (en er dus een wachtwoord gespecificeerd is, zoals in het geval van authenticateUser) dan wordt deze upgenomen in de query met een AND ervoor, als er geen password is opgegeven wordt alleen gezocht op username.

Mijn Java code blijft daarmee dus gelijk. Al het 'moeilijke' werk wordt door de SQL Maps afgehandeld.

Op welke wijze wordt er dan gegarandeerd dat bij het user object die de 'authenticateUser' functie binnen krijgt daadwerkelijk het wachtwoord geset is? Dan zou dat naar mijn idee toch ook nodig zijn om dit in de 'authenticateUser' functie te controleren, anders zou deze tenonrechte kunnen aangeven dat een gebruiker correct is ingelogd of mag inloggen.

[ Voor 3% gewijzigd door pjotrk op 26-09-2004 02:40 ]

[|orion]

pjotrk schreef op 26 september 2004 @ 02:33:
[...]


Op welke wijze wordt er dan gegarandeerd dat bij het user object die de 'authenticateUser' functie binnen krijgt daadwerkelijk het wachtwoord geset is? Dan zou dat naar mijn idee toch ook nodig zijn om dit in de 'authenticateUser' functie te controleren, anders zou deze tenonrechte kunnen aangeven dat een gebruiker correct is ingelogd of mag inloggen.

De checks op die info (iets niet ingevuld, ongelijk password, etc.) worden uitgevoerd voordat de user ge-insert wordt in de database. Daar hebben deze methoden niets mee te maken.