Return to Answer

Yes, this is likely a code smell, which could lead to code that is difficult to understand and that has a lower chance of being easily re-used. As a result the maintainability and consistency drops.

As other posters have noted context is everything (don't go in heavy-handed if it's a one off or if the practice has been acknowledged as deliberately incurred technical debt to be re-factored later). Broadly speaking if there is a parameter (any type, this sometimes happens with integers as well) passed into a function that selects specific behaviour to be executed, then the design is 'inverted' and has a tendency to the less maintainable spectrum of code. This kind of design implies that the function in question knows of all the cases AND has knowledge to calculate different inputs. The function is doing more than two things and this makes it harder to test and change.

Making a function of every second parameter 'state' and in the case of a boolean, that means two functions, results in cohesive functions.

So what is a highly cohesive function?

It's a function that does one thing and one thing only. It may rely on other cohesive functions, but when it does, delegating is the 'only thing it does'. Compare with work in a real life factory. Every single part of the production process is handled by a different entity. Apply this idea of inseparable 'actions' or 'behaviours' to a function in order to get a cohesive function.

As you have already noted, the intertwining of these concerns seems off.

So the notion of Cohesiveness helps us identify that the function in question is not highly cohesive and that we could refactor the code to produce a set of more cohesive functions.

So the question could be restated; Given that we all agree passing behavioural selection parameters is best avoided how do we improve matters?

I would get rid of the parameter completely. Having the ability to turn off access control even for testing is a potential security risk. For testing purposes either stub or mock the access check to test both the access allowed and access denied scenarios.

As for future functions that seem to use 'behaviour parameters', simply resort to the Strategy pattern or something similar. Analyze the required functions from top to bottom.

Ref: Cohesion (computer science)

Yes, this is likely a code smell, which would lead to unmaintainable code that is difficult to understand and that has a lower chance of being easily re-used.

As other posters have noted context is everything (don't go in heavy-handed if it's a one off or if the practice has been acknowledged as deliberately incurred technical debt to be re-factored later) but broadly speaking if there is a parameter passed into a function that selects specific behaviour to be executed then further step-wise refinement is required; Breaking up this function in to smaller functions will produce more highly cohesive ones.

So what is a highly cohesive function?

It's a function that does one thing and one thing only.

The problem with a parameter passed in as you describe, is that the function is doing more than two things; it may or may not check the users access rights depending on the state of the Boolean parameter, then depending on that decision tree it will carry out a piece of functionality.

It would be better to separate the concerns of Access Control from the concerns of Task, Action or Command.

As you have already noted, the intertwining of these concerns seems off.

So the notion of Cohesiveness helps us identify that the function in question is not highly cohesive and that we could refactor the code to produce a set of more cohesive functions.

So the question could be restated; Given that we all agree passing behavioural selection parameters is best avoided how do we improve matters?

I would get rid of the parameter completely. Having the ability to turn off access control even for testing is a potential security risk. For testing purposes either stub or mock the access check to test both the access allowed and access denied scenarios.

Ref: Cohesion (computer science)

Yes, this is likely a code smell, which would lead to unmaintainable code that is difficult to understand and that has a lower chance of being easily re-used.

As other posters have noted context is everything (don't go in heavy-handed if it's a one off or if the practice has been acknowledged as deliberately incurred technical debt to be re-factored later) but broadly speaking if there is a parameter passed into a function that selects specific behaviour to be executed then further step-wise refinement is required; Breaking up this function in to smaller functions will produce more highly cohesive ones.

So what is a highly cohesive function?

It's a function that does one thing and one thing only.

The problem with a parameter passed in as you describe, is that the function is doing more than two things; it may or may not check the users access rights depending on the state of the Boolean parameter, then depending on that decision tree it will carry out a piece of functionality.

It would be better to separate the concerns of Access Control from the concerns of Task, Action or Command.

As you have already noted, the intertwining of these concerns seems off.

So the notion of Cohesiveness helps us identify that the function in question is not highly cohesive and that we could refactor the code to produce a set of more cohesive functions.

So the question could be restated; Given that we all agree passing behavioural selection parameters is best avoided how do we improve matters?

I would get rid of the parameter completely. Having the ability to turn off access control even for testing is a potential security risk. For testing purposes either stub or mock the access check to test both the access allowed and access denied scenarios.

Ref: Cohesion (computer science)

