1
\$\begingroup\$

I have been programming in various languages for a very long time, and this was one of my first attempts to build an Android app. Apparently this was not good enough for the challengee, could somebody please point out the most major flows and oversights in my design?

The app display a list of pets in a recyclerView and opens a webview to wikipedia when clicked. The Call and Chat button will display an alert telling if shop is open or not - placeholder for something else...

OpenHours are parsed from a json.

Full project: https://github.com/simonsso/Pet-Shop-Boy

MainActivity.kt

package net.thesimson.petshopboy

import android.app.AlertDialog
import android.content.DialogInterface
import androidx.appcompat.app.AppCompatActivity
import android.os.Bundle
import android.view.View
import androidx.annotation.UiThread
import kotlinx.android.synthetic.main.activity_main.*
import org.json.JSONArray
import org.json.JSONException
import org.json.JSONObject
import java.util.*

class MainActivity : AppCompatActivity() {
    @UiThread
    fun displayOpenHours() {
        // Create an alertbox dialog
        val dialogBuilder = AlertDialog.Builder(this)

        val storeOpenGreetingMessage:String = if( ShopHours.isShopOpen(Calendar.getInstance()) ){
                resources.getString(R.string.thanksshopopen)
            }else {
                resources.getString(R.string.thanksshopclosed)
        }
        dialogBuilder.setMessage(storeOpenGreetingMessage)
            .setCancelable(false)
            .setPositiveButton(resources.getString(R.string.ok), DialogInterface.OnClickListener { dialog, _id ->
                dialog.cancel()
            })
        val alert = dialogBuilder.create()
        alert.setTitle(resources.getString(R.string.alerttitle))
        alert.show()
    }

    @UiThread
    fun updateCallButtonsVisability(){
        chat_button.visibility = if (ShopHours.chatEnabled) {
            View.VISIBLE
        } else {
            View.GONE
        }
        call_button.visibility = if (ShopHours.callEnabled) {
            View.VISIBLE
        } else {
            View.GONE
        }
        workHours.text = if (ShopHours.humanReadableSign != "") {
             resources.getString(R.string.officehours) + "  " + ShopHours.humanReadableSign
        } else {
            resources.getString(R.string.hours_not_loaded_from_config)
        }
    }
    @UiThread
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(R.layout.activity_main)

        call_button.setOnClickListener{displayOpenHours()}
        chat_button.setOnClickListener{displayOpenHours()}
        updateCallButtonsVisability()

        PawCache.requestContent( "https://simonsso.github.io/Pet-Shop-Boy/config.json") { config_json_string ->
            // NB file is known to be malformated json finding the outermost JSONObject
            val config = try {
                 JSONObject(
                    config_json_string.substring(
                        config_json_string.indexOf("{"),
                        config_json_string.lastIndexOf("}") + 1
                    )
                )
            }catch (e: JSONException) {
                // Parsing failed return an empty object
                JSONObject()
            }
            // Config loaded OK, update UI!
            this@MainActivity.runOnUiThread {
                ShopHours.parseBuisinessHours(config.optString("workHours"))
                ShopHours.chatEnabled = config.optBoolean("isChatEnabled")
                ShopHours.callEnabled = config.optBoolean("isCallEnabled")

                updateCallButtonsVisability()
            }
        }

        val adapter = RecomendedPetsRecyclerViewAdapter(this, PetZoo.pets)

        PawCache.requestContent("https://simonsso.github.io/Pet-Shop-Boy/pets.json") {pets_json_string->

                // NB file is known to be malformated json finding the outermost JSONObject
            val temp_net_pets = try {
                JSONArray(
                    pets_json_string.substring(
                        pets_json_string.indexOf("["),
                        pets_json_string.lastIndexOf("]") + 1
                    )
                )
            }catch (e:JSONException){
                JSONArray()
            }
            this@MainActivity.runOnUiThread {
                PetZoo.pets.clear()
                for (i in 0 until temp_net_pets.length()) {
                    // JSONArray is broken and will return len 11 but last item does not exist
                    // This try will cactch such an exception
                    try {
                        if (temp_net_pets.getJSONObject(i) is JSONObject) {
                            PetZoo.pets.add(temp_net_pets[i] as JSONObject)
                        }
                    }catch (e:JSONException){

                    }
                }
                adapter.notifyDataSetChanged()
            }
        }
        recycler_view.adapter = adapter
        adapter.notifyDataSetChanged()
    }
}

