2
\$\begingroup\$

I have cardView in my android app with three child component. I set the background color for each components. Based on card INDEX I reset the background colors individually.

If it's just one cardView then its not a problem, but I have to include 16 (16 more card indexes) more cardViews exactly the same structure but only the id will be different to the view and child component. How could it be done ?

 public void cardView_type_Colors(int card_idx, String colors){

    if (card_idx == 0){
        if (colors.equals("YELLOW")){
            color_cardView1 =
                    color_cardViewDisplayName1 =
                            color_cardViewDisplayNumber1 =
                                    color_cardViewDisplayTown1 =
                                            color_btncardView1 = R.color.LimeYellow;
            setCardView_text_colors(0,"BLACK");

        } else if (colors.equals("PALEGREEN")){
            color_cardView1 =
                    color_cardViewDisplayName1 =
                            color_cardViewDisplayNumber1 =
                                    color_cardViewDisplayTown1 =
                                            color_btncardView1 = R.color.PaleGreen;
            setCardView_text_colors(0,"WHITE");
        } else if (colors.equals("ORANGE")){
            color_cardView1 =
                    color_cardViewDisplayName1 =
                            color_cardViewDisplayNumber1 =
                                    color_cardViewDisplayTown1 =
                                            color_btncardView1 = R.color.Orange;
            setCardView_text_colors(0,"WHITE");
        } else if (colors.equals("RED")){
            color_cardView1 =
                    color_cardViewDisplayName1 =
                            color_cardViewDisplayNumber1 =
                                    color_cardViewDisplayTown1 =
                                            color_btncardView1 = R.color.Red;
            setCardView_text_colors(0,"WHITE");
        } else if (colors.equals("DEFAULT")){
            color_cardView1 =
                    color_cardViewDisplayName1 =
                            color_cardViewDisplayNumber1 =
                                    color_cardViewDisplayTown1 =
                                            color_btncardView1 = R.color.LightGrey;
            setCardView_text_colors(0,"WHITE");
        } else if (colors.equals("DISABLE")){
            color_cardView1 =
                    color_cardViewDisplayName1 =
                            color_cardViewDisplayNumber1 =
                                    color_cardViewDisplayTown1 =
                                            color_btncardView1 = R.color.DisableColor;
            setCardView_text_colors(0,"BLACK");
        }

    } else if (card_idx == 1){
        if (colors.equals("YELLOW")){
            color_cardView2 =
                    color_cardViewDisplayName2 =
                            color_cardViewDisplayNumber2 =
                                    color_cardViewDisplayTown2 =
                                            color_btncardView2 = R.color.LimeYellow;
            setCardView_text_colors(1,"BLACK");
        } else if (colors.equals("PALEGREEN")){
            color_cardView2 =
                    color_cardViewDisplayName2 =
                            color_cardViewDisplayNumber2 =
                                    color_cardViewDisplayTown2 =
                                            color_btncardView2 = R.color.PaleGreen;
            setCardView_text_colors(1, "WHITE");
        } else if (colors.equals("ORANGE")){
            color_cardView2 =
                    color_cardViewDisplayName2 =
                            color_cardViewDisplayNumber2 =
                                    color_cardViewDisplayTown2 =
                                            color_btncardView2 = R.color.Orange;
            setCardView_text_colors(1,"WHITE");
        } else if (colors.equals("RED")){
            color_cardView2 =
                    color_cardViewDisplayName2 =
                            color_cardViewDisplayNumber2 =
                                    color_cardViewDisplayTown2 =
                                            color_btncardView2 = R.color.Red;
            setCardView_text_colors(1,"WHITE");
        } else if (colors.equals("DEFAULT")){
            color_cardView2 =
                    color_cardViewDisplayName2 =
                            color_cardViewDisplayNumber2 =
                                    color_cardViewDisplayTown2 =
                                            color_btncardView2 = R.color.LightGrey;
            setCardView_text_colors(1,"WHITE");
        } else if (colors.equals("DISABLE")){
            color_cardView2 =
                    color_cardViewDisplayName2 =
                            color_cardViewDisplayNumber2 =
                                    color_cardViewDisplayTown2 =
                                            color_btncardView2 = R.color.DisableColor;
            setCardView_text_colors(1,"BLACK");
        }
    }
}
\$\endgroup\$

closed as off-topic by tinstaafl, dfhwze, Dannnno, 409_Conflict, pacmaninbw Sep 14 at 14:53

This question appears to be off-topic. The users who voted to close gave these specific reasons:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – 409_Conflict, pacmaninbw
  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – tinstaafl, dfhwze, Dannnno
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ Why can't you use enum or R.color.XXX directly instead of a String? \$\endgroup\$ – bhathiya-perera Sep 12 at 12:32
  • \$\begingroup\$ Ofcourse I could use that. But how could that solve my problem ? \$\endgroup\$ – cantona_7 Sep 12 at 12:58
  • \$\begingroup\$ I think 16 cards problem can be fixed with some array and a class to store relevant information. Also if we use an enum for colors or directly use R.color we don't need to perform string match. \$\endgroup\$ – bhathiya-perera Sep 12 at 13:06
  • 1
    \$\begingroup\$ It would be better to know how you are using color_cardView1 and other things you are setting. Also can you change the title to represent the business requirement of included code so we can get some context. \$\endgroup\$ – bhathiya-perera Sep 12 at 13:06
3
\$\begingroup\$

I'll start this post off with a notice that my knowledge of the Java language isn't very high. However, I have a lot of experience with development in languages like C# and JavaScript so there are a few things I can help with. Most of my code below will be pseudo-code, but I did research more proper syntax and import statements to clean it up a bit.


