1
\$\begingroup\$

I am new to Scala.

I have been trying to complete an exercise where the ask was as follows:

// Write a function isPerfectNumber which takes integer input and returns String output.

// It finds if a number is perfect, and returns true if perfect, else returns false

// Write a higher order function myHigherOrderFunction which takes isPerfectNumber and intList as input, and returns a List of Strings which contain the output if the number is perfect or not using map.

Perfect Number :

https://rosettacode.org/wiki/Perfect_numbers#Scala

My Code :

object ListMapHigherOrder{
def main(args:Array[String])
{

val intRes = args.toList
val intList: List[Int] = intRes.map(_.toInt).toList

 def isPerfectNumber(input: Int) :String  = 
 {
 var check_sum = ( (2 to math.sqrt(input).toInt).collect  { case x if input % x == 0 => x + input / x}  ).sum 
 if ( check_sum == input - 1 )
      return "true"
      else 
      return "false"
      }

def myHigherOrderFunction(argFn: Int => String, argVal:List[Int]): List[String] = { argVal.map(argFn) }

println(myHigherOrderFunction(isPerfectNumber, intList))

}
}

Code execution : scala ScalaExcercise12.scala 1 6 13

Expected Output : List(false , true , false)

the code gives expected output, am not sure how the backend testing is being done.... it just dosent pass the test.

Is there any issue with the code? - i did like to fix it , but cant i see anything wrong/missing

any help or direction is greatly appreciated.

\$\endgroup\$
1
  • \$\begingroup\$ Why on Earth does the exercise require the return type to be String instead of the obvious Boolean? \$\endgroup\$ Dec 14, 2019 at 13:15

1 Answer 1

1
\$\begingroup\$

Here is my corrected solution.

object ListMapHigherOrder{
  def main(args:Array[String]) = {

    val intList: List[Int] = args.map(_.toInt).toList

    def isPerfectNumber(input: Int) :String  = {
      val check_sum = ( (2 until input).collect  { case x if input % x == 0 => x }  ).sum
      if (input <= 1) "false"
      else if ( check_sum == input - 1 ) "true"
      else "false"
    }

    def myHigherOrderFunction(argFn: Int => String, argVal:List[Int]): List[String] = { argVal.map(argFn) }

    println(myHigherOrderFunction(isPerfectNumber, intList))
  }
}

Since this is supposed to be code review, I will not mince words. Some problems I noticed with your code:

1) Your formatting is irregular. For anyone trying to understand your code, this is a needless obstruction.

2) You do not check if the number is negative or equal to 1. In all such cases, the number is not perfect. You need to check for these cases.

3) You are using a var for check_sum. This is an eyesore for anyone who knows the language. Prefer immutable values.

4) You copied and pasted your code for isPerfectNumber from someone else without understanding what it does. In this case, the code you copied and pasted is actually wrong, because it gives the wrong answer for 1. It is probably also a good idea to explicitly check if the number is negative. The original code was also relatively inefficient because of the call to square root. The change to remove square root is only a micro-optimization, but it should be faster.

5) You are trusting that calling toInt on a String won't throw an exception. This depends on the assignment specification. If the exercise assumes you will always get well-formed input, then I suppose this is fine. But if not, you need to use a try-catch block or a Try monad. Since I am not sure if you need this stuff, I won't add it to your solution.

6) The idiom for myHigherOrderFunction is strange. There is almost no point to passing a list and a lambda function to a function just so you can call map on it. If you need to call map, just call map. Creating a wrapper function is unnecessary and an eyesore. I understand that this is what the assignment asks you to do, probably to test your understanding of functions as values. However, it is ugly code, I cannot help but mention that this should be avoided normally.

I think this question is rather manageable, and that if you had spent more time on it, you would have been able to get it yourself. However, I will not lecture you. The biggest mistake you made was trusting someone else's code without checking it yourself. So you should not trust me either.

\$\endgroup\$
5
  • \$\begingroup\$ intRes serves no purpose. args.map(_.toInt).toList also eliminates the redundant .toList invocation. \$\endgroup\$
    – jwvh
    Nov 14, 2019 at 4:48
  • \$\begingroup\$ @jwvh Thank you for the correction. I have edited the answer to include your correction. \$\endgroup\$
    – Allen Han
    Nov 14, 2019 at 4:52
  • \$\begingroup\$ Regarding item 6: in this very early phase of learning programming, it is a good exercise to reimplement the basic and proven primitives, just to get an understanding of how they work. Of course, after this exercise you should be told by your teacher that someone else already did that work and tested it comprehensively, and that you therefore should use their implementation instead of your own. \$\endgroup\$ Dec 14, 2019 at 13:22
  • \$\begingroup\$ Regarding item 4: How is calling sqrt a single time worse than needlessly iterating over the other \$n-\sqrt n\$ numbers, just to sum up a few of them? \$\endgroup\$ Dec 14, 2019 at 13:26
  • \$\begingroup\$ @RolandIllig About item 4, point taken. As n gets bigger, it is more likely that calculating the square root is beneficial. I suppose the only way to know for sure is with microbenchmarking, or instruction counting for the two versions of the program. Without such analysis, it is difficult to be certain. \$\endgroup\$
    – Allen Han
    Dec 14, 2019 at 19:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.