5
\$\begingroup\$

I have created a Grid out of using the new concept of List in Kotlin. I'm open to any feed back to how this code could be improved. It functions as expected and I'm happy with the results.


import android.os.Bundle
import androidx.activity.ComponentActivity
import androidx.activity.compose.setContent
import androidx.compose.foundation.Image
import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.aspectRatio
import androidx.compose.foundation.layout.fillMaxHeight
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.layout.width
import androidx.compose.foundation.lazy.grid.GridCells
import androidx.compose.foundation.lazy.grid.LazyHorizontalGrid
import androidx.compose.foundation.lazy.grid.LazyVerticalGrid
import androidx.compose.foundation.lazy.grid.items
import androidx.compose.material3.Card
import androidx.compose.material3.Icon
import androidx.compose.material3.MaterialTheme
import androidx.compose.material3.Surface
import androidx.compose.material3.Text
import androidx.compose.runtime.Composable
import androidx.compose.ui.Modifier
import androidx.compose.ui.platform.LocalContext
import androidx.compose.ui.res.painterResource
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.tooling.preview.Preview
import androidx.compose.ui.unit.dp
import com.example.grid.ui.theme.DataSource
import com.example.grid.ui.theme.DataSource.topics
import com.example.grid.ui.theme.GridTheme
import com.example.grid.ui.theme.Topic


class MainActivity : ComponentActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContent {
            GridTheme {
                // A surface container using the 'background' color from the theme
                Surface(
                    modifier = Modifier.fillMaxSize(),
                    color = MaterialTheme.colorScheme.background
                ) {
                    GridApp()
                }
            }
        }
    }
}

@Composable
fun GridApp() {
    GridList(gridList = DataSource.topics, modifier = Modifier)

}

@Composable
fun Topics(topic: Topic, modifier: Modifier = Modifier){
    val number = topic.numbers.toString()
    Card{

Row {


            Image(painter = painterResource(topic.image), contentDescription = stringResource(topic.name),
                modifier
                .size(68.dp)
               .fillMaxHeight())
       Column(modifier.width(120.dp)){Text(LocalContext.current.getString(topic.name),
            modifier = modifier
                .padding(horizontal = 16.dp)
                .padding(top = 16.dp)
                .padding(bottom = 8.dp),
           style = MaterialTheme.typography.bodyMedium)

Row(modifier = Modifier, horizontalArrangement = Arrangement.End){
    Icon(painterResource(R.drawable.ic_grain), contentDescription = "few dots",
        Modifier
            .padding(start = 16.dp)
            .padding(end = 8.dp))
    Text((number), modifier)

            }
       }

}
    }
}


@Composable
fun GridList(gridList: List<Topic>, modifier: Modifier = Modifier.padding(8.dp)){
    LazyVerticalGrid(columns = GridCells.Fixed(2), verticalArrangement = Arrangement.spacedBy(8.dp), horizontalArrangement = Arrangement.spacedBy(8.dp), modifier = modifier.padding(8.dp)) {
        items(topics) { topic ->
            Topics(topic = topic)
        }

    }




}




@Preview
@Composable
private fun showTopicGrid() {
    Topics(Topic(R.string.film, 321, R.drawable.film))
}

enter image description here

\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

Looks quite good already, I haven't found any serious issues. The are some minor improvements that I would suggest, though.

Small bugs

  1. I'm not sure how your data source looks like, but you import com.example.grid.ui.theme.DataSource.topics and use it in GridList as parameter for the items builder. That doesn't seem right, since there is already an unused List<Topic> parameter for this composable. I would rename that parameter from gridList to topcis and remove the import statement.
  2. It is unclear to me what the modifier parameter of Topics should modify. The expectation of what such a modifier does would be to modify the entirety of that composable, to change its size, for example. You, however, just apply it to an Image, a Column and two different Texts. That doesn't seem right. I do not think there even should be a modifier parameter for Topics.
  3. GridList has a modifier parameter with the default Modifier.padding(8.dp). Modifier parameters always should have the default Modifier to harmonize the behaviour with other composables. Since you apply that padding later on anyways, correcting the default value won't have any undesired side effects.

