Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Done with all the task #3

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

buddiex
Copy link

@buddiex buddiex commented Feb 27, 2017

I am done with all the task.

Though i know i can still do some refactoring ( task 3) ... will try to improve on it soon.

Your comments are welcome

Copy link
Contributor

@ikenna ikenna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good effort 👍


object DataMunging {

def main(args: Array[String]) ={
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove def main(args: Array[String]) by just extending App, that is, object DataMunging extends App

source.close()
}
//
// def getSourceData(filename: String): BufferedSource = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can delete commented out code to keep your code clean

// return Source.fromFile(filename)
// }

def find_min(data: List[(String, Double)]): (String, Double)= {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job factoring out this common code 👍

getMinGoalDiff()
}
def getMinSpread() = {
val source = Source.fromFile("weather.dat")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also factor out

  1. the code to fetch all the lines in the function, because that is common to both getMinSpread() and getMinGoalDiff().
  2. filter out the header and the lines with "-----" and the blank line in this function.
  3. and you tokenise (split) by space.
    That is:
def getLines(val fileName:String):List[Array[String]] = {
               val source = Source.fromFile(fileName)
               val lines = source.getLines.toList
                source.close
               lines.drop(0)
                       .filter(line=> !line.contains("-----"))
                       .filter(line => !line.trim.isEmpty)
                       .map(_.split("\\s+"))
   }

.map(_.split("\\s+")) // split each line on white space
.map(x =>(x(2), x(7).toDouble - x(9).toDouble)) // select team and different between f and a
val team = find_min(teams)
println(s"2: the team with the smallest difference for and against goal is ${team._1} with a difference of ${team._2} degrees")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice touch with the string interpolation 👍

.map(_.split("\\s+")) // split on regex white space
.map(x => (x(1), x(2).stripSuffix("*").toDouble-x(3).stripSuffix("*").toDouble)) //clean the recourds as somelines have * and aslo conver to double
val min_spread = find_min(spread) // get minimum spread
println(s"1: the day with the smallest temparature spread is day ${min_spread._1} with a spread of ${min_spread._2} degrees")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

println at this point is okay. However your getMinSpread() function can be more testable with a unit test if you make it return the actual spread. In other words, making it 'a pure function without side-effects' .
The println can be cone in the main method. So your main method would look like:

def main(args: Array[String]) ={
    val minSpread = getLines("weather.dat"). getMinSpread
    println(s"1: the day with the smallest temparature spread is day ${minSpread._1} with a spread of ${minSpread._2} degrees")
    val team = getLines("football.dat").getMinGoalDiff()
    println(s"2: the team with the smallest difference for and against goal is ${team._1} with a difference of ${team._2} degrees")
 }

.drop(2) //the first line is a header and second line is a blank space so skip those lines here.
.map(_.split("\\s+")) // split on regex white space
.map(x => (x(1), x(2).stripSuffix("*").toDouble-x(3).stripSuffix("*").toDouble)) //clean the recourds as somelines have * and aslo conver to double
val min_spread = find_min(spread) // get minimum spread
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scala style guide usually goes with camelCase variable names like minSpread, rather than having underscores like min_spread

http://docs.scala-lang.org/style/

def getMinGoalDiff() = {
val source = Source.fromFile("football.dat") //get file access object
val teams = source.getLines.toList
.drop(1) //drop the header
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you implement getLines the way I suggest, getMinGoalDiff() becomes
def getMinGoalDiff(line: Array[String):(String, Double).
Also lines 22 to 26 will no longer be needed.

def find_min(data: List[(String, Double)]): (String, Double)= {
return data.reduceLeft((x,y) => if (x._2 < y._2) x else y)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, very very good effort 👍 👍 . For someone new to Scala, you have done a great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants