Project review, part 2

@can-gurkan, @JNK234 ,

Hi. I know this is very delayed, but I finally got around to reviewing the code for this project.

Here are some things that stood out to me:

• If this project was funded by the CCL (which I think it was), then I think ownership should be transferred to @NetLogo
• Boilerplate in *Provider classes
• Awkward re-throw pattern
• Unnecessary locking
• Unnecessary try pattern
• Verbose parsing

Boilerplate in *Provider classes

There's tons of boilerplate/repetition between the different *Provider classes. They should just inherit from a common trait that gives defaults for all of the common behavior. Aside from the create*Request and parse*Response methods, these classes mostly only differ on things that could be (maybe ideally) declared in a .json file somewhere. It's just several hundred lines of code, largely saying the same things over and over.

Awkward re-throw pattern

Several times in LLMExtension.scala, there's some unusual code where ExtensionException is caught and immediately rethrown.

You can work around this by replacing:

      } catch {
        case e: ExtensionException => throw e
        case e: Exception =>
          throw new ExtensionException(myFavoriteErrorMessage)
      }

with

      } catch {
        case e: Exception if !e.isInstanceOf[ExtensionException] =>
          throw new ExtensionException(myFavoriteErrorMessage)
      }

Unnecessary locking

ConfigStore.scala has a bunch of locking code, like:

class ConfigStore {
  private val config = mutable.Map[String, String]()
  private val lock = new Object

  def set(key: String, value: String): Unit = {
    lock.synchronized {
      config(key) = value
    }
  }

  // ...
}

A very minor point: Object isn't used in Scala. AnyRef is the Scala equivalent.

Next, I don't think lock needs to exist at all. Instead, just delete private val lock = ... and replace all mentions of lock with config.

But, even so, I think the synchronized machinery is a bit nasty. I think you can get rid of all of the synchronized usages and the lock variable, if you just do this instead:

import java.util.concurrent.ConcurrentHashMap
import scala.jdk.CollectionConverters._

class ConfigStore {
  private val config = new ConcurrentHashMap[String, String]().asScala
  // ...
}

This will use a collection that manages all of the necessary synchronization for you.

Unnecessary try pattern

LLMExtension.scala contains several instances of code like this:

        val responseMessage = Await.result(responseFuture, {
          val timeoutSeconds = try {
            configStore.getOrElse(ConfigStore.TIMEOUT_SECONDS, ConfigStore.DEFAULT_TIMEOUT_SECONDS).toInt
          } catch {
            case _: NumberFormatException => 30
          }
          timeoutSeconds.seconds
        })

But I think the try is unnecessary. I'm struggling to see what realistic circumstances could cause that code to throw an exception.

Verbose parsing

ConfigLoader.scala contains this method definition:

private def parseLine(line: String, lineNumber: Int): Try[(String, String)] = {
  Try {
    val equalIndex = line.indexOf('=')

    if (equalIndex == -1) {
      throw new IllegalArgumentException(s"Missing '=' separator in line: $line")
    }

    if (equalIndex == 0) {
      throw new IllegalArgumentException(s"Missing key in line: $line")
    }

    val key = line.substring(0, equalIndex).trim
    val value = line.substring(equalIndex + 1).trim

    if (key.isEmpty) {
      throw new IllegalArgumentException(s"Empty key in line: $line")
    }

    // Allow empty values
    (key, value)
  }
}

That is... a lot of text, for something I'm finding conceptually simple. If you want, here's some regular expression-based code for parsing that information (with much less detailed error-reporting):

private def parseLine(line: String, lineNumber: Int): Try[(String, String)] = {
  val KVRegex = """^\s*(\w+)\s*=\s*(\w+)\s*$""".r
  line match {
    case KVRegex(key, value) => Success((key, value))
    case                   _ => Failure(new IllegalArgumentException(s"Invalid key-value pair on line $lineNumber: $line"))
  }
}

Regardless: Good work, on a cool project. 😎 You guys made something ambitious and useful. Once you address some (or most) of this stuff, I think that you'll have something of very high quality.

Review the files named in the checklist: LLMExtension.scala, ConfigStore.scala, ConfigLoader.scala, and the *Provider classes. First inspect how these areas are used and how their current behavior is tested. Done means implementing a clearly scoped subset of the proposed cleanup without changing required parsing, error handling, or concurrency behavior.