-2
\$\begingroup\$
private eFileStatus CheckConfigSettings()
{
            mFileStatus = mXMLHelper.CheckElement(eElementName.StartLanguage.ToString(), out mContent);
            if (eFileStatus.OK != mFileStatus)
            {
                return mFileStatus;
            }
            else if (eFileStatus.OK == mFileStatus && mContent.Length == 0)
            {
                mFileStatus = ShowInputFormForElement(eElementName.StartLanguage);
                if (eFileStatus.OK != mFileStatus)
                {
                    return mFileStatus;
                }
            }

            mFileStatus = mXMLHelper.CheckElement(eElementName.ExportPath.ToString(), out mContent);
            if (eFileStatus.OK != mFileStatus)
            {
                return mFileStatus;
            }
            else if (eFileStatus.OK == mFileStatus && mContent.Length == 0)
            {
                mFileStatus = ShowInputFormForElement(eElementName.ExportPath);
                if (eFileStatus.OK != mFileStatus)
                {
                    return mFileStatus;
                }
            }
}

eFileStatus is an enum and so is mFileStatus. Basically, CheckElement() checks if an element in an XML is empty or not. Next, if there is an error (it is not equal to eFileStatus.OK) then it will return the value of mFileStatus. Otherwise, if there is no error and the element is empty, it will call the ShowInputFormForElement() method to enable the user to fill up the value for the element.

There are more than two elements to check and I would like to learn how to do this better.

\$\endgroup\$
4
  • 2
    \$\begingroup\$ This code cannot compile - you don't have a "fall-through" return value. \$\endgroup\$ Apr 1, 2013 at 14:48
  • \$\begingroup\$ @JesseC.Slicer You mean by fall-through that his 'return' statements are wrapped in if statements, which may or may not be hit? \$\endgroup\$ Apr 3, 2013 at 15:14
  • 1
    \$\begingroup\$ @IsaiahNelson that's the reason why, yes. But the code simply won't compile noting that not all paths will return a value. \$\endgroup\$ Apr 3, 2013 at 15:40
  • 2
    \$\begingroup\$ -1 for not posting code (yes, I realize that the question used to have code). Note that if your goal in removing your code was to prevent people from seeing private code, your goal has not been accomplished. \$\endgroup\$
    – Brian
    Jul 12, 2013 at 19:51

2 Answers 2

9
\$\begingroup\$

It would be straightforward to extract the repeated code into a function. However, this would still leave a lot of room for improvement:

Separation of concerns: Your function does several unrelated things: reading a configuration file, some input validation, and user input. These things should be separated.

User experience: Your function presents a separate form for each configuration parameter. In the worst case, if the configuration file is empty, the user will be confronted with a series of forms, where each form asks for a single parameter. This is not very user friendly.

Therefore I would suggest to refactor your code as follows:

  • introduce a Configuration class to hold the configuration
  • this class should be able to read and write itself to the configuration file
  • it should also be able to validate itself
  • have a separate configuration UI, that lets the user enter and edit the configuration in a suitable manner
  • finally, tie everything together with a function that uses the pieces above to read the configuration, validate it, and if it fails validation, to show the configuration UI.
\$\endgroup\$
0
\$\begingroup\$

Without re-architecting things too much, you might try:

List<T> elementsToCheck = new List<T> { eElementName.StartLanguage, eElementName.ExportPath };

foreach (T ele in elementsToCheck)
{
        mFileStatus = mXMLHelper.CheckElement(ele.ToString(), out mContent);
        if (eFileStatus.OK != mFileStatus)
        {
            return mFileStatus;
        }
        else if (eFileStatus.OK == mFileStatus && mContent.Length == 0)
        {
            mFileStatus = ShowInputFormForElement(ele.StartLanguage);
            if (eFileStatus.OK != mFileStatus)
            {
                return mFileStatus;
            }
        }
}

This type of iterative structure may be a good start to a configuration management class.

\$\endgroup\$

Your Answer

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