This is not really a C# question. This is about the fundamentals of what a function is and how it should operate. Technically, a "function" should treat all input parameters as immutable, and return some value. A "method" may have side effects. Here you are using a C# "method", but you can treat it as a function, which tends to lead to cleaner code.
Things to keep in mind when writing functions:
- no side effects
- reduce it to the simplest set of instructions that make sense semantically
- reduce branching and nested loops as much as possible (these are usually indicators that further refactoring can be done to extract another function from the current one)
- swap out concrete classes of dependencies with interfaces (this goes way beyond the scope of what I'm posting today, but it helps with writing tests and can alleviate hassles during maintenance).
What is the method/function supposed to do? Try to reduce it to a few sentence in your native language, and then try to write those as a few lines of code. It looks like your method is doing the following... "if setting is not set to an integer value set it to nullValue, and return it's value. I would expect the final method to be no more than a few lines (and I do say method intentionally here, because there is a side effect - changing the value of setting if it "wasn't set").
You are probably going to end up with a method, but I'm going to harp on the importance of functions. By writing pure functions (no side effects), and reducing the complexity of individual functions, you can reduce required unit test case coverage dramatically. That being said, and this is at the core of your question, you still want to try to identify all edge cases for your method/function. For instance, say I have the following contrived function...
bool IsPositive(int val) { return val > 0; }
There are the obvious cases of > 0, and < 0, and the (albeit not so subtle) edge-case == 0. Currently, val = 0 would be treated as negative, which may or may not be desired behavior. There is a single expression being evaluated in this function, and there is no branching or looping, so the complexity is about as low as it gets, and we can easily reason about how this will behave.
In terms of C# specifically. You could wrap everything in a generic try/catch block, but that is... what's the opposite of "best practice"? Oh yeah, garbage. If you think you could encounter some sort of exception that is completely beyond your control (i.e. unmanaged resources - db connections, http client connections, etc.) then you can target that exception specifically. Then you would use a try/catch but only catching that specific exception. This of course implies that you have already ascertained the possible points of failure in your function.
The ability to determine points of failure can come from understanding the data-structures you are interacting with, or more commonly, just experience (trial and error, debugging). This ventures beyond C# again, and back to general programming skills. Like any skill, it takes time to develop, and some are naturally better at it than others.
Let's take a look at your code now...
public int GetIntSetting(SET setting, int nullState)
What is a SET? For now we'll ignore the fact that SET's behavior is likely encapsulated in some existing .NET class.
{
// Get the setting value or nullState if it was not set
var str = GetStringSetting(setting, nullState.ToString());
You have retrieved the setting, or nullState.ToString() at this point, and saved it in str. So in the next line, you can use that instead of evaluating it again. However, here you are only using the value once, so I would argue there is no point in allocating the memory for a variable, just to dereference it in the following line.
// Check if it's an integer
var isInt = int.TryParse(GetStringSetting(setting, nullState.ToString()), out int parsed);
Along those same lines, there is no reason to save the int.TryParse() result to a variable, it can be moved into the if expression. BUT, you don't need to TryParse, because if setting is not set it will be using nullState.ToString(), which we know must be an integer as a string, that will be successfully parsed. Then the trick is figuring out what GetStringSetting() actually did (code smell).
// If integer then return else update settings and return nullState
if (isInt )
{
return parsed;
}
else
{
UpdateIntSetting(setting, nullState);
return nullState;
}
Topically, you are returning in the if block, no need for else.
So if the setting was an integer value, return it, otherwise change it to nullState and return nullState. But I believe GetStringSetting()'s 2nd parameter is the value returned, if the first parameter was "not set", whatever that means (null? null or empty?). That means that you would have already been using nullState if the setting was not set (GetStringSetting(setting, nullState.ToString()))
...but maybe the setting could have been set to a non-numeric string (convoluted edge case). If that happened the setting would still be a non-numeric string, but the rest of the method would be operating using nullState.
Without knowing more about other code it's hard to say what your edge cases are, and that tends to be a code smell - requiring other code to understand how a method/function will behave (sometimes, in practice, it can't be avoided).
This method has potential side effects. If setting is not set, or not an integer value, its value would change to nullState. I think the best I can do with the information given, since I do not know anything about SET class, is to just assume that if parsed is equal to nullState, then setting needs to be updated. This could lead to subsequent updates that are not required (so this could be optimized with more info).
public int GetIntSetting(ISetting setting, int nullState)
{
int.Parse(GetStringSetting(setting, nullState.ToString()), out int parsed);
if(parsed == nullState)
{
UpdateIntSetting(setting, nullState);
}
return parsed;
}
You could invert the if statement's expression, and return immediately parsed != nullState (VS or a tool like resharper might make that suggestion), but the complexity wouldn't change in this case, and you would end up with two return statements instead of one.
Personally, I would probably make these (GetIntSetting()/GetStringSetting()) extension methods, or expose them on the ISetting interface I would create.
Now that we have refactored this, it is easy to see that our only possible points of failure are only...
GetStringSetting() because we are calling it on the first line but it is an unknown.UpdateIntSetting() again, an unknown
This is good. We would repeat the process for the two unknown methods, and we can begin passing the unit test cases that we would have written for this.
Finally, we are left with a method because, as mentioned above it has the potential side effect of changing setting's value. I would have to take a look at the rest of the code base, but it might make sense to leave it up to the caller what to do if the returned value is nullState. I know upfront it might make sense to hide the update inside this method, but it makes this individual piece more complex, and that complexity can accumulate quickly.
Update
I would guess GetStringSetting() does something similar, in that it updates the setting if it was not set. And since GetIntSetting() is dependent on it, and passes nullValue.ToString() as the default value, we know that after we call it setting would have been updated with nullValue if it was not set, then we can remove the update from GetIntSetting(). Then the code becomes...
public int GetIntSetting(ISetting setting, int nullState)
=> int.Parse(GetStringSetting(setting, nullState.ToString()), out int parsed);
It's still a method because it's dependent on a method, and there is still the potential side effect, but it reduces the complexity of GetIntSetting(). This is all based on the assumption that GetStringSetting() is performing an update.
We can write test cases for this, but this unit of work should not throw any exceptions (GetStringSetting(), or something it calls might). If these were extension methods or on the ISetting interface, then you could mock up GetStringSetting() to ensure GetIntSetting() does not fail from integration testing.
