ronantakizawa/codereview-bench huggingface dataset:
Repository: android/compose-samples
File: Jetsurvey/app/src/main/java/com/example/compose/jetsurvey/survey/PhotoUriManager.kt (Kotlin)

Code before:
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package com.example.compose.jetsurvey.survey

import android.content.ContentValues
import android.content.Context
import android.net.Uri
import android.provider.MediaStore

/**
 * Manages the creation of photo Uris. The Uri is used to store the photos taken with camera.
 */
class PhotoUriManager(private val appContext: Context) {

    private val photoCollection by lazy {
        MediaStore.Images.Media.getContentUri(MediaStore.VOLUME_EXTERNAL_PRIMARY)
    }

    private val resolver by lazy { appContext.contentResolver }

    // Uri used to save photos taken with the camera
    var uri: Uri? = null
        get() = field
        private set(value) {
            field = value
        }

    fun buildNewUri(): Uri? {
        uri = resolver.insert(photoCollection, getPhotoDetails())
        return uri
    }
}

private fun getPhotoDetails() = ContentValues().apply {
    put(MediaStore.Images.Media.DISPLAY_NAME, generateFilename())
    put(MediaStore.Images.Media.MIME_TYPE, "image/jpeg")
}

/**
 * Create a unique file name based on the time the photo is taken
 */
private fun generateFilename() = "selfie-${System.currentTimeMillis()}.jpg"


Review comment (suggestion):
Should this be public? Seems strange that there are 2 exposed `Uri`s, this and that returned from `buildNewUri`

Code after:
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package com.example.compose.jetsurvey.survey

import android.content.ContentValues
import android.content.Context
import android.provider.MediaStore

/**
 * Manages the creation of photo Uris. The Uri is used to store the photos taken with camera.
 */
class PhotoUriManager(private val appContext: Context) {

    private val photoCollection by lazy {
        MediaStore.Images.Media.getContentUri(MediaStore.VOLUME_EXTERNAL_PRIMARY)
    }

    private val resolver by lazy { appContext.contentResolver }

    fun buildNewUri() = resolver.insert(photoCollection, buildPhotoDetails())

    private fun buildPhotoDetails() = ContentValues().apply {
        put(MediaStore.Images.Media.DISPLAY_NAME, generateFilename())
        put(MediaStore.Images.Media.MIME_TYPE, "image/jpeg")
    }

    /**
     * Create a unique file name based on the time the photo is taken
     */
    private fun generateFilename() = "selfie-${System.currentTimeMillis()}.jpg"
}
Repository: android/compose-samples
File: Jetchat/app/src/main/java/com/example/compose/jetchat/MainViewModel.kt (Kotlin)

Code before:
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package com.example.compose.jetchat

import androidx.lifecycle.ViewModel
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.StateFlow

/**
 * Used to communicate between screens.
 */
class MainViewModel : ViewModel() {

    private val _drawerShouldBeOpened = MutableStateFlow(false)
    val drawerShouldBeOpened: StateFlow<Boolean> = _drawerShouldBeOpened

    fun openDrawer() {
        _drawerShouldBeOpened.value = true
    }
    fun resetOpenDrawer() {
        _drawerShouldBeOpened.value = false
    }
}


Review comment (suggestion):
Find this name a bit confusing. What about

```suggestion
    fun resetOpenDrawerAction() {
```

Code after:
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package com.example.compose.jetchat

import androidx.lifecycle.LiveData
import androidx.lifecycle.MutableLiveData
import androidx.lifecycle.ViewModel

/**
 * Used to communicate between screens.
 */
class MainViewModel : ViewModel() {

    private val _drawerShouldBeOpened = MutableLiveData(false)
    val drawerShouldBeOpened: LiveData<Boolean> = _drawerShouldBeOpened

    fun openDrawer() {
        _drawerShouldBeOpened.value = true
    }
    fun resetOpenDrawerAction() {
        _drawerShouldBeOpened.value = false
    }
}
Repository: android/compose-samples
File: Owl/app/src/androidTest/java/com/example/owl/ui/NavigationTest.kt (Kotlin)