Minor improvements

  1. You sometimes use LocalContext.current.getString() and sometimes you use stringResource(). It should always be the latter.
  2. The string literal "few dots" should be replaced by a string resource.
  3. In Topics the Text composable that displays the topic name uses the padding modifier three times in a row which can be condensed to a single padding(start = 16.dp, top = 16.dp, end = 16.dp, bottom = 8.dp). The same applies to the Icon a bit down below where the padding can be condensed to padding(start = 16.dp, end = 8.dp).
  4. Also in the Topics composable, the size of the Image is set to a square with size(68.dp) and then the height is changed to fillMaxHeight(). It would be a bit more clear to only set width(68.dp) (instead of size) because the height component won't matter anyways.
  5. To increase readbility, functions with multiple parameters should have each parameter on a seperate line. This applies to function declarations as well as function calls. The last parameter should always have a trailing comma (the same as the other parameters), even if it's optional for the last parameter. Multiline function calls should always use parameter names.
  6. The Text that displays the number is wrapped unnecessarily in parentheses.
  7. The composable showTopicGrid should start with a capital letter.
  8. There are some unused imports and some indentation issues that Android Studio can automatically correct for you if you press Ctrl+Alt+O and Ctrl+Alt+L respectively.
  9. There are a lot of superflous empty lines that can be removed.

I have incorporated all these changes into your code. It now looks like this (I stripped the imports):

class MainActivity : ComponentActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContent {
            GridTheme {
                // A surface container using the 'background' color from the theme
                Surface(
                    modifier = Modifier.fillMaxSize(),
                    color = MaterialTheme.colorScheme.background,
                ) {
                    GridApp()
                }
            }
        }
    }
}

@Composable
fun GridApp() {
    GridList(
        topics = DataSource.topics,
    )
}

@Composable
fun Topics(
    topic: Topic,
) {
    val number = topic.numbers.toString()
    Card {
        Row {
            Image(
                painter = painterResource(topic.image),
                contentDescription = stringResource(topic.name),
                modifier = Modifier
                    .width(68.dp)
                    .fillMaxHeight(),
            )
            Column(
                modifier = Modifier.width(120.dp),
            ) {
                Text(
                    text = stringResource(topic.name),
                    modifier = Modifier
                        .padding(start = 16.dp, top = 16.dp, end = 16.dp, bottom = 8.dp),
                    style = MaterialTheme.typography.bodyMedium,
                )

                Row(
                    modifier = Modifier,
                    horizontalArrangement = Arrangement.End,
                ) {
                    Icon(
                        painter = painterResource(R.drawable.ic_grain),
                        contentDescription = stringResource(R.string.ic_grain_description),
                        modifier = Modifier
                            .padding(start = 16.dp, end = 8.dp),
                    )
                    Text(
                        text = number,
                        modifier = Modifier,
                    )
                }
            }
        }
    }
}

@Composable
fun GridList(
    topics: List<Topic>,
    modifier: Modifier = Modifier,
) {
    LazyVerticalGrid(
        columns = GridCells.Fixed(2),
        verticalArrangement = Arrangement.spacedBy(8.dp),
        horizontalArrangement = Arrangement.spacedBy(8.dp),
        modifier = modifier.padding(8.dp),
    ) {
        items(topics) { topic ->
            Topics(topic = topic)
        }
    }
}

@Preview
@Composable
private fun ShowTopicGrid() {
    Topics(Topic(R.string.film, 321, R.drawable.film))
}

Final thoughts

As your app grows in size you will have more and more data sources that need to be transformed into UI elements. This can get quite complex really fast. The canonical recommendation is to structure your app in different layers that handle the different aspects independent from each other.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you so much for taking the time to review my work! I'll definitely keep this in mind as I'm starting a new project soon! \$\endgroup\$ Commented Mar 2 at 13:02

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