2

I have a function that was written because several methods used the same sequence of code so an abstraction was useful. The function takes a string representing the path to a registry key and then a list of 3-tuple containing attributes to update the key.

This function is subsequently called updateKeyAttributes:

_ROOT_MASK = {HKEY_CLASSES_ROOT: 'HKEY_CLASSES_ROOT',
              HKEY_CURRENT_USER: 'HKEY_CURRENT_USER'}

def updateKeyAttributes(hiveroot: HKType, key: str, attrs: Sequence[Tuple[Optional[str], int, Optional[Any]]) -> None:

    try:
        with CreateKeyEx(hiveroot, key, 0, KEY_WRITE) as open_key:
            for name, typ, value in attrs:
                SetValueEx(open_key, name, 0, typ, val)
    except OSError as err:
        path = f'{_ROOT_MASK[hiveroot]\\{key}'
        raise RegistryError(f'Failed to update key attributes: {path}') from err

As I was looking at this function, I started to wonder if it is violating the SRP?

To me, its responsibility is to "update the attributes of a particular key", and that it's only job. However, how granular does that "single responsibility" go?

The body of the function does 3 things:

  1. Creates (subsequently Opens) a handle to an HKey
  2. Sets the values of the attributes for the HKey
  3. Raises a wrapped exception if #1 or #2 fail

Is the SRP that granular or is my understanding that the function is only responsible for updating, and conversely not deleting, valid within the SRP? I've read that SRP only applies to how classes behave together in a program, but not about whether it relates to how standalone methods. Surely at some point a method does something more than just a single thing:

def main():

    program_init()
    program_run()
    program_cleanup()

Unless your granularity is "it managed the programs lifespan"...

Edit:

In the event it is a violation, I can pull some out:

def updateKeyAttributes(key: HKType, attrs: Sequence[Tuple[Optional[str], int, Optional[Any]]) -> None:
    
    for name, typ, val in attrs:
        SetValueEx(key, name, 0, typ, val)

try:
    with CreateKeyEx(hiveroot, key, 0, KEY_WRITE) as open_key:
        updateKeyAttributes(open_key, attrs)
except OSError as err:
    path = f'{_ROOT_MASK[hiveroot]\\{key}'
    raise RegistryError(f'Failed to update key attributes: {path}') from err

But now the module will have, two try/except blocks rather than sharing a common function.

4
  • Never. SRP applies to modules (or classes), not functions. – user949300 Mar 16 at 3:48
  • 2
    Does this answer your question? What does it mean for a method or a function to do one thing? – gnat Mar 16 at 4:50
  • 1
    @user949300: your statement is a misleading - just because Bob Martin originally used that term in the context of classes does not mean one can ignore responsibilities in the context of functions. – Doc Brown Mar 16 at 7:12
  • Not just 3, it's much much worse! This function does thousands of things. (1) moves a value into a register, (2) reads a value from the stack, (3) decrements the stack pointer, (4) reads the constant KEY_WRITE, ...... – user253751 Mar 16 at 13:03
4

SRP is a Rule of Thumb. Its amazing how many thumbs it does not fit.

Take it as a guide, not as a law of nature.


I'd argue that the most important principal is Readability. SRP being one of its horsemen, but not the main show.

I argue that Readability is more important because the code is read by many people and many machines, many times over.

SRP can help get us to more readable code, but like you've highlighted it can go circular and lead us back out to worse code.

Think of normal forms in a database. Each normal form from 1st through to 3rd slowly improves the quality of the data structure and generally makes it more legible. But as you keep normalising you will arrive at the Triple store. A Triple can store any data, it is arguably the most normalised, and universal schema you can have. When you don't know what you are storing, its the best option.

It is however also the most terrible data-structure to have. Any question you have about the organisation or structure of the data has to be discovered by trawling through all the data in the store, or at least the relevant parts. You have no upfront certainty and must double check every assumption.

Similarly in code you want to "normalise"/apply SRP while it is improving the code quality judged by how easy it is to read and understand. You know its easier to understand because the structure tells you the answer without having to execute the program.

The moment SRP starts reducing that code quality, ignore it. You'll know this because you will have to run the program to discover how it hangs together. Not that you can remove this need altogether, it is a program after all, not a lookup table. But given the choice between having to try to understand by looking at the result, and understanding by reading the code itself, prefer reading the code.

2

SRP granularity should match the granularity of your logic. You deal with registry items, then that is the abstraction level for your program. Anything lower level is covered for you by OS/library functions so it is pointless to replicate that in your program. At the other side you do not want to mix dealing with the registry with your own methods so you isolate the registry item handling in a way that allows you to store and address the things that matter to you.

If your logic only stores and retrieves strings you may want to eliminate the item type from the argument list because it is meaningless to the logic that calls the method. You do want to see a name and a value in an argument list, tuples are not the best fit here, you don't want to create puzzles.

4
  • Perhaps you can expand on your last paragraph? Why are tuples not a good fit here? The attrs parameter expects a list of 3-tuple representing the (name, type, value) of each attribute to be applied to a particular key (key: HKEYType is an open handle). – madeslurpy Mar 16 at 11:57
  • @madeslurpy Perhaps it is just me not being familiar with Python but the way I see it (name, type, value) does not appear in the function's signature. So from a user/client perspective it is unclear what is to be passed in, one has to dig deeper into the function to find out what to pass and in what order. If one types "updateKeyAttributes(" in the IDE, all names and descriptions of arguments should pop up. If that were the case it would not be so bad. Still it seems like an arbitrary split. Why pass part of the arguments that all address the same thing explicitly and another part as tuples? – Martin Maat Mar 16 at 12:39
  • @madeslurpy The use case for tuples is where you have multiple values but you can only pass one thing and a dedicated class would be overkill. Like with a return value. I do not see that here, you can pass as many named arguments as you like. – Martin Maat Mar 16 at 12:39
  • The Sequence[Tuple[Optional[str], int, Optional[Any]] annotation denotes how the container of size n containing tuples of size 3 should be constructed. The docstring of the function, omitted in the OP, explains what each parameter is. The reason for the split is that one key (of type HKEYType) may have n number of attributes to update. By passing a container of these 3-tuples, the function opens just a single handle and iterates the container; the alternative would be to open the key and loop over the container, calling updateKeyAttributes on each element (a single tuple). – madeslurpy Mar 16 at 14:05
0

Look through recent posts and you will find one about a team lead with fanatical dedication to SRP who destroyed a company’s code base.

Never apply SRP because you are told to apply SRP as the only reason. It’s the most destructive rule ever invented. It has caused untold damages. You are looking for artificial reasons to apply SRP. In reality, but the truth is, your function has one responsibility only.

Think about it: You wrote this code. Are you going to ask someone else to press the “save” button because that is a separate responsibility? And a third person to try it out? No, it’s one responsibility to add a tested function to your source code control,organising all the needed reviews.

1
  • can you show this recent post? – user253751 Mar 16 at 13:11

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.