PawCache.kt

package net.thesimson.petshopboy

import android.graphics.Bitmap
import android.graphics.BitmapFactory
import android.os.Handler
import android.widget.ImageView
import okhttp3.*
import java.io.IOException
import java.util.concurrent.ConcurrentHashMap

import kotlin.concurrent.thread


object PawCache {
    lateinit var client: OkHttpClient

    val cache = ConcurrentHashMap<String, Bitmap>()

    fun requestContent(url: String, onDataFetched:(String)->Unit ){
        thread {
            try {
                val request = Request.Builder().url(url).build()
                client.newCall(request)
                    .enqueue (object :Callback  {
                        override fun onResponse(call: Call, response: Response) {
                            if (!response.isSuccessful) return
                            onDataFetched(response.body!!.string())
                        }
                        override fun onFailure(call: Call, e: IOException) {
                            // Do nothing on failure
                        }
                    })
            }catch (e:Exception){
                println(e.message)
            }
        }
    }

    // Request image, and load it into ImageView
    // Errors are ignored here- a new attempt to fetch an uncached image
    // will be made next time UI needs it...

    fun requestImage(imageUrl: String, dest: ImageView) {
        val uiThreadHandler=Handler()
        if( cache.containsKey(imageUrl)){
            dest.setImageBitmap(cache[imageUrl])
            return
        }
        thread {
            try {
                val request = Request.Builder().url(imageUrl).build()
                client.newCall(request)
                    .enqueue (object :Callback  {
                        override fun onResponse(call: Call, response: Response) {
                            if (!response.isSuccessful) return
                            val bm = BitmapFactory.decodeStream(response.body!!.byteStream())
                            uiThreadHandler.post(Runnable {
                                cache[imageUrl]=bm
                                dest.setImageBitmap(bm)
                            })
                        }
                        override fun onFailure(call: Call, e: IOException) {
                            // Do nothing on failure
                        }
                    })
            }catch (e:Exception){
                println(e.message)
            }
        }
    }
}

PetRecyclerViewAdapter.kt

package net.thesimson.petshopboy

import android.content.Context
import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import android.widget.TextView
import android.widget.ImageView
import android.widget.LinearLayout
import androidx.recyclerview.widget.RecyclerView
import org.json.JSONException
import org.json.JSONObject
import android.content.Intent


object PetZoo{
    var pets = ArrayList<JSONObject>()
}

class RecomendedPetsRecyclerViewAdapter(private var context: Context, private var dataList:ArrayList<JSONObject>):
    RecyclerView.Adapter<RecomendedPetsRecyclerViewAdapter.ViewHolder>() {
    override fun getItemCount(): Int {
        return dataList.size
    }
    override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): ViewHolder {
        return ViewHolder(LayoutInflater.from(context).inflate(R.layout.pet_row_item, parent, false))
    }

    override fun onBindViewHolder(holder: ViewHolder, position: Int) {
        try {
            val curItem = dataList[position]

            if (curItem is JSONObject) {
                val petname = curItem.optString("title")
                holder.textView.text = petname

                val image_url = curItem.optString("image_url")
                val content_url = curItem.optString("content_url")

                if (image_url!=""){
                    PawCache.requestImage(image_url,holder.icon)
                }

                holder.top.setOnClickListener {
                    val intent : Intent = Intent( context, PetBrowserActivity::class.java)
                    intent.putExtra("loadurl",content_url)
                    intent.putExtra("petname",petname)
                    context.startActivity( intent )
                }
            }
        }catch (e:JSONException){
            println(e.message)
        }
    }

    class ViewHolder(itemView: View) : RecyclerView.ViewHolder(itemView) {
        val textView:TextView =itemView.findViewById(R.id.petRVTextView)
        val icon:ImageView =itemView.findViewById(R.id.petRVImageView)
        val top:LinearLayout =itemView.findViewById(R.id.petRVLinearLayout)
    }
}

