-2
\$\begingroup\$

I have a method which uses the same code over and over again. I would like to optimize my code in such a way that there is no repetition in my code. I am sure there is a better way to do things instead of using the if-else statements.

private void web_content(final String content1, String content_type1, final String content2, String content_type2, final String content3, String content_type3, final String content4, String content_type4, final String content5, String content_type5, final String content6, String content_type6) {
    if (content_type1.equals("text"))
    {
        WebView webView1;
        webView1 = new WebView(this);
        webView1.setLayoutParams(new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT));
        web_linearLayout.addView(webView1);

        if (content1.equals("NA"))
        {
            Log.d("Content1 blank","content blank");
        }else
        {
            Document doc = Jsoup.parse(html_open);
            doc.append(content1);
            doc.append(html_close);
            webView1.getSettings().setJavaScriptEnabled(true);
            webView1.getSettings().setAllowFileAccess(true);
            webView1.getSettings().setLoadsImagesAutomatically(true);
            webView1.loadDataWithBaseURL("", String.valueOf(doc), "text/html", "utf-8", "");

        }


    }else if (content_type1.equals("image"))
    {


        String image =  image_url+content1;
        downloadImages(image);

    }else if (content_type1.equals("button"))
    {
        Button button = (Button) getLayoutInflater().inflate(R.layout.button_template,null);
        LinearLayout.LayoutParams layoutParams = new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT);
        layoutParams.gravity = Gravity.CENTER_HORIZONTAL;
        button.setLayoutParams(layoutParams);
        web_linearLayout.addView(button);

        final WebView webView1;
        webView1 = new WebView(MainActivity.this);
        webView1.setLayoutParams(new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT));
        web_linearLayout.addView(webView1);

        button.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view)
            {
                Document doc = Jsoup.parse(html_open);
                doc.append(content1);
                doc.append(html_close);
                webView1.getSettings().setJavaScriptEnabled(true);
                webView1.getSettings().setAllowFileAccess(true);
                webView1.getSettings().setLoadsImagesAutomatically(true);
                webView1.loadDataWithBaseURL("", String.valueOf(doc), "text/html", "utf-8", "");

            }
        });

    }
    if (content_type2.equals("text"))
    {

        if (content1.equals("NA"))
        {
            Log.d("Content2 blank","content blank");
        }else
        {
            WebView webView1;
            webView1 = new WebView(this);
            webView1.setLayoutParams(new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT));
            web_linearLayout.addView(webView1);

            Document doc = Jsoup.parse(html_open);
            doc.append(content2);
            doc.append(html_close);
            webView1.getSettings().setJavaScriptEnabled(true);
            webView1.getSettings().setAllowFileAccess(true);
            webView1.getSettings().setLoadsImagesAutomatically(true);
            webView1.loadDataWithBaseURL("", String.valueOf(doc), "text/html", "utf-8", "");
        }


    }else if (content_type2.equals("image"))
    {

        String image =  image_url+content2;
        downloadImages(image);


    }else if (content_type2.equals("button"))
    {
        Button button = (Button) getLayoutInflater().inflate(R.layout.button_template,null);
        LinearLayout.LayoutParams layoutParams = new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT);
        layoutParams.gravity = Gravity.CENTER_HORIZONTAL;
        button.setLayoutParams(layoutParams);
        web_linearLayout.addView(button);

        final WebView webView1;
        webView1 = new WebView(MainActivity.this);
        webView1.setLayoutParams(new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT));
        web_linearLayout.addView(webView1);

        button.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view)
            {
                Document doc = Jsoup.parse(html_open);
                doc.append(content2);
                doc.append(html_close);
                webView1.getSettings().setJavaScriptEnabled(true);
                webView1.getSettings().setAllowFileAccess(true);
                webView1.getSettings().setLoadsImagesAutomatically(true);
                webView1.loadDataWithBaseURL("", String.valueOf(doc), "text/html", "utf-8", "");

            }
        });

    }

    if (content_type3.equals("text"))
    {
        if (content1.equals("NA"))
        {
            Log.d("Content3 blank","content blank");
        }else
        {
            WebView webView1;
            webView1 = new WebView(this);
            webView1.setLayoutParams(new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT));
            web_linearLayout.addView(webView1);

            Document doc = Jsoup.parse(html_open);
            doc.append(content3);
            doc.append(html_close);
            webView1.getSettings().setJavaScriptEnabled(true);
            webView1.getSettings().setAllowFileAccess(true);
            webView1.getSettings().setLoadsImagesAutomatically(true);
            webView1.loadDataWithBaseURL("", String.valueOf(doc), "text/html", "utf-8", "");

        }


    }else if (content_type3.equals("image"))
    {
        String image =  image_url+content3;
        downloadImages(image);



    }else if (content_type3.equals("button"))
    {
        Button button = (Button) getLayoutInflater().inflate(R.layout.button_template,null);
        LinearLayout.LayoutParams layoutParams = new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT);
        layoutParams.gravity = Gravity.CENTER_HORIZONTAL;
        button.setLayoutParams(layoutParams);
        web_linearLayout.addView(button);

        final WebView webView1;
        webView1 = new WebView(MainActivity.this);
        webView1.setLayoutParams(new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT));
        web_linearLayout.addView(webView1);

        button.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view)
            {
                Document doc = Jsoup.parse(html_open);
                doc.append(content3);
                doc.append(html_close);
                webView1.getSettings().setJavaScriptEnabled(true);
                webView1.getSettings().setAllowFileAccess(true);
                webView1.getSettings().setLoadsImagesAutomatically(true);
                webView1.loadDataWithBaseURL("", String.valueOf(doc), "text/html", "utf-8", "");

            }
        });

    }
    if (content_type4.equals("text"))
    {
        if (content1.equals("NA"))
        {
            Log.d("Content4 blank","content blank");
        }else
        {

            WebView webView1;
            webView1 = new WebView(this);
            webView1.setLayoutParams(new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT));
            web_linearLayout.addView(webView1);

            Document doc = Jsoup.parse(html_open);
            doc.append(content4);
            doc.append(html_close);
            webView1.getSettings().setJavaScriptEnabled(true);
            webView1.getSettings().setAllowFileAccess(true);
            webView1.getSettings().setLoadsImagesAutomatically(true);
            webView1.loadDataWithBaseURL("", String.valueOf(doc), "text/html", "utf-8", "");
        }

    }else if (content_type4.equals("image"))
    {

        String image =  image_url+content4;
        downloadImages(image);


    }else if (content_type4.equals("button"))
    {
        Button button = (Button) getLayoutInflater().inflate(R.layout.button_template,null);
        LinearLayout.LayoutParams layoutParams = new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT);
        layoutParams.gravity = Gravity.CENTER_HORIZONTAL;
        button.setLayoutParams(layoutParams);
        web_linearLayout.addView(button);

        final WebView webView1;
        webView1 = new WebView(MainActivity.this);
        webView1.setLayoutParams(new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT));
        web_linearLayout.addView(webView1);

        button.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view)
            {
                Document doc = Jsoup.parse(html_open);
                doc.append(content4);
                doc.append(html_close);
                webView1.getSettings().setJavaScriptEnabled(true);
                webView1.getSettings().setAllowFileAccess(true);
                webView1.getSettings().setLoadsImagesAutomatically(true);
                webView1.loadDataWithBaseURL("", String.valueOf(doc), "text/html", "utf-8", "");

            }
        });

    }
    if (content_type5.equals("text"))
    {
        if (content1.equals("NA"))
        {
            Log.d("Content3 blank","content blank");
        }else
        {

            WebView webView1;
            webView1 = new WebView(this);
            webView1.setLayoutParams(new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT));
            web_linearLayout.addView(webView1);

            Document doc = Jsoup.parse(html_open);
            doc.append(content5);
            doc.append(html_close);
            webView1.getSettings().setJavaScriptEnabled(true);
            webView1.getSettings().setAllowFileAccess(true);
            webView1.getSettings().setLoadsImagesAutomatically(true);
            webView1.loadDataWithBaseURL("", String.valueOf(doc), "text/html", "utf-8", "");
        }

    }else if (content_type5.equals("image"))
    {
        String image =  image_url+content5;
        downloadImages(image);

    }
    else if (content_type5.equals("button"))
    {
        Button button = (Button) getLayoutInflater().inflate(R.layout.button_template,null);
        LinearLayout.LayoutParams layoutParams = new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT);
        layoutParams.gravity = Gravity.CENTER_HORIZONTAL;
        button.setLayoutParams(layoutParams);
        web_linearLayout.addView(button);

        final WebView webView1;
        webView1 = new WebView(MainActivity.this);
        webView1.setLayoutParams(new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT));
        web_linearLayout.addView(webView1);

        button.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view)
            {

                Document doc = Jsoup.parse(html_open);
                doc.append(content5);
                doc.append(html_close);
                webView1.getSettings().setJavaScriptEnabled(true);
                webView1.getSettings().setAllowFileAccess(true);
                webView1.getSettings().setLoadsImagesAutomatically(true);
                webView1.loadDataWithBaseURL("", String.valueOf(doc), "text/html", "utf-8", "");

            }
        });

    }
    if (content_type6.equals("text"))
    {
        if (content1.equals("NA"))
        {
            Log.d("Content3 blank","content blank");
        }else
        {

            WebView webView1;
            webView1 = new WebView(this);
            webView1.setLayoutParams(new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT));
            web_linearLayout.addView(webView1);

            Document doc = Jsoup.parse(html_open);
            doc.append(content6);
            doc.append(html_close);
            webView1.getSettings().setJavaScriptEnabled(true);
            webView1.getSettings().setAllowFileAccess(true);
            webView1.getSettings().setLoadsImagesAutomatically(true);
            webView1.loadDataWithBaseURL("", String.valueOf(doc), "text/html", "utf-8", "");
        }

    }else if (content_type6.equals("image"))
    {

        String image =  image_url+content6;
        downloadImages(image);


    }else if (content_type6.equals("button"))
    {
        Button button = (Button) getLayoutInflater().inflate(R.layout.button_template,null);
        LinearLayout.LayoutParams layoutParams = new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT);
        layoutParams.gravity = Gravity.CENTER_HORIZONTAL;
        button.setLayoutParams(layoutParams);
        web_linearLayout.addView(button);

        final WebView webView1;
        webView1 = new WebView(MainActivity.this);
        webView1.setLayoutParams(new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT));
        web_linearLayout.addView(webView1);

        button.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view)
            {
                Document doc = Jsoup.parse(html_open);
                doc.append(content6);
                doc.append(html_close);
                webView1.getSettings().setJavaScriptEnabled(true);
                webView1.getSettings().setAllowFileAccess(true);
                webView1.getSettings().setLoadsImagesAutomatically(true);
                webView1.loadDataWithBaseURL("", String.valueOf(doc), "text/html", "utf-8", "");

            }
        });

    }
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. \$\endgroup\$ – Jamal Aug 27 '16 at 7:29
4
\$\begingroup\$

