3
\$\begingroup\$

CRITICAL = 0
MEDIUM = 1
LOW = 0


test = {
    "version": "0",
    "id": "85fc3613-e913-7fc4-a80c-a3753e4aa9ae",
    "detail-type": "ECR Image Scan",
    "source": "aws.ecr",
    "account":
        "123456789012",
    "time": "2019-10-29T02:36:48Z",
    "region": "us-east-1",
    "resources": [
        "arn:aws:ecr:us-east-1:123456789012:repository/my-repo"
    ],
    "detail": {
        "scan-status": "COMPLETE",
        "repository-name": "my-repo",
        "finding-severity-counts": {
            "CRITICAL": 10,
            "MEDIUM": 9
        }
    }
}
resource = test["resources"][0]

finding_severity_counts = test["detail"]["finding-severity-counts"]

if "CRITICAL" in finding_severity_counts:
    critical_count = finding_severity_counts["CRITICAL"]
    if critical_count > CRITICAL:
        print("Resource {} has {} critical findings".format(resource, critical_count))
        
if "MEDIUM" in finding_severity_counts:
    medium_count = finding_severity_counts["MEDIUM"]
    if medium_count > MEDIUM:
        print("Resource {} has {} medium findings".format(resource, medium_count))
        
if "LOW" in finding_severity_counts:
    low_count = finding_severity_counts["LOW"]
    if low_count > LOW:
        print("Resource {} has {} low findings".format(resource, low_count))

I have got this code and am trying to optimize it so it is smaller and takes up less lines of code. Right now the code is kind of ugly and not at all efficient. Is there any way to make this more efficient?

\$\endgroup\$
  • 2
    \$\begingroup\$ Are you focussing on reducing the amount of lines or is simply making this more readable and less repetitive the focus? One doesn't exclude the other, but it's good to know what your intentions are with this. I can think of extremes in both directions you may not like. \$\endgroup\$ – Mast Jul 13 at 14:10
  • 1
    \$\begingroup\$ I'm mostly looking to reduce the repetitive nature of the code which im hoping will have the side benifit of reducing the number of lines \$\endgroup\$ – John Smith Jul 13 at 14:13
  • 2
    \$\begingroup\$ Great, we can work with that. There's the minor problem of the missing definition of test though. I can't test your code and my suggestions without it. \$\endgroup\$ – Mast Jul 13 at 14:18
  • 1
    \$\begingroup\$ Sorry about that, just edited the coded to add it \$\endgroup\$ – John Smith Jul 13 at 14:21
  • \$\begingroup\$ You have four lines repeated three times, the only difference being the severity - did you try refactoring to use e.g. for severity in ("CRITICAL", "MEDIUM", "LOW"):? \$\endgroup\$ – jonrsharpe Jul 13 at 15:11
1
\$\begingroup\$

You're going to have issues when you'll have multiple resources. Because you'll have multiple resources, but you will only have a single detail dictionary. Which means you won't know what resource generate which count. So you should probably review that structure - I'm guessing what you want is that resource should be a dictionary instead of a list. The keys for that dictionnary would be each resource ID, and within each resources you'll have further key-values pairs (such as your details).

I haven't touched that as I may be reading your use case wrong.

For the rest:

CRITICAL = 0
MEDIUM = 1
LOW = 0

test = {
    "version": "0",
    "id": "85fc3613-e913-7fc4-a80c-a3753e4aa9ae",
    "detail-type": "ECR Image Scan",
    "source": "aws.ecr",
    "account":
        "123456789012",
    "time": "2019-10-29T02:36:48Z",
    "region": "us-east-1",
    "resources": [
        "arn:aws:ecr:us-east-1:123456789012:repository/my-repo"
    ],
    "detail": {
        "scan-status": "COMPLETE",
        "repository-name": "my-repo",
        "finding-severity-counts": {
            "CRITICAL": 10,
            "MEDIUM": 9
        }
    }
}

def get_summaries(test):
    summaries       = []
    severity_counts = test["detail"]["finding-severity-counts"]
    resource        = test["resources"][0]
    for severity, counts in severity_counts.items():
        summaries.append(f"Resource {resource} has {counts} {severity} findings")
    return summaries

summaries = get_summaries(test)
for s in summaries:
    print(s)
    
  • mydictionnary.items() allows you to iterate on all the keys, values of that dictionnary. That removes the need for the if-else
  • f"my strings in which I want a {my_variable} value" is a newer and IMO clearer, more concise syntax than format. It should do the job most of the time. The severity (LOW, MED, CRITICAL) is already present in "finding-severity-counts" variable, so why not re-use it. This allows using a single line to format your strings, as opposed to printing 3 different ones.
  • It's better to encapsulate things in method, rather than just dump everything directly in the main file. This is why you have a get_summaries() methods now. Allows re-use, etc.
  • Generally in python, convention for variable is using_underscores_to_split_words. I haven't change your keys in the test dictionary is other parts of your application may depend on the hyphen... but should be considered changing if you're going to use python. Just a convenient convention, and if you're just starting out anyways, might as well get these things right.

Note that I may easily have expressed that same code in fewer lines. I wouldn't really need to do severity_counts = test["detail"]["finding-severity-counts"], and then iterate on severity_counts - I could just do for k,v in test["detail"]["finding-severity-counts"].items(). However this is less readable imo.

=================

Other than that, as others have touched in the comment... the real goal of refactoring your code (e.g. revisiting existing, working code to improve it) should not really be to make it "shorter". It should be to make it better. How do we define better will vary with different applications, but things to consider:

  • a more concise notation. Not because it is shorter, but because using fewer lines to the the same things may mean you're using a loop (instead of long if-elif), or just having a better code structure.
  • more readable code. ensuring your choice of variable and method names clearly indicates what happens
  • more re-usable code. Your code generally should consist of methods. Each method should have one clear purpose and focus on doing that one thing really well.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks so much! You are a proper legend! \$\endgroup\$ – John Smith Jul 13 at 15:27
2
\$\begingroup\$

There is indeed a great deal of repetition in your code. A straight-forward solution to this would be putting your severity levels into a dictionary and iterate over it instead:

test = {
    "version": "0",
    "id": "85fc3613-e913-7fc4-a80c-a3753e4aa9ae",
    "detail-type": "ECR Image Scan",
    "source": "aws.ecr",
    "account":
        "123456789012",
    "time": "2019-10-29T02:36:48Z",
    "region": "us-east-1",
    "resources": [
        "arn:aws:ecr:us-east-1:123456789012:repository/my-repo"
    ],
    "detail": {
        "scan-status": "COMPLETE",
        "repository-name": "my-repo",
        "finding-severity-counts": {
            "CRITICAL": 10,
            "MEDIUM": 9
        }
    }
}

resource = test["resources"][0]
finding_severity_counts = test["detail"]["finding-severity-counts"]

THRESHOLDS = {
    'CRITICAL' : 0,
    'MEDIUM'   : 1,
    'LOW'      : 0
}

for severity in THRESHOLDS:
    if severity in finding_severity_counts:
        count = finding_severity_counts[severity]
        if count > THRESHOLDS[severity]:
            print("Resource {} has {} {} findings".format(resource, count, severity.lower()))

This should produce the exact same result as your code does, by reusing the logic (that's the same for every severity. Note that we can reuse the count, since we're only interested in one value at a time.

That is, assuming you're only interested in the amount of severities if they reach a certain threshold. Hence the name.

It would probably be prettier to put the for-loop into a function though. An even better solution would probably involve one of Python's 'collections' functions, like Counter.

| 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.