ShopHours.kt

package net.thesimson.petshopboy

import androidx.annotation.UiThread
import java.util.*

object ShopHours {
    // TODO support for weekends and work days
    var openAt:Int = 0
    var closeAt:Int = 0
    var humanReadableSign:String = ""
    var chatEnabled:Boolean = false
    var callEnabled:Boolean = false

    @UiThread
    fun parseBuisinessHours(s:String):Boolean{
        // Whops, Retrieving groups by name is not supported on this platform.
        // Use group numbers
        // Days: 1
        // OpenHour: 2
        // OpenMinute: 4
        // CloseHour: 5
        // CloseMinute:7
        // Live example:  https://regex101.com/r/tBRHnN/1
        val pattern = Regex("""([\w-]+)\s+(\d+)(:(\d+))?\s*-\s*(\d+)(:(\d+))?""")

        val matchResult=pattern.matchEntire(s)
        if (matchResult != null ){
            val groups = matchResult.groups
            // TODO Week day/Weekend
            val openHour:Int = groups[2]?.value?.toIntOrNull()?:0
            val openMin:Int = groups[4]?.value?.toIntOrNull()?:0
            val closeHour:Int = groups[5]?.value?.toIntOrNull()?:0
            val closeMin:Int = groups[7]?.value?.toIntOrNull()?:0

            openAt = 60 * openHour + openMin
            closeAt = 60 * closeHour + closeMin
            humanReadableSign = s
            return true
        }else{
            return false
        }
    }
    @UiThread
    fun isShopOpen(time:Calendar):Boolean{
        if(time.get(Calendar.DAY_OF_WEEK) == Calendar.SATURDAY ||
           time.get(Calendar.DAY_OF_WEEK) == Calendar.SUNDAY ) return false
        return  time.get(Calendar.HOUR_OF_DAY) * 60 + time.get(Calendar.MINUTE) in openAt..closeAt
    }
}

Please let me know in the comments if I should post more of the files here or I should trim the code above to some problematic area.

\$\endgroup\$
  • \$\begingroup\$ Please start by letting me know how to improve this question to meet the site praxis ;-) \$\endgroup\$ – Simson Feb 13 at 8:25
  • 2
    \$\begingroup\$ Have you already read How do I ask a good question? \$\endgroup\$ – AlexV Feb 13 at 8:55
  • \$\begingroup\$ Had read it now I have read it! Slightly better already I think. \$\endgroup\$ – Simson Feb 13 at 9:00
  • \$\begingroup\$ Should I dump all code in here or will that just mess up? \$\endgroup\$ – Simson Feb 13 at 9:01
  • 2
    \$\begingroup\$ I think the title could still be improved since you present a concrete application example. Regarding the amount of code: please paste all the code that provides relevant context for what you want to have reviewed. Having additional code in a GitHub repo is generally accepted here. I'm don't know kotlin well enough to be sure, but from what I can see it looks like the code has no external dependencies other than 3rd party libraries, which is good for review. \$\endgroup\$ – AlexV Feb 13 at 9:11
2
\$\begingroup\$

Use resource value itself

  • dialogBuilder.setMessage accepts a messageId.
  • SetPositiveButton accepts txtId
  • setTitle accepts titleId
  • -

Use lambda's

setPositiveButton accepts a functional interface. due to sam-conversions, you can pass in a lambda.