I am sure there is a better way to do things instead of using the if-else statements.

There certainly is. Not only is your current code quite difficult to read, it is also difficult to maintain. What if a seventh content type is added? Just copy-pasting isn't a good idea here, as it can quickly introduce bugs (which probably already happened, see below). It will also make it very difficult to see if the content types are actually all treated equally.

The first thing you want to do is introduce a new function to get rid of the duplication:

private void handleContentType(String contentType, String content, String content1) {
    if (contentType.equals("text"))
    {

        if (content1.equals("NA"))
        {
            // log stuff
        }else
        {
            // do stuff with content
        }


    }else if (contentType.equals("image"))
    {

        // do stuff with content


    }else if (contentType.equals("button"))
    {
        // do stuff with content

    }
}

I've given the function a rather generic name for now, which isn't recommended in practice.

I have no idea why content1 is used in all cases, I assume it's a copy-paste bug, but I've left it in in case it isn't.

Now you can easily use this function for all your content types:

handleContentType(content_type1, content1, content1);
handleContentType(content_type1, content2, content1);
[...]

Even better, you could pass on a list and iterate over that, meaning that your function now wouldn't care how many content types there may be.

You could also convert your if into a switch, which makes sense here:

private void handleContentType(String contentType, String content, String content1) {
    switch(contentType) {
        case "text":
            // handle the text
            break;
        case "image":
            // handle the image
            break;
        case "button":
            // handle the button
            break;
        default:
            // handle the default     
    }
}

