2
\$\begingroup\$

The file football.dat contains the results from the English Premier League for 2001/2. The columns labeled ‘F’ and ‘A’ contain the total number of goals scored for and against each team in that season (so Arsenal scored 79 goals against opponents, and had 36 goals scored against them). Write a program to print the name of the team with the smallest difference in ‘for’ and ‘against’ goals.

Please give me advice regarding best practices.

import java.io.File
import java.io.Reader

fun main(arg: Array<String>) {
    val inputStream = File("football.dat").inputStream()
    val reader = inputStream.bufferedReader().use { it.readLines() }
    //prints first Name in array.
    println(reader.first().team)
}

fun Reader.readLines(): List<teamAndScoreDifference> {
    val arrayOfTeamsAndGoals = arrayListOf<teamAndScoreDifference>()
    forEachLine {
        //if makePair(it) is not null,
        makePair(it)?.let {
            //add makePair(it)
            arrayOfTeamsAndGoals.add(it)
        }
    }
    val result = arrayOfTeamsAndGoals
            //by lowest (GoalsScored - GoalsTaken)
            .sortedBy { it.scoreDiffere }
    return result
}

data class teamAndScoreDifference(val team: String, val scoreDiffere: Int)

fun makePair(line: String): teamAndScoreDifference? {
    val teamName = line
            .take(23)
            .substringAfter(". ")
            .substringBefore(" ")
    val goalsScoredAndTaken: String = line
            .take(52)
            .substringAfterLast("   ")
    val goalsScored =
            goalsScoredAndTaken
            .substringBefore(" ")
            .toIntOrNull()
    val goalsTaken =
            goalsScoredAndTaken
            .substringAfterLast("  ")
            .toIntOrNull()
    if (goalsScored != null && goalsTaken != null) {
        return teamAndScoreDifference(teamName, Math.abs(goalsScored - goalsTaken))
    }
    //else return null
    return null
}

football.dat:

       Team            P     W    L   D    F      A     Pts
    1. Arsenal         38    26   9   3    79  -  36    87
    2. Liverpool       38    24   8   6    67  -  30    80
    3. Manchester_U    38    24   5   9    87  -  45    77
    4. Newcastle       38    21   8   9    74  -  52    71
    5. Leeds           38    18  12   8    53  -  37    66
    6. Chelsea         38    17  13   8    66  -  38    64
    7. West_Ham        38    15   8  15    48  -  57    53
    8. Aston_Villa     38    12  14  12    46  -  47    50
    9. Tottenham       38    14   8  16    49  -  53    50
   10. Blackburn       38    12  10  16    55  -  51    46
   11. Southampton     38    12   9  17    46  -  54    45
   12. Middlesbrough   38    12   9  17    35  -  47    45
   13. Fulham          38    10  14  14    36  -  44    44
   14. Charlton        38    10  14  14    38  -  49    44
   15. Everton         38    11  10  17    45  -  57    43
   16. Bolton          38     9  13  16    44  -  62    40
   17. Sunderland      38    10  10  18    29  -  51    40
   -------------------------------------------------------
   18. Ipswich         38     9   9  20    41  -  64    36
   19. Derby           38     8   6  24    33  -  63    30
   20. Leicester       38     5  13  20    30  -  64    28
\$\endgroup\$
2
\$\begingroup\$

Class names in Java or Kotlin always start with a capital, so TeamAndScoreDifference. It makes it difficult to read the code otherwise because I assume it's a variable.

The name of readLines() is not quite right since it does many other things. It does 3 things: read the lines, transform them to pairs and sorts them. It's awkward that the sorting is in this method, but picking the first element was left in the main method.

I don't like the pattern that you use in the body of readLines() where you initialize an empty list and then fill it with a for-loop (a forEachLine actually). This is what is usually done in C and Java, but modern functional languages proceed differently. The pattern that is being executed here is just a mapping: you take a list of objects and return a list of modified objects. That is done better with the map function; see my code below.

Also it's not clear why your readLines() should be an extension method and not just a plain function.

You don't need to define this function anyway since File.readLines() already exists.

Other programmers might have used a plain Pair instead of a data class TeamAndScoreDifference. But I'm not recommending one way or another.

makePair could probably have used a better name , but I can't come up with one right now. You could have used pattern matching to extract the values from the text. If you don't know how to use regex you will have to learn it at some point as a programmer, either in Java, Kotlin, or any other language. But it seems that your file has columns of predetermined width, so your solution does work as it is.

The if-else clause in Kotlin is an expression which returns a value, so you can make the final return less awkward at the end of makePair.

You forgot to consider that the answer might not be unique.

You write too many comments. They just describe what the code is doing. You can put official documentation for a public API, otherwise you only put comments to explain something quite complicated that another programmer might have a hard time comprehending. So there are very rarely comments within the code. Another problem with comments is that they can sometimes be contradictory with the code. You have such an instance with //by lowest (GoalsScored - GoalsTaken) where the code instead uses the absolute value.

import java.io.File

fun main(arg: Array<String>) {
    val lines = File("football.dat").readLines()
    val teamsWithScoreDiff = lines.map(::makePair).filterNotNull()
    val minDiff = teamsWithScoreDiff.map(TeamAndScoreDifference::scoreDiffere).min()
    val minTeams = teamsWithScoreDiff.filter { it.scoreDiffere == minDiff }
    println("Team(s) with minimal score difference of %minDiff: %{minTeams}")
}

data class TeamAndScoreDifference(val team: String, val scoreDiffere: Int)

fun makePair(line: String): TeamAndScoreDifference? {
    val teamName = line
            .take(23)
            .substringAfter(". ")
            .substringBefore(" ")
    val goalsScoredAndTaken: String = line
            .take(52)
            .substringAfterLast("   ")
    val goalsScored =
            goalsScoredAndTaken
                    .substringBefore(" ")
                    .toIntOrNull()
    val goalsTaken =
            goalsScoredAndTaken
                    .substringAfterLast("  ")
                    .toIntOrNull()
    return if (goalsScored != null && goalsTaken != null)
        TeamAndScoreDifference(teamName, Math.abs(goalsScored - goalsTaken))
    else
        null
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Do you have any comments about the makePair function? \$\endgroup\$ – Carl Fujinami Aug 14 '17 at 3:24
  • \$\begingroup\$ There is a paragraph about it in the middle. Basically: the name could be improved, it could have used pattern matching instead and the final return statement could be improved too. \$\endgroup\$ – toto2 Aug 14 '17 at 11:09
0
\$\begingroup\$

I ended up changing the makePair function to return null if it's created nulls for either goalsScored or goalsTaken. This avoids needless executing of code and removes the ugly if and else statements.

fun makePair(line: String): TeamAndScoreDifference? {
val teamName = line
        .take(23)
        .substringAfter(". ")
        .substringBefore(" ")

val goalsScoredAndTaken: String = line
        .take(52)
        .substringAfterLast("   ")
val goalsScored =
        goalsScoredAndTaken
                .substringBefore(" ")
                .toIntOrNull() ?: return null
val goalsTaken =
        goalsScoredAndTaken
                .substringAfterLast("  ")
                .toIntOrNull() ?: return null

return TeamAndScoreDifference(teamName, Math.abs(goalsScored - goalsTaken))
}
| 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.