Code before:
        // Given the app in the courses screen
        startActivity(MainDestinations.COURSES_ROUTE)

        // Navigate to the first course
        composeTestRule.onNode(
            hasContentDescription(getFeaturedCourseLabel()).and(hasSubstring(courses.first().name))
        ).performClick()

        // Assert navigated to the course details
        composeTestRule.onNodeWithSubstring(getCourseDesc().take(15)).assertExists()
    }

    @Test
    fun coursesToDetailAndBack() {
        coursesToDetail()
        composeTestRule.runOnUiThread {
            activity.onBackPressed()
        }

        // The first course should be shown
        composeTestRule.onNodeWithSubstring(courses.first().name).assertExists()
    }

    private fun getOnboardingFabLabel(): String {
        return InstrumentationRegistry.getInstrumentation().targetContext.resources
            .getString(R.string.label_continue_to_courses)
    }

    private fun getFeaturedCourseLabel(): String {
        return InstrumentationRegistry.getInstrumentation().targetContext.resources
            .getString(R.string.featured)
    }

    private fun getCourseDesc(): String {
        return InstrumentationRegistry.getInstrumentation().targetContext.resources
            .getString(R.string.course_desc)
    }
}


Review comment (suggestion):
- Remove the activity property
- Remove the onActivity
- Replace `activity` with `composeTestRule.activity`
- Replace `InstrumentationRegistry.getInstrumentation().targetContext.resources` with `composeTestRule.activity.resources`

Code after:
        composeTestRule.onNode(
            hasContentDescription(getFeaturedCourseLabel()).and(hasSubstring(courses.first().name))
        ).performClick()

        // Assert navigated to the course details
        composeTestRule.onNodeWithSubstring(getCourseDesc().take(15)).assertExists()
    }

    @Test
    fun coursesToDetailAndBack() {
        coursesToDetail()
        composeTestRule.runOnUiThread {
            composeTestRule.activity.onBackPressed()
        }

        // The first course should be shown
        composeTestRule.onNodeWithSubstring(courses.first().name).assertExists()
    }

    private fun getOnboardingFabLabel(): String {
        return composeTestRule.activity.resources.getString(R.string.label_continue_to_courses)
    }

    private fun getFeaturedCourseLabel(): String {
        return composeTestRule.activity.resources.getString(R.string.featured)
    }

    private fun getCourseDesc(): String {
        return composeTestRule.activity.resources.getString(R.string.course_desc)
    }
}
Repository: android/compose-samples
File: Owl/app/src/main/java/com/example/owl/ui/OwlApp.kt (Kotlin)

Code before:
        ) {
            tabs.forEach { tab ->
                BottomNavigationItem(
                    icon = { Icon(painterResource(tab.icon), contentDescription = null) },
                    label = { Text(stringResource(tab.title).toUpperCase()) },
                    selected = currentRoute == tab.route,
                    onClick = {
                        if (tab.route != currentRoute) {
                            navController.navigate(tab.route) {
                                popUpTo = navController.graph.startDestination
                                launchSingleTop = true
                            }
                        }
                    },
                    alwaysShowLabel = false,
                    selectedContentColor = MaterialTheme.colors.secondary,
                    unselectedContentColor = LocalContentColor.current,
                    modifier = Modifier.navigationBarsPadding()
                )
            }
        }
    }
}

@Composable
fun OwlScaffold(
    bottomBar: @Composable () -> Unit,
    content: @Composable (Modifier) -> Unit
) {
    Scaffold(
        backgroundColor = MaterialTheme.colors.primarySurface,
        bottomBar = bottomBar
    ) { innerPadding ->
        val modifier = Modifier.padding(innerPadding)
        content(modifier)
    }
}


Review comment (suggestion):
Just use `Scaffold`? The only thing this is really doing is wrapping the content padding into a modifier.

Code after:
    if (currentRoute in routes) {
        BottomNavigation(
            Modifier.navigationBarsHeight(additional = 56.dp)
        ) {
            tabs.forEach { tab ->
                BottomNavigationItem(
                    icon = { Icon(painterResource(tab.icon), contentDescription = null) },
                    label = { Text(stringResource(tab.title).toUpperCase()) },
                    selected = currentRoute == tab.route,
                    onClick = {
                        if (tab.route != currentRoute) {
                            navController.navigate(tab.route) {
                                popUpTo = navController.graph.startDestination
                                launchSingleTop = true
                            }
                        }
                    },
                    alwaysShowLabel = false,
                    selectedContentColor = MaterialTheme.colors.secondary,
                    unselectedContentColor = LocalContentColor.current,
                    modifier = Modifier.navigationBarsPadding()
                )
            }
        }
    }
}
Repository: android/compose-samples
File: JetNews/app/src/androidTest/java/com/example/jetnews/HomeScreenTests.kt (Kotlin)