Misc

  • it would probably make sense to put contentX and content_typeX into some sort of object, as they obviously belong together.
  • You should try to follow common Java standards (camelCase instead of snake_case, opening curly bracket on the same line, etc).
  • You could benefit from introducing a lot more functions, eg for the webview code.
  • Your naming could use some work. Why is it webView1? Is there a webView2? Numbers in variables generally point to a bad design. Most of the time, it suggests that a list should be used. If a list is not possible, it still suggests bad naming habits.
\$\endgroup\$
2
\$\begingroup\$

First things first, you do not follow a consistent code formatting throughout your code, i.e your method declaration has the opening brace { at the end of the line, while the if statements have them in a separate line. Also, sometimes you add blank spaces in between arguments, sometimes you don't.

Next, passing 12 arguments to a single method is IMO too much. A user who has to work with your API has to somehow remember what argument to pass at which position. Besides that, you pass actually 6 tuples of arguments to the method. In addition to that, the handling of the 3 cases for each of the 6 tuples is identical -> Don't repeat yourself (DRY)!

You can start to refactor your code by extracting the common code and place it into a new method i.e. handleContent(String content, String content_type) and invoke this method for each passed tuple inside your web_content(...) method.

As content_type seems to be something constant - either text, image or button - it could make sense to define them in a ContentType enum. This enum defines each of the three content types and their work that should be performed on a passed content. This can be achieved via an inlined lambda expression.

The implementation of the enum can look like this:

private enum ContentType {

    TEXT(content -> {
        WebView webView = new WebView(this);
        webView.setLayoutParams(
            new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, 
                                          LinearLayout.LayoutParams.WRAP_CONTENT));
        web_linearLayout.addView(webView);

        if (content.equals("NA")) {
            Log.d("Content1 blank", "content blank");
        } else {
            Document doc = Jsoup.parse(html_open);
            doc.append(content);
            doc.append(html_close);
            webView.getSettings().setJavaScriptEnabled(true);
            webView.getSettings().setAllowFileAccess(true);
            webView.getSettings().setLoadsImagesAutomatically(true);
            webView.loadDataWithBaseURL("", String.valueOf(doc), "text/html", "utf-8", "");
        }
    }),
    IMAGE(content -> {
        String image = image_url + content;
        downloadImages(image);
    }),
    BUTTON(content -> {
        Button button = (Button) getLayoutInflater().inflate(R.layout.button_template, null);
        LinearLayout.LayoutParams layoutParams = 
            new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, 
                                          LinearLayout.LayoutParams.WRAP_CONTENT);
        layoutParams.gravity = Gravity.CENTER_HORIZONTAL;
        button.setLayoutParams(layoutParams);
        web_linearLayout.addView(button);

        WebView webView = new WebView(MainActivity.this);
        webView.setLayoutParams(
            new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, 
                                          LinearLayout.LayoutParams.WRAP_CONTENT));
        web_linearLayout.addView(webView);

        button.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view)
            {
                Document doc = Jsoup.parse(html_open);
                doc.append(content);
                doc.append(html_close);
                webView.getSettings().setJavaScriptEnabled(true);
                webView.getSettings().setAllowFileAccess(true);
                webView.getSettings().setLoadsImagesAutomatically(true);
                webView.loadDataWithBaseURL("", String.valueOf(doc), "text/html", "utf-8", "");

            }
        });
    });

    private Consumer<String> work;

    ContentType(Consumer<String> work) {
        this.work = work;
    }

    public void handleContent(String content) {
        this.work.accept(content);
    }
}

