6
\$\begingroup\$

I'm trying to come up with a design for a chess engine in Kotlin that hides implementation details, and that ensures that such an implementation cannot accidentally become spaghetti.

Client side example code

I envision that a client would access the chess engine with something like:

package chessprototype

import chessprototype.chess.Chess

class ChessClient {
    fun hello() {
        val searcher = Chess.mateSearcher()
        searcher.board = "k1K/2Q w"
        val move = searcher.findBestMove()
        println("Best move is " + move.toString())
    }
}

To achieve that I created the following.

Main interface code

package chessprototype.chess

// Undesired cyclic dependency needed to expose the MateSearcher
import chessprototype.chess.searchers.MateSearcher

/** Understandable interface for the client */
class Chess {
    interface Move {
        override fun toString(): String
    }
    data class TextMove(val text: String): Move { override fun toString() = text }
    interface Searcher {
        var board: String
        fun findBestMove(): Move
    }
    companion object {
        fun mateSearcher() = MateSearcher()
    }
}

I've put all interfaces that a client would use in a facade-like Chess object. And I've put basic access types in there as well, such as TextMove

I've put a factory method in the Chess object as well to expose the complicated MateSearcher while hiding its implementation. I've put it in a companion object so that I can create it without instantiating a Chess class.

Implementation code

package chessprototype.chess.searchers

// Undesired cyclic dependency needed for
// - basic types (Move, TextMove)
// - the interface to implement (Searcher)
import chessprototype.chess.Chess

class MateSearcher: Chess.Searcher {
    override var board: String = ""
    override fun findBestMove(): Chess.Move {
        return doComplicatedSearch()
    }
    // Complicated code that should not be exposed to the client
    private fun doComplicatedSearch(): Chess.Move { return Chess.TextMove("Qb7#") }
}

I've put the implementation in a separate package so that:

  • it cannot accidentally use code that was not explicitly imported first.
  • it cannot accidentally allow other code to use implementation details.

As you can see there is a mutual import that seems undesired to me.

My challenges

  1. Ensure that the Searcher cannot 'accidentally' start using low level implementations from other parts of the code.
  2. Ensure that the main interface cannot 'accidentally' leak implementation details.
  3. Ensure that the main interface for the client is as clean and simple as possible.

My immediate problem: how can I eliminate the cyclic dependency of packages?

And more generally, what would you do differently and why?

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Welcome to Code Review. First of all, I am planning on writing a review for you later today. Secondly, the custom on this site is for titles to include something related to what the code does, otherwise we will end up with too similar titles that aren't very helpful. Therefore I have added the Chess-part of your question again. (Although it's possible that the title can be improved further, I think this is good enough) \$\endgroup\$ – Simon Forsberg Mar 30 '20 at 10:22
  • \$\begingroup\$ For a guide on how to handle follow-ups, please see what you may and may not do after receiving answers. \$\endgroup\$ – Simon Forsberg Mar 30 '20 at 22:19
  • \$\begingroup\$ Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 Mar 30 '20 at 22:30
3
\$\begingroup\$

The cyclic import dependencies can only be removed in one way: By removing one of them. :)

As I see it now, you have:

  • Client, which needs to create the MateSearcher object, and also know about the API.
  • An API
  • An implementation class

Naturally, the implementation class - MateSearcher - needs to be aware of the API, so that's not an import that you want to remove.

I'd recommend putting a ChessFactory class (or rather, object) in the same package as the MateSearcher. It can look something like this:

object ChessFactory {
    fun mateSearcher(): Searcher = MateSearcher()
}

Some things to note here:

  • It's an object, not a class, meaning the method can be called with ChessFactory.mateSearcher() (no need for a companion object)
  • The method declares to return a Searcher, it doesn't need to declare that it is a MateSearcher since that will make the client aware about the implementation details.

Now, removing the construction of the MateSearcher leaves your Chess class with... two interfaces and a data class. The Chess class is basically wrapping them, which I personally don't find very useful, but I'll leave that to you to decide whether to keep it or not.


Looking a bit closer at what the Chess class contains,

class Chess {
    interface Move {
        override fun toString(): String
    }
    data class TextMove(val text: String): Move { override fun toString() = text }
    interface Searcher {
        var board: String
        fun findBestMove(): Move
    }
}

The Move interfaces specifies that a toString method should exist. Such an interface always exists. It's also not recommended to make your code dependent on the return values of toString. The toString method should be used for only one thing: Printing information to you, the programmer. Its results should not be shown to users of the program and its implementation details should not be important for the application logic. It's currently hard to tell what your Move interface should contain as the only implementation is your TextMove class, which makes me doubt that you need the Move interface at all. What other implementations will you have?

Your generic Move class should possibly have methods like Move.validate(ChessBoard) and Move.perform(ChessBoard).


You have code in your MateSearcher that is commented with Complicated code that should not be exposed to the client. I'd like to make you aware of the fact that that function is marked private, and no function or field marked private will ever be exposed to anything else other than the class itself, no matter if we're implementing an interface or not.


The Searcher interface stings a bit to me, especially when looking at your usage of it. Let me ask you this: What if I have a Searcher and call findBestMove() without setting the board? That shouldn't be allowed, right? Why is the board not a parameter to findBestMove()?

Making this change would leave us with:

interface Searcher {
    fun findBestMove(board: String): Move
}

Which could be changed to:

typealias Searcher = (board: String) -> Move

Which, in my opinion, is more Kotlinic (like Pythonic but for Kotlin), and it would make it easier to use lambdas or method references for the searcher.

\$\endgroup\$
0

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.