3
\$\begingroup\$

I have finished creating an adapter class which displays characters' info. When I looked back and reviewed my own code, I think the code fragment below somehow could be broken into smaller methods.

...
private static class ViewHolder {
    ImageView imgIconState;
    ImageView imgCharacterStatus;
    TextView txtName;
}

@Override
public View getView(int index, View view, ViewGroup viewGroup) {
    ViewHolder characterProfile;
    if (view == null) {
        LayoutInflater inflater = (LayoutInflater) mContext.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
        view = inflater.inflate(R.layout.character_item, null);
        characterProfile = new ViewHolder();
        characterProfile.imgIconState = (ImageView) view.findViewById(R.id.icon_state);
        characterProfile.imgCharacterStatus = (ImageView) view.findViewById(R.id.character_state);
        characterProfile.txtName = (TextView) view.findViewById(R.id.name);
        view.setTag(characterProfile);
    } else {
        characterProfile = (ViewHolder) view.getTag();
    }

    Character character =  mLeBluetoothLightsList.get(index);
    characterProfile.imgIconState.setImageResource(mIconStateRrs[character.getIconStatus()]);
    characterProfile.imgCharacterStatus.setImageResource(character.isAlive() ? R.drawable.alive : R.drawable.dead);
    characterProfile.txtName.setText(character.getCharacterName());

    if (Config.IS_DEBUG) {
        Log.i(mClassName, "getView - Index: " + index
                + "| Icon status: " + character.getIconStatusForLog()
                + "| Character status: " + character.isAliveForLog()
                + "| Name: " + character.getCharacterName());
    }

    return view;
}
...

I am attempting to write a 3 to 10 lines function, but I find it hard to break it smaller.

Should I refactor this or leave it as it is? If the answer is the former, please give me some advice.

\$\endgroup\$
  • 1
    \$\begingroup\$ Hey, welcome to Code Review!. This looks like a nice first question. \$\endgroup\$ – Graipher Jan 26 '17 at 8:06
1
\$\begingroup\$

I think this method is not that long that you really need to refactor it, but you could do something like

public View getView(int index, View view, ViewGroup viewGroup) {
    view = createViewIfNeeded(view)        
    final ViewHolder characterProfile = (ViewHolder) view.getTag();
    Character character =  mLeBluetoothLightsList.get(index);
    characterProfile.imgIconState.setImageResource(mIconStateRrs[character.getIconStatus()]);
    characterProfile.imgCharacterStatus.setImageResource(character.isAlive() ? R.drawable.alive : R.drawable.dead);
    characterProfile.txtName.setText(character.getCharacterName());
    logIfDebug(index, character);
    return view;
}

private static void logIfDebug(int index, Character character){
    if (Config.IS_DEBUG) {
        Log.i(mClassName, "getView - Index: " + index
                + "| Icon status: " + character.getIconStatusForLog()
                + "| Character status: " + character.isAliveForLog()
                + "| Name: " + character.getCharacterName());
    }
}

// You could extract createAndInitCharacterProfile from this method if you want
private View createViewIfNeeded(View view){
    if (view == null) {
        LayoutInflater inflater = (LayoutInflater) mContext.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
        view = inflater.inflate(R.layout.character_item, null);
        ViewHolder characterProfile = new ViewHolder();
        characterProfile.imgIconState = (ImageView) view.findViewById(R.id.icon_state);
        characterProfile.imgCharacterStatus = (ImageView) view.findViewById(R.id.character_state);
        characterProfile.txtName = (TextView) view.findViewById(R.id.name);
        view.setTag(characterProfile);
   }
   return view;
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer and sorry because of the late reply. By the way, I have two questions about this. ① Should I break from "final ViewHolder characterProfile = (ViewHolder) view.getTag();" to "characterProfile.txtName.setText(character.getCharacterName());" into another method, for example updateCharacterInfoViews() ? ② What if my characterProfile grows bigger, this time is not 3 views but 9 views, this means my getView() method will grow six times longer than it is at this time. Shall the version without refactoring acceptable ? \$\endgroup\$ – ZodiacLeo123 Jan 27 '17 at 0:58
  • 1
    \$\begingroup\$ @ZodiacLeo123: ① Depends on the readability. I would avoid to long chains of .dfg.fdgd.gf.fgd.gffg.get() and store it in a local variable as you did in your code. ② If it grows bigger I would extract it to a method which sets all the fields. You could do this right now, but the getView(...) method is very short, so it would not really improve readability. \$\endgroup\$ – MrSmith42 Jan 27 '17 at 13:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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