Cleaning small block of code (Switch, Case, If?)

Question

I hear that there are many ways to clean a code and make it perform quicker. Can you help me clear the code below. I have a lot of coding on my program that looks like the code below. I'm still a beginner at C#

case "jarvis":
    if (ranNum == 1) { QEvent = ""; JARVIS.Speak("Yes sir"); }
    else if (ranNum == 2) { QEvent = ""; JARVIS.Speak("Yes, whats up?"); }
    else if (ranNum == 3) { QEvent = ""; JARVIS.Speak("Yes, I'm here"); }
    else if (ranNum == 4) { QEvent = ""; JARVIS.Speak("I'm here"); }
    else if (ranNum == 5) { QEvent = ""; JARVIS.Speak("go head sir, "); }
    else if (ranNum > 5) { QEvent = ""; JARVIS.Speak("I'm listening"); }
    break;

This question is better suited for codereview.stackexchange in my opinion — User 12345678, Sep 12 '13 at 02:49

p.s.w.g · Answer 1 · 2013-09-12T03:08:10.113

9

The main benefit of using clean code is not to make it perform more quickly—although that's often a consequence—but to make it easier to maintain and modify if the requirements change.

That said, I'd recommend storing all the strings you might pass to JARVIS.Speak in an array like this:

string[] javisSays = new[] { 
    "Yes sir", 
    "Yes, whats up?", 
    "Yes, I'm here", 
    "I'm here", 
    "go head sir, ", 
    "I'm listening" 
};

Then you can structure your case statement like this:

case "jarvis":
    if (ranNum > 0)
    {
        QEvent = "";
        var quote = jarvisSays[Math.Min(ranNum, jarvisSays.Length) - 1];
        JARVIS.Speak(quote);
    }
    break;

edited Sep 12 '13 at 03:08

answered Sep 12 '13 at 02:51

p.s.w.g

146,324
30
291
331

just for the question asker, this method means you can just add more responses and it will automatically have more replies, however wherever you generate your random number, you should constrain it by the length of jarvisSays – Keith Nicholas Sep 12 '13 at 02:57
Would not a List or dictionary be better? And then `if myList.Contains(ranNum) {myList[ranNum]};` – Austin T French Sep 12 '13 at 03:01
4

@AthomSfere You could do that, but unless your dynamically adding rows, or the key values (`ranNum`) contain gaps, I don't really see the benefit of that over simple array. – p.s.w.g Sep 12 '13 at 03:07
Hmm, I was under the impression as a general rule a List<> was faster, more efficient. Otherwise I would use your code but with List instead of Array, if its wrong its good to know – Austin T French Sep 12 '13 at 03:16
1

@AthomSfere For simple, per-index lookup, a `List` and an `T[]` have the same performance. In fact the `List` class uses an array internally (which is why it has the same performance). However, there are lots of times when a list would be faster (e.g. dynamically adding / removing items) because it uses a fairly good strategy for efficiently resizing the internal array. – p.s.w.g Sep 12 '13 at 03:21

score 2 · Answer 2 · answered Sep 12 '13 at 02:56

2

create method to return the speak text by giving random number then

case "jarvis":
    if(ranNum >0)
    {
        QEvent = "";
        JARVIS.Speak(GetQuote(ranNum));
    }
    break;

answered Sep 12 '13 at 02:56

Damith

62,401
13
102
153

score 0 · Answer 3 · answered Sep 12 '13 at 02:52

0

Could do a switch statement.

switch (ranNum )
        {
            case 1:
             QEvent = ""; JARVIS.Speak("Yes sir");
            break;
            case 2:
             QEvent = ""; JARVIS.Speak("Yes sir");
            break;
        }

answered Sep 12 '13 at 02:52

Ryan Schlueter

2,223
2
13
19

I think it's still the same ;) – Pyromancer Sep 12 '13 at 03:34

tinstaafl · Answer 4 · 2013-09-12T04:40:40.017

Since the use of a switch/case block and 'jarvis' being one case would indicate that there may be more than one speaker and each one may have something different to say, a Dictionary>, would be well suited here, making the collection of speakers and the phrases each one can speak more dynamic.

Then it would look something like this:

        string speaker;
        Dictionary<string, List<string>> Speakers = new Dictionary<string, List<string>>();
        Speakers.Add("jarvis",new List<string>{"Yes sir", 
                                                "Yes, whats up?", 
                                                "Yes, I'm here", 
                                                "I'm here", 
                                                "go head sir, ", 
                                                "I'm listening" });
        switch(speaker)
            case "jarvis":
                QEvent = ""; JARVIS.Speak(Speakers[speaker][rannum]);
                break;

With this it would be a simple matter to set up a .csv or even .xml file to hold the data, and add it to the dictionary.

Cleaning small block of code (Switch, Case, If?)

4 Answers4