1
\$\begingroup\$

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.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ what is the difference between strcture id and folder id ? could you provide a data sample data for both ? \$\endgroup\$
    – iSR5
    Aug 11, 2021 at 19:47
  • 1
    \$\begingroup\$ also, please provide AddNewParentFolderId and AddNewChildFolderId codes as well. \$\endgroup\$
    – iSR5
    Aug 11, 2021 at 19:49
  • 3
    \$\begingroup\$ Don't think of guidelines as inescapable hard rules, because they are not. There is no hard limit on method arguments or method body line count. That's not to say you don't have to look for improvements, but it's a matter of setting realistic expectations because otherwise you are prone to falling into an overkill trap. \$\endgroup\$
    – Flater
    Aug 13, 2021 at 9:03

2 Answers 2

4
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for your answer. So would you furthermore say, that I can make a own Model for _folderRepo.Addfolder(AddFolderModel newFolder)? Or is this bad practice? \$\endgroup\$ Sep 1, 2021 at 14:05
  • \$\begingroup\$ @SpedoDeLaRossa assuming that _folderRepository is an instance of a type that you created, then yes, you could create a model for it; however, the signature could be as simple as summary, newStructureId, folderNumber, publish, isoDate, not really a need for a whole new model there IMO because it currently appears rather niche. \$\endgroup\$ Sep 1, 2021 at 15:29
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking β€œPost Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.