Yes, this is likely a code smell, which could lead to code that is difficult to understand and that has a lower chance of being easily re-used. As a result the maintainability and consistency drops.

As other posters have noted context is everything (don't go in heavy-handed if it's a one off or if the practice has been acknowledged as deliberately incurred technical debt to be re-factored later). Broadly speaking if there is a parameter (any type, this sometimes happens with integers as well) passed into a function that selects specific behaviour to be executed, then the design is 'inverted' and has a tendency to the less maintainable spectrum of code. This kind of design implies that the function in question knows of all the cases AND has knowledge to calculate different inputs. The function is doing more than two things and this makes it harder to test and change.

Making a function of every second parameter 'state' and in the case of a boolean, that means two functions, results in cohesive functions.

So what is a highly cohesive function?

It's a function that does one thing and one thing only. It may rely on other cohesive functions, but when it does, delegating is the 'only thing it does'. Compare with work in a real life factory. Every single part of the production process is handled by a different entity. Apply this idea of inseparable 'actions' or 'behaviours' to a function in order to get a cohesive function.

As you have already noted, the intertwining of these concerns seems off.

So the notion of Cohesiveness helps us identify that the function in question is not highly cohesive and that we could refactor the code to produce a set of more cohesive functions.

So the question could be restated; Given that we all agree passing behavioural selection parameters is best avoided how do we improve matters?

I would get rid of the parameter completely. Having the ability to turn off access control even for testing is a potential security risk. For testing purposes either stub or mock the access check to test both the access allowed and access denied scenarios.

As for future functions that seem to use 'behaviour parameters', simply resort to the Strategy pattern or something similar. Analyze the required functions from top to bottom.

Ref: Cohesion (computer science)

Yes, this is likely a code smell, which would lead to unmaintainable code that is difficult to understand and that has a lower risk of being easily re-used.

As other posters have noted context is everything (don't go in heavy-handed if it's a one off or if the practice has been acknowledged as deliberately incurred technical debt to be re-factored later) but broadly speaking if there is a parameter passed into a function that selects specific behaviour to be executed then further step-wise refinement is required; Breaking up this function in to smaller functions will produce more highly cohesive ones.

So what is a highly cohesive function?

It's a function that does one thing and one thing only.

The problem with a parameter passed in as you describe, is that the function is doing more than two things; it may or may not check the users access rights depending on the state of the Boolean parameter, then depending on that decision tree it will carry out a piece of functionality.

It would be better to separate the concerns of Access Control from the concerns of Task, Action or Command.

As you have already noted, the intertwining of these concerns seems off.

So the notion of Cohesiveness helps us identify that the function in question is not highly cohesive and that we could refactor the code to produce a set of more cohesive functions.

So the question could be restated; Given that we all agree passing behavioural selection parameters is best avoided how do we improve matters?

I would get rid of the parameter completely. Having the ability to turn off access control even for testing is a potential security risk. For testing purposes either stub or mock the access check to test both the access allowed and access denied scenarios.

Ref: Cohesion (computer science)

Yes, this is likely a code smell, which would lead to unmaintainable code that is difficult to understand and that has a lower chance of being easily re-used.

As other posters have noted context is everything (don't go in heavy-handed if it's a one off or if the practice has been acknowledged as deliberately incurred technical debt to be re-factored later) but broadly speaking if there is a parameter passed into a function that selects specific behaviour to be executed then further step-wise refinement is required; Breaking up this function in to smaller functions will produce more highly cohesive ones.

So what is a highly cohesive function?

It's a function that does one thing and one thing only.

The problem with a parameter passed in as you describe, is that the function is doing more than two things; it may or may not check the users access rights depending on the state of the Boolean parameter, then depending on that decision tree it will carry out a piece of functionality.

It would be better to separate the concerns of Access Control from the concerns of Task, Action or Command.

As you have already noted, the intertwining of these concerns seems off.

So the notion of Cohesiveness helps us identify that the function in question is not highly cohesive and that we could refactor the code to produce a set of more cohesive functions.

So the question could be restated; Given that we all agree passing behavioural selection parameters is best avoided how do we improve matters?

I would get rid of the parameter completely. Having the ability to turn off access control even for testing is a potential security risk. For testing purposes either stub or mock the access check to test both the access allowed and access denied scenarios.

Ref: Cohesion (computer science)