Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm modeling a reflection-based controller. I would like to know if you agree with my implementation and about what could be enhanced.

I'm starting with reflection and I would like to know if I'm using good practices.

public class StartController extends HttpServlet {

    @Override
    public void service(HttpServletRequest req, HttpServletResponse resp)
            throws ServletException, IOException
    {
        String vars = req.getParameter("mvc");
            ApplicationContext context = WebApplicationContextUtils.getWebApplicationContext(getServletContext());

        MainController main = new MainController();
        main.RESTProcess(req, resp, context, vars);
    }
}

public class MainController extends Controller {

    public void RESTProcess(HttpServletRequest req, HttpServletResponse resp, ApplicationContext context, String vars)
            throws IOException, ServletException
    {
        String[] url;
        int nvars;

        String controller = null;
        String action = null;;
        String[] params = null;
        int i;
        int n;

        String controllerName;
        String actionName;

        if(vars == null || "".equals(vars.trim()))
        {
            this.redirect(resp,"home");
        }
        else
        {       
            url = vars.split("/");
            nvars = url.length;

            if(nvars > 0)
            {
                controller = url[0].trim(); //users
                if(nvars > 1)
                {
                    action = url[1].trim(); //edit
                    if(nvars > 2)
                    {
                        n = 0;
                        params = new String[nvars - 2]; //array[0] = 5 (iduser)
                        for(i = 2; i < nvars; i++)
                        {
                            params[n] = url[i];
                            n++;
                        }
                    }
                }


                controllerName = this.getFirstUpper(controller) + "Controller"; //HomeController, UserController pattern
                if(!controllerName.equals("Controller"))
                {
                    actionName = "action" + this.getFirstUpper(action); //actionIndex, actionEdit pattern
                    if(!action.equals("action"))
                    {
                        try
                        {
                            Class classe = Class.forName("com.sample.controller." + controllerName);
                            try
                            {
                                Constructor c = classe.getConstructor(HttpServletRequest.class, HttpServletResponse.class, String[].class);
                                try
                                {
                                    Object obj = c.newInstance(req, resp, context, params);

                                    Method m = classe.getMethod(actionName, null);
                                    m.invoke(obj, null);

                                    System.out.println(obj.toString());
                                }
                                catch (Exception e)
                                {
                                    e.printStackTrace();
                                }
                            }
                            catch (Exception e)
                            {
                                e.printStackTrace();
                            }
                        }
                        catch (Exception e)
                        {
                            e.printStackTrace();
                        }
                    }
                }
            }
            else
            {
                this.redirect(resp,"home");
            }
        }
    }

    public String getFirstUpper(String str)
    {
        Integer x = str.length();
        str = str.substring(0,1).toUpperCase().concat(str.substring(1, x));

        return str;
    }
}

public class UserController extends Controller {

    private HttpServletRequest req;
    private HttpServletResponse resp;
    private ApplicationContext context;
    private String[] params;

    public UserController(HttpServletRequest req, HttpServletResponse resp, ApplicationContext context, String[] params)
    {
        this.req = req;
        this.resp = resp;
        this.context = context;
        this.params = params;
    }

    public void actionEdit()
    {
        Long id = Long.valueOf(this.params[0]);
        System.out.println("/home/edit");
    }
}
share|improve this question

2 Answers 2

I would make some changes to getFirstUpper():

public String getFirstUpper(String str)
{
    Integer x = str.length();
    str = str.substring(0,1).toUpperCase().concat(str.substring(1, x));

    return str;
}

The name x is not a very good name as it's only a single character. I'm not even sure if a separate variable is needed for this, but if so, consider renaming it to stringLength. The type should also be int instead, which is the return type of String.length(). Either way, you can just pass length() directly into substring() (in place of x).

It's also unnecessary to create a str variable if the inner function's return value is just going to be returned right away. Plus, it's already known that this method is supposed to return a String.

return str.substring(0,1).toUpperCase().concat(str.substring(1, str.length()));

I also have a general comment regarding curly braces:

The Java standard dictates the use of "Egyptian braces," which involves putting the opening brace on the same line as the statement, rather than the next line by itself:

public String getFirstUpper(String str) {
    // ...
}
share|improve this answer

Only few general comments from me:

  1. Start your method names with lowercase.

  2. Remove excess try keywords.

  3. Catch only specific type of Exceptions!

share|improve this answer
1  
Please elaborate a bit more on the reasoning behind your proposals. While they are all good, the motto of CR is "teach a man how to fish" and not "give a man a fish" ;) –  Vogel612 Aug 8 at 18:06

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.