Windows forms set saved groupbox location settings

Question

I have a Windows form that when a user moves some groupboxes around, before the application exits, it saves those X and Y coordinates into a MySQL database (with a bit of added text to identify which groupbox those settings belong to). This all works fine. When they log back into the application, I can pull those settings too.

Is this the most efficient way to accomplish parse & apply saved settings from a string?

Here's the working function that's grabbing this text string and parses it:

private void apply_GUI_settings(string settings)
{
 // The string below, GUI_settings, will evaluate to: "[1:{X=7,Y=29}][2:{X=52,Y=433}]"
 string GUI_settings = settings;
 bool startFound = false;
 for (int c = 0; c < GUI_settings.Length; c++)
 {
 switch (GUI_settings[c])
 {
 case '[':
 {
 startFound = true;
 }
 break;
 case '1': // groupBox_AccountInfo.Location settings
 case '2': // groupBox_YourCharacters.Location settings
 {
 string which_groupBox = GUI_settings[c].ToString();
 if (startFound)
 {
 c++; // Iterate over the "1"
 if (GUI_settings[c] == ':')
 {
 int n = 0;
 string X = "";
 string Y = "";
 c += 4; // Iterate over the ":{X="
 while (int.TryParse(GUI_settings[c].ToString(), out n))
 {
 c++;
 X += n.ToString();
 }
 n = 0;
 c += 3; // Iterate over the ",Y="
 while (int.TryParse(GUI_settings[c].ToString(), out n))
 {
 c++;
 Y += n.ToString();
 }
 n = 0;
 if (which_groupBox == "1")
 groupBox_AccountInfo.Location = new Point(int.Parse(X), int.Parse(Y));
 else if (which_groupBox == "2")
 groupBox_YourCharacters.Location = new Point(int.Parse(X), int.Parse(Y));
 // Might as well cycle thru the rest of this string until ']'
 while (GUI_settings[c] != ']')
 c++;
 }
 else
 {
 MessageBox.Show("[ERROR:53928]");
 }
 startFound = false;
 }
 else
 {
 continue;
 }
 }
 break;
 }
 }
}

Answer 1

Parsing the settings strings character by character is troublesome and can be hard to get it right.

It would be better to use a regex to match the string and extract the relevant parts using capture groups, for example:

\[(\d+):\{X=(\d+),Y=(\d+)\}\]

Regular expression visualization

Debuggex Demo

I know too little of C# to give a sample implementation, in case it helps, here it is in Java:

Pattern pattern = Pattern.compile("\\[(\\d+):\\{X=(\\d+),Y=(\\d+)\\}\\]");
String sample = "[1:{X=7,Y=29}][2:{X=52,Y=433}]";
Matcher matcher = pattern.matcher(sample);
while (matcher.find()) {
 int id = Integer.parseInt(matcher.group(1)),
 int x = Integer.parseInt(matcher.group(2)),
 int y = Integer.parseInt(matcher.group(3))
 // do what you need with id, x, y
}

• Zyre: Thank you, Janos. This looks like a far better route to take.

Answer 2

Exception Handling

MessageBox.Show("[ERROR:53928]");

I've a few issues with this.

1. That message will mean absolutely nothing to the end user. You should always strive to give end users meaningful, clear, and non-technical messages.
2. This method shouldn't be responsible for displaying messages to the user anyway. You should be throwing an exception instead. Let the calling code figure out what to do with the error. Most likely, it should pass it off to a logger and maybe display a message. I don't have enough context to actually say what the correct thing would be.

Naming

It's standard style to use PascalCase for classes and methods; camelCase for variables and arguments. At no time should there be underscores in your names. It's not a big deal if you will be the only developer to ever touch the code, but if you work on a team or expect to open source the code, it will be important to code in a style that other C# devs expect. For more information, please see The Official Microsoft C# Style Guide.

• Zyre: The "ERROR" message is for me personally for reference while debugging and will be removed when code goes into production. Your reply/post is off topic and does not contain any contextual constructive advice.
• Phrancis: That's all well and good. Next time it would be best to state this in your question to avoid confusion.
• RubberDuck: If you read our help center, you'll find that "Any and all facets of the code" is up for review. I felt your question had been addressed, but saw other issues with your code that rightfully should be mentioned. Your code should be complete, working, and as good as you can make it before posting a question here. If you explicitly plan on changing the code prior to production, it is not ready for review. I do not appreciate that both my time and yours has been wasted because of it.

Answer 3

From your code snippet I assume that it's part of a Winforms form. This indicates somewhat of a design problem as it's violating the Single Responsibility Principle and your representation of the information (winforms controls) are very tightly coupled to the serialization which in total will it make hard to unit test properly.

I would therefor suggest two things:

1. Create a POCO class which holds the positional information. Something like this:

   public enum ControlType
   {
    AccountInfo,
    Characters
   }
   public class ControlPosition
   {
    public readonly ControlType Target;
    public readonly int X;
    public readonly int Y;
    public ControlPosition(ControlType target, int x, int y)
    {
    Target = target;
    X = x;
    Y = y;
    }
   }
   

2. Extract the serialization code into a dedicated serialization class. Something along these lines

   public class UISerializer
   {
    public string Serialize(ControlPosition position)
    {
    // serialize into your format
    }
    public ControlPosition Deserialize(string value)
    {
    // Deserialize into the actual settings object
    }
   }
   

With this in place your main code in the form would be reduced to:

private void ApplySettings(string settings)
{
 ControlPosition position = new UISerializer().Deserialize(settings);
 Point point = new Point(position.X, position.Y);
 switch (position.Target)
 {
 case ControlType.AccountInfo: 
 groupBox_AccountInfo.Location = point; 
 break;
 case ControlType.Characters: 
 groupBox_YourCharacters.Location = point; 
 break;
 }
}

Ideally you would inject the serializer into the class so you can mock it out for unit testing (in which case providing an interface for it will make mocking easier).