Code before:
        val snackbarHostState = SnackbarHostState()
        composeTestRule.setContent {
            JetnewsTheme {
                val scaffoldState = rememberScaffoldState(snackbarHostState = snackbarHostState)

                // When the Home screen receives data with an error
                HomeScreen(
                    posts = UiState(exception = IllegalStateException()),
                    favorites = emptySet(),
                    onToggleFavorite = {},
                    onRefreshPosts = {},
                    onErrorDismiss = {},
                    navigateToArticle = {},
                    openDrawer = {},
                    scaffoldState = scaffoldState
                )
            }
        }

        // Then the first message received in the Snackbar is an error message
        val snackbarText = InstrumentationRegistry.getInstrumentation()
            .targetContext.resources.getString(R.string.load_error)
        runBlocking {
            // snapshotFlow converts a State to a Kotlin Flow so we can observe it
            // wait for the first a non-null `currentSnackbarData`
            snapshotFlow { snackbarHostState.currentSnackbarData }.filterNotNull().first()
            composeTestRule.onNodeWithText(snackbarText).assertExists()
        }
    }
}


Review comment (suggestion):
This might work every time, but it's dangerous because you don't know how much time it's going to pass from the `first() ` to the onNodeWithText and the snackbar could be dismissed (although I doubt it).

A super solid test would assert against the value of `first()`

Code after:
        val snackbarHostState = SnackbarHostState()
        composeTestRule.setContent {
            JetnewsTheme {
                val scaffoldState = rememberScaffoldState(snackbarHostState = snackbarHostState)

                // When the Home screen receives data with an error
                HomeScreen(
                    posts = UiState(exception = IllegalStateException()),
                    favorites = emptySet(),
                    onToggleFavorite = {},
                    onRefreshPosts = {},
                    onErrorDismiss = {},
                    navigateToArticle = {},
                    openDrawer = {},
                    scaffoldState = scaffoldState
                )
            }
        }

        // Then the first message received in the Snackbar is an error message
        runBlocking {
            // snapshotFlow converts a State to a Kotlin Flow so we can observe it
            // wait for the first a non-null `currentSnackbarData`
            val actualSnackbarText = snapshotFlow { snackbarHostState.currentSnackbarData }
                .filterNotNull().first().message
            val expectedSnackbarText = InstrumentationRegistry.getInstrumentation()
                .targetContext.resources.getString(R.string.load_error)
            assertEquals(expectedSnackbarText, actualSnackbarText)
        }
    }
}
Repository: android/compose-samples
File: Jetsnack/app/src/main/java/com/example/jetsnack/ui/home/FilterScreen.kt (Kotlin)

Code before:
        )
    }
}

@Composable
fun MaxCaloriesSlider() {
    var sliderPosition by remember { mutableStateOf(0f) }

    Slider(
        value = sliderPosition,
        onValueChange = {
            sliderPosition = it
        },
        valueRange = 0f..300f,
        steps = 5,
        modifier = Modifier
            .fillMaxWidth(),
        colors = SliderDefaults.colors(
            thumbColor = JetsnackTheme.colors.brand,
            activeTrackColor = JetsnackTheme.colors.brand
        )
    )
}

@Composable
fun FilterTitle(text: String,) {
    Text(
        text = text,
        style = MaterialTheme.typography.h6,
        color = JetsnackTheme.colors.brand,
        modifier = Modifier.padding(bottom = 8.dp)
    )
}
@Composable
fun SortOption(
    text: String,
    icon: ImageVector?,
    onClickOption: () -> Unit,
    selected: Boolean
) {
    Row(
        modifier = Modifier
            .padding(top = 14.dp)
            .clickable { onClickOption() }
    ) {
        if (icon != null) {
            Icon(imageVector = icon, contentDescription = null)
        }
        Text(
            text = text,
            style = MaterialTheme.typography.subtitle1,

Review comment (suggestion):
```suggestion
fun FilterTitle(text: String) {
```

Code after:
                onChanged(filter)
            }
        )
    }
}