setPositiveButton(
    resources.getString(R.string.ok), 
     DialogInterface.OnClickListener { dialog, _id ->
         dialog.cancel()
     }
)

therefor becomes:

setPositiveButton(
    resources.getString(R.string.ok), 
     { dialog, _id -> dialog.cancel() }
)

In kotlin, you can write a lmabda outside the parenthesis if they are the last argument.
The code therefor becomes:

setPositiveButton(resources.getString(R.string.ok)){ dialog, _id -> 
    dialog.cancel()
)

And due to the former chapter, it can be rewritten to: setPositiveButton(R.string.ok){ dialog, _id -> dialog.cancel() )

. Therefor it's not needed to get the string yourself. I personally love to us when to make the incrementation way smaller, even if you use it for it then.

extension-functions

In Kotlin you can define functions in a way that it looks like they're part of the class. We can use this to simplify the following code:

chat_button.visibility = if (ShopHours.chatEnabled) {
    View.VISIBLE
} else {
    View.GONE
}

by adding:

fun View.setVisible(visible: Boolean){
    this.visibility = if(visible) View.Visible else View.Gone
}

You can write chat_button.setVisible(ShopHours.chatEnabled)

You can do this with properties to:

var View.isVisible : Boolean
    get() = this.visibility == View.Visible
    set(value) {
        this.visibility = if(value) View.Visible else View.Gone
    }

Which allows us to write the former code as: chat_button.isVisible = ShopHours.chatEnabled

This property is also provided in kotlin-ktx, Androidx.core.view to be precise.

strings with params

When you are doing it correct by using string-resources, why not go all the way?
You can get a string with parameters in Android, see formatting.
When you want to add a string as parameter use % + paramNumber + $S and pass the params through getString.

example:

<string name="sayHi">I say %1$S %2$S. </string>
getString(R.strings.sayHi, "hello", "you")

coroutines

Kotlin has a concept called coroutines.
Google has a tutorial here.
This are lightweight threads and compiler magic.

In pawCache you can write:

suspend fun Request.getResponse(url : String) = suspendCoroutine<Response> {
    client.newCall(request)
        .enqueue(object : Callback {
            override fun onResponse(call: Call, response: Response) {
                if (!response.isSuccessful) it.resume(response)
            }

            override fun onFailure(call: Call, e: IOException) {
                it.resumeWithException(e)
            }
        })
}

And then you can rewrite your other functions:

suspend fun requestContent(url: String) = 
    Request.Builder().url(url).build().getResponse()

This will return null for the case where you said doNothing and you have to catch the exception of OnFailure.

suspend fun requestImage(imageUrl: String, dest: ImageView) {
    val uiThreadHandler=Handler()
    if( cache.containsKey(imageUrl)){
        dest.setImageBitmap(cache[imageUrl])
        return
    }
    try {
        val response = Request.Builder().url(imageUrl).build().getResponse() ?: return
        val bm = BitmapFactory.decodeStream(response.body!!.byteStream())
        withContext(Dispatchers.Main){
            cache[imageUrl]=bm
            dest.setImageBitmap(bm)
        }
    } catch(e : Exception){
        println(e.message)
    }
}

println

Assuming you don't have pure-java modules, you use the android logger.
There is a possibility to extend this by using libraries.
The only one I really heard of isis Timber( medium article ), but most of the time I'm just using the Android logger.

Anko

Anko is old, not maintained well, but still useful. For example, You wrote:

val intent : Intent = Intent( context, PetBrowserActivity::class.java)
intent.putExtra("loadurl",content_url)
intent.putExtra("petname",petname)
context.startActivity( intent )

using Anko/commons you can rewrite this to:

startActivity<PerBrowserActivity>(
    "loadurl" to content_url,
    "petName" to petname
)

architecture

You shoulld seperate view, logic and data code should be seperated. Therefor take a look at Android architecture components, mvp / mvvm or the mviCore library

Hope this helps.

| improve this answer | |
\$\endgroup\$

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.