widerules / swe574-group1

Automatically exported from code.google.com/p/swe574-group1
0 stars 0 forks source link

Code review request #40

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago

NetworkService.java TODO explains why I need a review.

Original issue reported on code.google.com by turkmen...@gmail.com on 10 May 2014 at 8:56

GoogleCodeExporter commented 9 years ago
If the method finds out that token is expired or invalid, it can notify method 
caller by throwing business exceptions like TokenExpiredException or 
InvalidTokenException instead of taking response object as an argument and 
filling it.

Let's define two new exceptions, TokenExpiredException and 
InvalidTokenException, which extend java.lang.Exception.

//exception definitions
public class InvalidTokenException extends Exception {

}

public class TokenExpiredException extends Exception {

}

Then, your authenticate method will look like this:

    public static User authenticate(String token) throws InvalidTokenException, TokenExpiredException {

            BaseDao baseDao = DaoFactory.getInstance().getBaseDao();

            KeyValuePair<String, Object> qParams = new KeyValuePair<String,Object>("token", token);
            AccessToken aToken = baseDao.executeNamedQuery("AccessToken.getTokenByHash", qParams);

            // if no token is returned - invalid token, die
            if (aToken==null){
                    throw new InvalidTokenException();
            }

            // if token expired - die
            if (aToken.getExpiresAt()!=null && aToken.getExpiresAt().after(new Date())){
                throw new TokenExpiredException();
            }

            return aToken.getUser();
    }

Then caller method can be implemented like the following:

    public GetProfileResponse getProfileOfSelf(@WebParam(name="token") String token) {

        GetProfileResponse response = new GetProfileResponse();
        BaseDao baseDao = DaoFactory.getInstance().getBaseDao();

        // TODO: Code review: passing response (out) variable to the authenticate method
        // there may be a neater way of doing this?
        // ofcourse
        try {
            User user = ServiceCommons.authenticate(token);
            KeyValuePair<String,Object> qParams = new KeyValuePair<String,Object>("uid", user.getId());
            UserProfile up = baseDao.executeNamedQuery("UserProfile.getUserProfile", qParams);
            if(up != null){
                response.mapUserProfile(up);
            }
            response.succeed();

        } catch (InvalidTokenException e) {
            response.fail(ServiceErrorCode.TOKEN_INVALID);

        } catch (TokenExpiredException e) {
            response.fail(ServiceErrorCode.TOKEN_EXPIRED);

        } catch (Exception e) {
            response.fail(ServiceErrorCode.INTERNAL_SERVER_ERROR);
        }

        return response;
    }   

Original comment by esinkara...@gmail.com on 10 May 2014 at 6:06

GoogleCodeExporter commented 9 years ago
I hope above definitions will be useful

Original comment by esinkara...@gmail.com on 10 May 2014 at 6:07

GoogleCodeExporter commented 9 years ago

Original comment by esinkara...@gmail.com on 10 May 2014 at 6:42

GoogleCodeExporter commented 9 years ago
Caner selam,

Bu arada belirtmek isterim, Network servisinde benim test olarak eklediğim
bazı kodlar var Örnek Marcelino Sanchez user'inin dönüldüğü kodlar. 
Onları
git ten çok anlamadığım için istemeden commit ettim, bilgine...

Esin

Original comment by esinkara...@gmail.com on 10 May 2014 at 7:37

GoogleCodeExporter commented 9 years ago
I wrote the above comment via email without knowing that it will appear as a 
comment in this task. If I knew it, I would write it in English to prevent this 
weird situation; I mean half Turkish half English style.

Original comment by esinkara...@gmail.com on 10 May 2014 at 7:48

GoogleCodeExporter commented 9 years ago
Hi Esin,

Thank you very much for the review, very useful and elaborate.

How in the world did you find Marcelino Sanchez ? The standard Turkish 
placeholder I know, as you would know better, is Ahmet Kullanici.

Original comment by turkmen...@gmail.com on 12 May 2014 at 7:37

GoogleCodeExporter commented 9 years ago
+just for the record

I was aware that passing and modifying a mutable object to a function was a 
very ugly way of doing things. BUT This is so much code it defeats the whole 
point of functional programming :) I just added two files to the project and 4 
LOC to every method call. Amazing :)

Original comment by turkmen...@gmail.com on 12 May 2014 at 8:36

GoogleCodeExporter commented 9 years ago
Yes, I know that it is less common than Ahmet 
Uzunuzunkavaklaraltındauyuruyanmazoglu. Marcelina Sanchez is one of the actors 
who took park on The Warriors movie, which was shot on 1979.

http://www.imdb.com/title/tt0080120/?ref_=nv_sr_1

Original comment by esinkara...@gmail.com on 16 May 2014 at 8:47

GoogleCodeExporter commented 9 years ago

Original comment by esinkara...@gmail.com on 16 May 2014 at 8:47