1
\$\begingroup\$

I have a c# application which displays the names of assessments and the zone which they are performed upon, separated by a comma and a space. For instance:

  • assessment1, Firewall
  • mom's assessment, LAN

When the user selects on of these assessments, I need to separate the assessment name from the zone name. I also need to replace any single quotes in the string by two single quotes so I can query my database without issues (this i cannot change, it has to be done because no SQLiteParameters are used in the function that queries my database).

This is how I do this currently (note that _view.SelectedAssessment is the entire string of both the assessment name and zone):

string zone = _view.SelectedAssessment.Substring(_view.SelectedAssessment.IndexOf(", ") + 2);
string assessmentName = _view.SelectedAssessment.Substring(0, _view.SelectedAssessment.Length - zone.Length - 2);

Is there a better way to do this? Any improvements?

\$\endgroup\$
  • \$\begingroup\$ no SQLiteParameters are used in the function that queries my database - why so? \$\endgroup\$ – t3chb0t Dec 15 '16 at 9:16
  • \$\begingroup\$ @t3chb0t This was already in place. I did change the insert function to use SQLiteParameters. But changing the query function would require me to rewrite way too much existing code. \$\endgroup\$ – Marthe Veldhuis Dec 15 '16 at 10:03
  • 3
    \$\begingroup\$ requiring rewriting "way too much existing code" for the benefit of having code that's basically secure against SQL-Injection is not rewriting "way too much code" ... If it were me, I'd talk to whoever's saying that it's way too much code and try to make them see reason. Because that's a walking exploit in your application \$\endgroup\$ – Vogel612 Dec 15 '16 at 12:03
  • 1
    \$\begingroup\$ I understand what you are saying, but you do not know this situation. 1. I am a student on an internship with zero prior knowledge of building any application, this is my third year computer science and I just have to make it work. 2. The company knows nothing about programming so they cannot help me. 3. They just want an application that automates their report making, so only about five people will every use it. 4. It is not connected to the internet. \$\endgroup\$ – Marthe Veldhuis Dec 15 '16 at 14:42
  • \$\begingroup\$ Don't forget that other people might want to learn something out of your question's answers make sure you always mark the one that helped you the most. \$\endgroup\$ – Denis Dec 16 '16 at 22:25
6
\$\begingroup\$

Just use the Split method

 string[] splittedArray = _view.SelectedAssessment.Split(new[]{','});
 string zone = splittedArray[1].Trim();
 string assessmentName = splittedArray[0].Trim();

Also be careful when doing a manual replace on strings that go into the DB as you might be vulnerable to SQL injection

\$\endgroup\$
  • 1
    \$\begingroup\$ This blows up when there's more than one comma in the input. not that the current code handles that part well :) \$\endgroup\$ – Vogel612 Dec 15 '16 at 12:07
  • 3
    \$\begingroup\$ @Vogel612 for sure, but not knowing if there might be such cases I prefer simple solutions \$\endgroup\$ – Paweł Łukasik Dec 15 '16 at 12:10
  • \$\begingroup\$ I have decided to use your solution because it is very simple and easy to use! I understand that Regex is cool and you can do much more with it. However, I will only use this piece of code once so I don't need any bells and whistles or flexibilities. \$\endgroup\$ – Marthe Veldhuis Dec 19 '16 at 7:16
7
\$\begingroup\$

Make it regex with named groups:

var matcher = new Regex(@"(?<assesment>.+?),\s*(?<zone>.+)");
var match = matcher.Match("assessment1, Firewall");
var assesment = match.Groups["assesment"].Value;
var zone = match.Groups["zone"].Value;

where

  • (?<assesment>.+?) - catches assesment ungreedy ? so that it stops at ,
  • ,\s* - the separator is , with an optional white space (zero or more times)
  • (?<zone>.+) - catches zone greedy

Sample at regex101.com


I wasn't aware of that but @Vogel612 kindly mentioned that backtracking as I use it in my solution above might be sometimes a problem: Why Using the Greedy .* in Regular Expressions Is Almost Never What You Actually Want and The Explosive Quantifier Trap.

Using the (?<assessment>[^,]+) expression would be much faster and safer here.

That change drops the match you propose from 30 steps until matching to 9, so it saves roughly 60% of execution time.


Another valuable remark by @forsvarir suggests using the statick Regex.IsMatch method over creating a new instance each time. This should be much faster here.

The difference in performance is due to the caching of regular expressions used in static method calls.

There is a lot more to that with pleny of examples on Optimizing Regular Expression Performance, Part I: Working with the Regex Class and Regex Objects [Ron Petrusha]


Why do I prefer the regex solution?

  • I can test it with regex101
  • I can use the same pattern for validation and data extraction. I know whether the data is in the correct format without checking each element of the array values[i] separately.
  • I can easier restrict the characters or casing or splitting rules
  • I can use named groups which I find easier to understand because I can see what I work with rather then just having a string[] especially that each filed has a meaning - otherwise I'd use the Regex.Split method if was just an array of some values
  • I can trim the data at the same time (requires some additional \s*)
  • I can use the same pattern for Regex.Replace if I wanted to change it
\$\endgroup\$
  • 4
    \$\begingroup\$ What are the advantages of this over the Split method mentioned below? \$\endgroup\$ – Marthe Veldhuis Dec 15 '16 at 14:47
  • \$\begingroup\$ @MartheVeldhuis I've added a few reasons why I prefer to use regex here. \$\endgroup\$ – t3chb0t Dec 18 '16 at 10:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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