Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I have this code:

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.

Thanks!

share|improve this question
2  
This code cannot compile - you don't have a "fall-through" return value. – Jesse C. Slicer Apr 1 at 14:48
@JesseC.Slicer You mean by fall-through that his 'return' statements are wrapped in if statements, which may or may not be hit? – Isaiah Nelson Apr 3 at 15:14
1  
@IsaiahNelson that's the reason why, yes. But the code simply won't compile noting that not all paths will return a value. – Jesse C. Slicer Apr 3 at 15:40

2 Answers

up vote 0 down vote

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.

share|improve this answer
up vote 8 down vote

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.
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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