Based on your post alone, it is hard to tell what is really going on which, is mostly due to the fact that you've got chained assignments, and your variable names are all very similar (this makes it hard to understand what each variable really is). I'm not quite sure why you have chained assignments here, but this is due to the lack of context. It does look very odd (and it could be my lack of knowledge in the Java language), because it looks you're assigning a Color to a String to a int to a String to what appears to be some sort of Control.

Create a Class

Without clarification, one suggestion I will make is to create a class for your Card objects. This will help to clarify what is going on a little better, and makes my upcoming suggestions much more feasible.

Card.java:

import java.awt.Color;
public class Card {
    public int id = 0;
    public int displayNumber = 0;
    public String displayName = "CARD";
    public String town = "Liverpool";
    public Color backgroundColor = Color.black;
    public Color foreColor = Color.white;
    public CardView view = null;
    public Card(int cardID, int num, String name, String town, Color bgColor) {
        id = cardID;
        displayNumber = num;
        displayName = name;
        town = town;
        backgroundColor = bgColor;
    }
}

I had to create an additional class called CardView since I'm not sure what that is, but assume it's a control. I just created an empty class to compile my initial testing.

CardView.java:

public class CardView { }

Again, without truly knowing what each of your variables are this is the best I can come up with and is purely based on your existing naming conventions. This class will help to make your code more readable since you now have consistent names for your variables.

Use a List

Lists are very useful objects, no matter the language you're in (I'd be really interested to see a language with a bad implementation of them). I heavily recommend using a list for this task since you can add all of your cards to it and then loop over it later when doing your assignments. For example, to create a list of Card objects:

List<Card> cards = new ArrayList<Card>();
cards.add(new Card(0, 0, "Card 1", Color.black));
cards.add(new Card(0, 0, "Card 2", Color.black));
cards.add(new Card(0, 0, "Card 3", Color.black));

Create Methods for Assigning Colors

This will help clean up a little since each method will do something very specific. For example, I'd recommend the following four methods:

  • applyCardColors(Card c, String backgroundColor)
    • Used to apply all relevant colors to a card.
  • applyBackgroundColor(Card c, String color)
    • Used to apply the background color.
  • determineAndApplyForeColor(Card c, String backgroundColor)
    • Used to determine the proper fore color and then apply it with the next method.
  • applyForeColor(Card c, String color)
    • Used to apply the proper fore color.

To some it may seem a little redundant, but it cleans up code and ensures better readability.

Use switch instead of if-else if-else

This one may not be recommended by all, but I believe it will help clean up the code since we've now broken your code down into individual methods. The ApplyBackgroundColor method has the largest switch structure, but this is due to the number of colors you can have.

public static void applyBackgroundColor(Card c, String color) {
    switch (color) {
        case "YELLOW": c.backgroundColor = Color.yellow; break;
        case "PALEGREEN": c.backgroundColor = Color.green; break;
        case "ORANGE": c.backgroundColor = Color.orange; break;
        //...
        default: c.backgroundColor = Color.black; break;
    }
}
public static void determineAndApplyForeColor(Card c, String backgroundColor) {
    switch (backgroundColor) {
        case "YELLOW":
        case "DISABLE": applyForeColor(c, "BLACK"); break;
        default: applyForeColor(c, "WHITE"); break;
    }
}
public static void applyForeColor(Card c, String color) {
    switch (color) {
        case "WHITE": c.foreColor = Color.white; break;
        default: c.foreColor = Color.black;
    }
}

This is easy to expand on in the future and prevents you from having incredibly large logical structures.

Loop over your Cards

Now that all of that ground work has been laid out, you can loop over your Card collection and apply colors that way:

for (int i = 0; i < cards.size(); i++) {
    Card c = cards.get(i);
    applyCardColors(c, cardColors[i]);
    System.out.print(c.displayName + ": " +
                     c.backgroundColor + ", " +
                     c.foreColor + "\n");
}

Of course you'll need that other method I mentioned above to keep things separated:

public static void applyCardColors(Card c, String backgroundColor) {
    applyBackgroundColor(c, backgroundColor);
    determineAndApplyForeColor(c, backgroundColor);
}

Use the enum Values

My heaviest recommendation is to use the predefined enum values that you're assigning so that you can prevent the switch structure and clean this code up even more:

Color[] cardColors = new Color[] { Color.yellow, Color.green, Color.red };
List<Card> cards = new ArrayList<Card>();
cards.add(new Card(0, 0, "Card 1", "London", Color.black));
cards.add(new Card(0, 0, "Card 2", "Paris", Color.black));
cards.add(new Card(0, 0, "Card 3", "Dubai", Color.black));

for (int i = 0; i < cards.size(); i++) {
  Card c = cards.get(i);
  c.backgroundColor = cardColors[i];
  applyForeColor(c, cardColors[i]);
  System.out.print(c.displayName + ": " +
                   c.backgroundColor + ", " +
                   c.foreColor + "\n");
}

The only part I had trouble with here was using the switch structure on the enum but this is due to my lack of knowledge in Java, I can do it as below in C#, but the online compiler I have didn't like it:

public static void applyForeColor(Card c, Color backgroundColor) {
    switch (backgroundColor) {
      case Color.yellow: c.foreColor = Color.black; break;
      default: c.foreColor = Color.white; break;
    }
}

Best of luck with your future endeavors!

\$\endgroup\$
  • \$\begingroup\$ Thank you for the answer. It is really helpful. I will implement it :) \$\endgroup\$ – cantona_7 Sep 13 at 8:02
  • \$\begingroup\$ Very good answer. However, It's a generally a Java convention to name members and methods in lowerCamelCase. \$\endgroup\$ – bhathiya-perera Sep 13 at 12:48
  • \$\begingroup\$ @bhathiya-perera thanks! I’ll update the post later today to reflect that! \$\endgroup\$ – PerpetualJ Sep 13 at 12:49

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