To look up the appropriate content type via the content_type string you should be able to do the following:

ContentType.valueOf(content_type.toUpperCase()).handleContent(content);

Note that the enum should be defined within your class so the web_linearLayout field or downloadImages(...) method are accessible inside the anonymous inner classes.

If an enum is not an option for you (for whatever reason), you can also use a Map<String, Consumer<String>> map which takes care of the content type handling.

The key of the map is your content_type value while the value is the same consumer the enum defined above:

public YourClass {

    private Map<String, Consumer<String>> contentTypes = new HashMap<>();

    public YourClass() {
        contentTypes.put("text", content -> {
            WebView webView = new WebView(this);
            webView.setLayoutParams(
            new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, 
                                          LinearLayout.LayoutParams.WRAP_CONTENT));
            web_linearLayout.addView(webView);

            if (content.equals("NA")) {
                Log.d("Content1 blank", "content blank");
            } else {
                Document doc = Jsoup.parse(html_open);
                doc.append(content);
                doc.append(html_close);
                webView.getSettings().setJavaScriptEnabled(true);
                webView.getSettings().setAllowFileAccess(true);
                webView.getSettings().setLoadsImagesAutomatically(true);
                webView.loadDataWithBaseURL("", String.valueOf(doc), "text/html", "utf-8", "");
            }
        });
        ...
    }

    ...
}

The lookup of the content type simplifies now to this statement:

this.contentTypes.get(content_type).accept(content);

If content and content_type always go hand in hand, it might also be worth a consideration to put them into an own class.

Note that the code presented here is untested as you also provided code that is not testable for us either due to the missing context (dependent fields, methods, ...).


Update:

As I had to learn that Android does not yet support all Java 8 features and I don't know which Android version you are using, chances are that you can't yet use lambda expressions directly.

In that case you can simply define a simple interface like

public interface ContentTypeHandler {
    void handleContent(String content);
}

instead of using something like

content -> { ... }

you now have to be a bit more descriptive. The enum or map declaration will have to change to

private enum ContentType {
    TEXT(new ContentTypeHandler() {
        @Overright
        public void handleContent(String content) {
            // stuff to execute
            WebView webView = new WebView(this);
            ...
        }
    });
}

or in case of a map:

contentTypes.put("text", new ContentTypeHandler() {
    @Overright
    public void handleContent(String content) {
        // stuff to execute
        WebView webView = new WebView(this);
        ...
    }
});

Due to the custom interface, the invocation now has to use .handleContent(content) instead of .accept(content) which might add a bit more semantics.

Further note: I missed that you check each text based content type against content1, my bad. If this is intended you need to change the content type handling to accept two arguments of course and also pass contentN as well as content1 to the expression as already suggested by @tim in his post.

\$\endgroup\$

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