The test: https://rosalind.info/problems/ini2/


Given: Two positive integers a and b, each less than 1000.
Return: The integer corresponding to the square of the hypotenuse of the right triangle whose legs have lengths a and b.

Sample Dataset

3 5

Sample Output


My script:

#!/bin/env python3
import argparse
import os.path
import sys

def int_sq(n):
    return int(n)**2

def square_hyp_triangle(a,b):
    #check int
    sq_sum=sum(int_sq(i) for i in [a,b]) 
    return sq_sum

def main(argv=None):
    parser = argparse.ArgumentParser(
            description='Import a file with integers')
                        help='an existing filename')

    args = parser.parse_args(argv)
    if not args.filename:
        sys.exit("Please provide an input file, or pipe it via stdin")

    for line in args.filename:
        line = line.rstrip().split()
        res = square_hyp_triangle(line[0],line[1])
        #print(line[0], line[1])

if __name__ == '__main__':

If you saved the above script named intro.py You can run it :

echo "3 5" | ./intro.py -

Which parts can be improved?

  • 1
    \$\begingroup\$ Please do not edit the question, especially the code, after an answer has been posted. Changing the question may cause answer invalidation. Everyone needs to be able to see what the reviewer was referring to. What to do after the question has been answered. You can ask a follow up question with a link back to this question. \$\endgroup\$
    – pacmaninbw
    Commented Oct 2, 2023 at 13:37

3 Answers 3


Imports are sorted -- nice!

optional type hinting

def int_sq(n):

Consider using this signature:

def int_sq(n: float) -> int:

(Recall that float includes int for hinting purposes.)

sum, map

    sq_sum = sum(int_sq(i) for i in [a, b]) 

Consider phrasing this as

    sq_sum = sum(map(int_sq, [a, b]))

since the temp var i is uninteresting and could be anonymous.

Also, delete the meaningless "#check int" comment. And maybe just return the expression without giving it a name.


The identifier square_hyp_triangle is fine. But do write an English sentence for it:

def square_hyp_triangle(a: float, b: float) -> float:
    """Finds the squared length of a triangle's hypotenuse.

Consider writing another few lines so that $ python -m doctest *.py does an automated test to verify things work as intended.

    """Finds the squared length of a triangle's hypotenuse.

    >>> square_hyp_triangle(3, 5)


In main the parser is nice enough, though "import" perhaps is a slightly odd verb. I'm glad you're using that library; it reduces the documentation burden for end users. Consider switching to typer, which gives you similar functionality "for free" (if you're using type hints).

I'm slightly surprised by the absence of ... , default=sys.stdin), but I guess your defaulting must work fine. The filename identifier seems to have the wrong name, given that you want it be an open file descriptor rather than a name.

In line.rstrip().split() maybe you had some CR/LF trouble? I don't see why you need to strip, given that split will discard whitespace for you.

tuple unpack

Avoid cryptic [0] / [1] subscripts, preferring a, b = line.split(). Or better, convert from str right here using:

        a, b = map(float, line.split())

Aha, now I understand that return int(n)**2 expression. That doesn't seem a very sensible place to do the conversion. I wouldn't really expect that a function could square "3". And even if it did, should I get back "9" instead of 9? Best to not even explore such territory, and stick to numeric values, so the squaring function has a single responsibility and is not also responsible for type conversions. Better to allocate that responsibility to the CLI interface called main.

Rejecting fractional inputs like 3.1 is maybe an unusual design choice. Guess it stems from the "Given:" and "Return:" parts of the spec, OK. Rosalind's "download a one-line text file dataset" requirement pushes this code in a slightly odd direction.

old debugs

        #print(line[0], line[1])

It served a purpose at one time. But now that your code is "done" and you're requesting review, it is the right time to tidy up and delete debug statements.

Which parts can be improved?

Adhere to Single Responsibility.

Recommend you adopt black auto-formatting, so expressions have a uniform appearance without even having to think about it.

  • \$\begingroup\$ Hello @J_H, would you be kind to elaborate on the ... , default=sys.stdin) I have mainly used stackoverflow to find a way to read from a file with argparse but it didn't have this default argument. Maybe point me to another post? \$\endgroup\$
    – K Y
    Commented Oct 2, 2023 at 9:54
  • 1
    \$\begingroup\$ I just took it literally from the docs. (I used to use argparse all the time, but nowadays I can usually go the lazy route of relying on import typer which does most of the heavy lifting for me.) The documentation example includes a call to parser.add_argument(... , default=sys.stdin) \$\endgroup\$
    – J_H
    Commented Oct 2, 2023 at 14:14

The command-line usage is awkward. Your program needs two arguments: a and b. In a case like that, the easiest thing to do is configure argparse – no surprises here – to take two arguments. Instead, you have set things up so that the user must either put the two integers in a file and pass the file path as an argument or to use the piped-echo approach illustrated in your demo usage.

The awkward command-line usage also forces you to parse text. But that's all unnecessary if you just do the natural thing: take two arguments.

Whenever feasible, let argparse enforce the usage requirements. Your program requires a and b. There's no escaping that. Make the arguments required.

Whenever feasible, let argparse enforce data types. Same idea. Your program wants integers. Configure argparse with that detail and let the library do the work for you.

Move command line argument parsing out of main(). This is just a good habit to get into. It allows you to test/debug the command-line interface separately, if needed, and it keeps main() focused narrowly on its primary job: top-level program orchestration.

from argparse import ArgumentParser
import sys

def main(args):
    opts = parse_args(args)
    ss = sum_of_squares(opts.a, opts.b)

def parse_args(args):
    ap = ArgumentParser()
    ap.add_argument('a', type = int)
    ap.add_argument('b', type = int)
    return ap.parse_args(args)

def sum_of_squares(*xs):
    return sum(x ** 2 for x in xs)

if __name__ == '__main__':
  • \$\begingroup\$ Unless CPython has changed in recent versions, sum([x ** 2 for x in xs]) should be faster than sum(x ** 2 for x in xs). \$\endgroup\$
    – ggorlen
    Commented Sep 30, 2023 at 3:53
  • 1
    \$\begingroup\$ As a note to this review, if you have your arguments in a file, then we can convert externally to command-line arguments - e.g. xargs <file intro.py. In my opinion, that's better than reimplementing this functionality in your program(s). \$\endgroup\$ Commented Sep 30, 2023 at 6:33
  • Don't parse command line arguments manually. Consider docopt instead.

  • Usually I advocate a more functions, please approach. In this case it seems you went a bit too far. I see no benefits of wrapping operator ** into a function.

  • int() may throw ValueError, which will terminate the script prematurely. Be prepared, and catch it.

  • \$\begingroup\$ Hello vnp, I am a bit confused by the "Don't parse command line arguments manually." Isn't the argparse the method that generally is used to parse command line arguments in python scripts? Isn't it manually either using docopt or argparse? \$\endgroup\$
    – K Y
    Commented Oct 2, 2023 at 7:56
  • 1
    \$\begingroup\$ Sorry for a late reply. Perhaps manual was bad wording. argparse requires much more code. In my opinion docopt is just cleaner. \$\endgroup\$
    – vnp
    Commented Oct 3, 2023 at 3:08

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.