@Composable
fun MaxCalories(sliderPosition: Float, onValueChanged: (Float) -> Unit) {
    FlowRow {
        FilterTitle(text = stringResource(id = R.string.max_calories))
        Text(
            text = stringResource(id = R.string.per_serving),
            style = MaterialTheme.typography.body2,
            color = JetsnackTheme.colors.brand,
            modifier = Modifier.padding(top = 5.dp, start = 10.dp)
        )
    }
    Slider(
        value = sliderPosition,
        onValueChange = { newValue ->
            onValueChanged(newValue)
        },
        valueRange = 0f..300f,
        steps = 5,
        modifier = Modifier
            .fillMaxWidth(),
        colors = SliderDefaults.colors(
            thumbColor = JetsnackTheme.colors.brand,
            activeTrackColor = JetsnackTheme.colors.brand
        )
    )
}

@Composable
fun FilterTitle(text: String) {
    Text(
        text = text,
        style = MaterialTheme.typography.h6,
        color = JetsnackTheme.colors.brand,
        modifier = Modifier.padding(bottom = 8.dp)
    )
}
@Composable
fun SortOption(
    text: String,
    icon: ImageVector?,
    onClickOption: () -> Unit,
    selected: Boolean
) {
    Row(
        modifier = Modifier
Repository: android/compose-samples
File: Jetsnack/app/src/main/java/com/example/jetsnack/ui/home/Feed.kt (Kotlin)

Code before:
import com.example.jetsnack.ui.components.JetsnackSurface
import com.example.jetsnack.ui.components.SnackCollection
import com.example.jetsnack.ui.theme.JetsnackTheme
import com.google.accompanist.insets.statusBarsHeight

@Composable
fun Feed(
    onSnackClick: (Long) -> Unit,
    modifier: Modifier = Modifier,
) {
    val snackCollections = remember { SnackRepo.getSnacks() }
    val filters = remember { SnackRepo.getFilters() }
    Feed(
        snackCollections,
        filters,
        onSnackClick,
        modifier
    )
}

@Composable
private fun Feed(
    snackCollections: List<SnackCollection>,
    filters: List<Filter>,
    onSnackClick: (Long) -> Unit,
    modifier: Modifier = Modifier,
) {

    JetsnackSurface(modifier = modifier.fillMaxSize()) {
        Box {
            SnackCollectionList(snackCollections, filters, onSnackClick)
            DestinationBar()
        }
    }
}

@OptIn(ExperimentalAnimationApi::class)
@Composable
private fun SnackCollectionList(
    snackCollections: List<SnackCollection>,
    filters: List<Filter>,
    onSnackClick: (Long) -> Unit,
    modifier: Modifier = Modifier
) {
    var filtersVisible by rememberSaveable { mutableStateOf(false) }
    Box(modifier) {
        LazyColumn {

            item {
                Spacer(Modifier.statusBarsHeight(additional = 56.dp))
                FilterBar(filters, onFilterClick = { filtersVisible = true })

Review comment (suggestion):
```suggestion
    modifier: Modifier = Modifier
```

Code after:
import com.example.jetsnack.ui.components.JetsnackSurface
import com.example.jetsnack.ui.components.SnackCollection
import com.example.jetsnack.ui.theme.JetsnackTheme
import com.google.accompanist.insets.statusBarsHeight

@Composable
fun Feed(
    onSnackClick: (Long) -> Unit,
    modifier: Modifier = Modifier
) {
    val snackCollections = remember { SnackRepo.getSnacks() }
    val filters = remember { SnackRepo.getFilters() }
    Feed(
        snackCollections,
        filters,
        onSnackClick,
        modifier
    )
}

@Composable
private fun Feed(
    snackCollections: List<SnackCollection>,
    filters: List<Filter>,
    onSnackClick: (Long) -> Unit,
    modifier: Modifier = Modifier
) {

    JetsnackSurface(modifier = modifier.fillMaxSize()) {
        Box {
            SnackCollectionList(snackCollections, filters, onSnackClick)
            DestinationBar()
        }
    }
}

@OptIn(ExperimentalAnimationApi::class)
@Composable
private fun SnackCollectionList(
    snackCollections: List<SnackCollection>,
    filters: List<Filter>,
    onSnackClick: (Long) -> Unit,
    modifier: Modifier = Modifier
) {
    var filtersVisible by rememberSaveable { mutableStateOf(false) }
    Box(modifier) {
        LazyColumn {

            item {
                Spacer(Modifier.statusBarsHeight(additional = 56.dp))
                FilterBar(filters, onShowFilters = { filtersVisible = true })
Repository: android/compose-samples
File: JetNews/app/src/main/java/com/example/jetnews/utils/WindowSize.kt (Kotlin)

Code before:
 *
 *     https://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package com.example.jetnews.utils

import androidx.compose.ui.unit.Dp

/**
 * Opinionated set of viewport breakpoints
 *     - Compact: Most phones in portrait mode
 *     - Medium: Most foldables and tablets in portrait mode
 *     - Expanded: Most tablets in landscape mode
 *
 * More info: https://material.io/archive/guidelines/layout/responsive-ui.html
 */
enum class WindowSize { Compact, Medium, Expanded }

fun getWindowSize(width: Dp): WindowSize = when (width.value) {
    in 0f..599f -> WindowSize.Compact
    in 600f..800f -> WindowSize.Medium
    else -> WindowSize.Expanded
}


Review comment (suggestion):
Since these are float ranges, anything greater than `599` but smaller than `600` (like `599.5`) will fall through to the `Expanded` case.

It might be clearer to just do a cascading when where the edges are clear:

```kotlin
fun getWindowSize(width: Dp): WindowSize = when {
    width < 600.dp -> WindowSize.Compact
    width <= 800.dp -> WindowSize.Medium
    else -> WindowSize.Expanded
}
```

Code after:
 *
 *     https://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package com.example.jetnews.utils

import androidx.compose.ui.unit.Dp

/**
 * Opinionated set of viewport breakpoints
 *     - Compact: Most phones in portrait mode
 *     - Medium: Most foldables and tablets in portrait mode
 *     - Expanded: Most tablets in landscape mode
 *
 * More info: https://material.io/archive/guidelines/layout/responsive-ui.html
 */
enum class WindowSize { Compact, Medium, Expanded }

fun getWindowSize(width: Dp): WindowSize = when {
    width.value < 0f -> throw IllegalArgumentException("Dp value cannot be negative")
    width.value < 600f -> WindowSize.Compact
    width.value <= 800f -> WindowSize.Medium
    else -> WindowSize.Expanded
}
Repository: android/compose-samples
File: JetNews/app/src/main/java/com/example/jetnews/utils/WindowSize.kt (Kotlin)

Code before:
 *
 *     https://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package com.example.jetnews.utils

import androidx.compose.ui.unit.Dp

/**
 * Opinionated set of viewport breakpoints
 *     - Compact: Most phones in portrait mode
 *     - Medium: Most foldables and tablets in portrait mode
 *     - Expanded: Most tablets in landscape mode
 *
 * More info: https://material.io/archive/guidelines/layout/responsive-ui.html
 */
enum class WindowSize { Compact, Medium, Expanded }

fun getWindowSize(width: Dp): WindowSize =
    if (width.value >= 0f) {
        when {
            width.value < 600f -> WindowSize.Compact
            width.value <= 800f -> WindowSize.Medium
            else -> WindowSize.Expanded
        }
    } else {
        throw IllegalArgumentException("Dp value cannot be negative")
    }


Review comment (suggestion):
Nit: you could put this into the `when` as well: `width.value < 0f -> throw IllegalArgumentException(...)`

Code after:
 *
 *     https://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package com.example.jetnews.utils

import androidx.compose.ui.unit.Dp

/**
 * Opinionated set of viewport breakpoints
 *     - Compact: Most phones in portrait mode
 *     - Medium: Most foldables and tablets in portrait mode
 *     - Expanded: Most tablets in landscape mode
 *
 * More info: https://material.io/archive/guidelines/layout/responsive-ui.html
 */
enum class WindowSize { Compact, Medium, Expanded }

fun getWindowSize(width: Dp): WindowSize = when {
    width.value < 0f -> throw IllegalArgumentException("Dp value cannot be negative")
    width.value < 600f -> WindowSize.Compact
    width.value <= 800f -> WindowSize.Medium
    else -> WindowSize.Expanded
}