Problem
I read about, that I only should have up to two parameters and a function should have about 10 lines. Now I am in trouble to achieve this. It is a Javascript/JSON thing in C#.
I have too many parameters and too many lines in a function and I would like to reduce it. I reduced it already, but what can I do better. I need all the parameters, because the DB needs them.
public JsonResult AddFolder(int id, string structureId, string newFolderName, string parent)
{
int folderNumber = 0;
//parent folder id
int parentFolder = Convert.ToInt32(GetFolderId(id).Split('_')[0]);
//get current period
int period = GetLastPeriod();
//actual ISO date
string folderDate = Helper.ConvertToIsoDate(DateTime.Now.ToString().Split(' ')[0]);
if (period == -1)
{
throw new Exception(ErrorMessages.PeriodNotFound);
}
bool publish;
//implement publish true when it is parent - false when it is child
try
{
if (parent == "#")
{
publish = true;
structureId = AddNewParentFolderId(folderNumber);
}
else
{
publish = false;
structureId = AddNewChildFolderId(structureId, parentFolder, parent);
}
}
catch (Exception ex)
{
Logging.LogToFile(ex, 3);
return Json(new
{
success = false,
message = ErrorMessages.CountFolderNotPossible
}, JsonRequestBehavior.AllowGet);
}
if (folderNumber < 0)
{
Logging.LogToFile("MrNumber has to be > 0", 3);
base.Response.StatusCode = 500;
return Json(new
{
errorAddFolderMsg = ErrorMessages.MrNumberNotFound
}, JsonRequestBehavior.AllowGet);
}
id = _folderRepository.AddFolder(structureId, parent, period, folderNumber, newFolderName, publish, folderDate);
if (id == -1)
{
Logging.LogToFile("Folder ID is -1", 3);
base.Response.StatusCode = 500;
return Json(new
{
success = false,
errorAddFolderMsg = ErrorMessages.AddFolderNotPossible
}, JsonRequestBehavior.AllowGet);
}
return Json(new
{
id = id,
folderId = structureId,
newFolderName = newFolderName,
parent = parent,
period = period
});
}
Furthermore I am not sure about my namings.
Solution
To amplify what’s already been stated in comments, the concept of having two parameters and approximately ten lines of code per function (it’s called a method in C# by the way), is really just a guideline. Its purpose is to make you think about the following points (there are more, but these are some of the primary points):
- Am I following OOP?
- Does my method have a single responsibility?
- Is my method easy to understand?
Simply put, your method should serve one purpose, it should be concise and easy to maintain, and since you’re using a language that supports OOP, you should use it when needed.
Parameter Count
I wouldn’t personally say that you have too many parameters for your method. The golden rule of thumb is actually a cap of seven, but this isn’t software driven, it’s a limitation for most human beings:
Countless psychological experiments have shown that, on average, the longest sequence a normal person can recall on the fly contains about seven items. This limit, which psychologists dubbed the “magical number seven” when they discovered it in the 1950s, is the typical capacity of what’s called the brain’s working memory.
There are also several posts on Stack Exchange (here and here are two good ones) asking about the topic of parameter count. The general consensus is that as you approach four parameters, you’re either starting to do too much, or you should be creating a model to represent those parameters.
In your particular case, I think you’re fine, count wise.
Line Count
Similar to your parameter counts, I can’t say you have too many lines. However, I will say that your method isn’t focused. It currently has the following responsibilities:
- Get a new structure ID.
- Attempt to add a new folder to the repo.
- Generate error response.
- Generate full response.
Why not separate those into their own methods?
Data Models
With all of the above in mind, why not create two new data models:
public sealed class FolderSummary {
public int FolderId { get; set; }
public string StructureId { get; set; }
public string FolderName { get; set; }
public string Parent { get; set; }
public int Period { get; set; }
}
public sealed class RepoResponse<T> {
public bool Success { get; set; }
public string Message { get; set; }
public T Data { get; set; }
}
You’ll come to find that you can utilize FolderSummary to call AddFolder, and make your return data strongly typed:
public JsonResult AddFolder(FolderSummary summary) {
...
return Json(new FolderSummary(...));
}
Additionally, you can utilize RepoResponse to handle error data and the full response at the same time, adding even further consistency to your output:
public JsonResult AddFolder(FolderSummary summary) {
...
catch (Exception ex) {
Logging.LogToFile(ex, 3);
return Json(new RepoResult<string>
{
Success = false,
Message = ErrorMessages.CountFolderNotPossible,
Data = structureId
}, JsonRequestBehavior.AllowGet);
}
...
return Json(new RepoResult<FolderSummary> { Data = new FolderSummary(...) });
Results
I took the liberty to implement everything I previously mentioned to demonstrate it in action:
public JsonResult AddFolderToRepo(FolderSummary summary) {
// Nothing prior to this check impacts the check itself, so do it first and get it out of the way since you're throwing an exception.
if (!TryGetLastPeriod(out int period))
throw new Exception(ErrorMessages.PeriodNotFound);
// Create a variable for storing the folder number.
int folderNumber = 0;
// Create a generic response to handle errors and the full data as part of the JsonResult.
RepoResponse<object> result = null;
// Get the structure ID.
result = TryGetStructureId(folderNumber, summary);
if (result.Success) {
// Save the new structure ID and attempt to add the folder.
string newStructureId = result.Data;
result = TryAddFolderToRepo(result.Data, folderNumber, summary);
if (result.Success) {
int newId = result.Data;
// Update the result data to the new folder summary
result.Data = new FolderSummary {
FolderId = newId,
StructureId = newStructureId,
FolderName = summary.FolderName,
Parent = summary.Parent,
Period = period
};
}
}
// Return the structured response.
if (result.Success)
return Json(result);
else
return Json(result, JsonRequestBehavior.AllowGet);
}
private bool TryGetLastPeriod(out int period) {
period = GetLastPeriod();
return period >= 0;
}
private RepoResponse<string> TryGetStructureId(int folderNumber, FolderSummary summary) {
string resultMessage = string.Empty;
string newStructureId = string.Empty;
try {
// Attempt to get the folder ID from the summary.
if (int.TryParse(GetFolderId(summary.Id).Split('_')[0], out int parentFolderId)) {
// Attempt to get the structure ID.
if (summary.Parent.Equals("#", StringComparison.InvariantCultureIgnoreCase))
newStructureId = AddNewParentFolderId(folderNumber);
else
newStructureId = AddNewChildFolderId(summary.StructureId, parentFolderId, summary.Parent);
}
} catch (Exception e) {
Logging.LogToFile(e, 3);
resultMessage = ErrorMessages.CountFolderNotPossible;
}
return new RepoResponse<string> {
Success = !string.IsNullOrWhiteSpace(newStructureId),
Message = resultMessage,
Data = newStructureId
};
}
private RepoResponse<int> TryAddFolderToRepo(string newStructureId, int folderNumber, FolderSummary summary) {
string resultMessage = string.Empty;
int newId = -1;
try {
if (folderNumber > 0) {
string isoDate = Helper.ConvertToIsoDate(DateTime.Now.ToString().Split(' ')[0]);
bool shouldPublish = summary.Parent.Equals("#", StringComparison.InvariantCultureIgnoreCase);
newId = _folderRepository.AddFolder(newStructureId, summary.Parent, summary.Period,
folderNumber, summary.FolderName, shouldPublish, isoDate);
} else {
Logging.LogToFile("MrNumber has to be > 0", 3);
Response.StatusCode = 500;
resultMessage = ErrorMessages.MrNumberNotFound;
}
} catch (Exception e) {
Logging.LogToFile(e, 3);
resultMessage = ErrorMessages.AddFolderNotPossible;
}
return new RepoResponse<int> {
Success = newId >= 0,
Message = resultMessage,
// Return folder number if we blew up on folder number.
Data = folderNumber > 0 ? newId : folderNumber
}
}
Note: There may be spelling and syntactical errors in the code snippets due to me typing this up quickly. However, I believe the message is clear, so, I’ll entrust you to resolve those.
The whole try block( //implement publish true when it is parent - false when it is child) can be reduced to one line.
try
{
structureId = parent == # ? AddNewParentFolderId(...) : AddNewChildFolderId(...);
}
catch
{
}
No need of the publish variable.
Use "parent == #" expression in place of publish variable.
To reduce the function parameters create an object and include the related parameters as the objects properties.