Poor programming practice?

[Cephalopod]

Someone on another site just told me what I am doing is poor practice.

What I do is I load a line of of data in my program, process it (split it up, register hotkeys). I've created a class to do all of this. When it comes to processing this data (aka when someone presses a key), it pulls some info from the form (info like round robin, class specific hotkeys) and performs the actions. It doesn't search the listview, as the index of the hotkey matches the index of the listview.

I was told what I should do is load the data into a multidimensional array (processed) or a arraylist (unprocessed) and then copy it into the listview on the form, then when it comes to processing the keypresses I pull the data from either the multidimensional array and proceed like normal or pull it from the arraylist, process it then proceed.

Arraylist seems stupid, as I process it all when adding it to the listview.
Array seems odd, as if the data is in the listview on the form why bother creating an array?

Speed is irrelevant here, but why is what I am doing considered poor practice?

---

My program both hooks the keyboard and allows the user to register hotkeys, I prefer the later. Just adding this so no one goes OT and recommends keyboard hooks.

[Apoc]

Post some code? What you're explaining is not making much sense.

[Cephalopod]

Posting code will be difficult, so I'll explain it further. If I still fail, then I'll go through and post the important sections of code.

When my program starts it loads a few files here and there, one of them is the file that contains my keybindings. Here is an example of what the file contains:

NONE:179:touchplay:NONE:179:touchplay:HOTKEYS
NONE:480:NONE:480:ALL
NONE:491:NONE:491:ALL
NONE:502:NONE:502:ALL
NONE:513:NONE:513:ALL
NONE:524:NONE:524:ALL
NONE:535:NONE:535:ALL
NONE:546:NONE:546:ALL:mages+1
NONE:557:NONE:557:ALL
NONE:568:NONE:568:ALL
CTRL:491:CTRL:491:ALL

Format is: Modifier Pressed:keycode pressed:key:Modifier to Send:Keycode to send:key to send:who to send to:special commands

It analyses each line, registers the hotkeys and then places the data into a listview sorted into the relevant columns.

When a hotkey is pressed, my program recieves the message and it is sent through to my class which handles keypresses and related functions (the code to load these keybindings is in the same class). My program then grabs the index of the hotkey and then reads the same line in the listview (located on the form) to figure out what it has to do. It figures it out, and then performs the actions.

This person told me this was a bad programming practice as I should not be reading from a listview on the main form on such a regular basis. Instead he suggested I load the above keybindings (I have many more, around 25 + the keyboard hook bindings but they're handled seperately) into an arraylist, then when I recieve the hotkey message, anaylse the line (same way, index of hotkey will = index in arraylist) and figure out how to perform the message.

So basically, what he is advocating is that I create an aditional arraylist and analyse the string every time a key is pressed rather than do it once and read it from a listview control. As you can see from the above format, each split (between the semi colons) goes into it's own column, so it's very easy for me to pluck the relevant information from the listview where if I stored it in an arraylist I'd have to perform a split every time a key is pressed.

[Apoc]

Ok now I understand ya!

And yes, I agree. It is bad programming practice. A GUI is just to present information to the user. You should never use it to store things you actually use in your working code.

My suggestion: Make a small Hotkey class that holds your information. And store that in a List<Hotkey>.

Example:

        public class Hotkey
        {
            public static List<Hotkey> Hotkeys = new List<Hotkey>();

            public string ModifierPressed { get; set; }
            public int KeyCode { get; set; }
            public string Key { get; set; }
            public string ModifierToSend { get; set; }
            public string SendTo { get; set; }
            public string WhoTo { get; set; }
            public string SpecialCommands { get; set; }

            public static void LoadHotkeys(string inputFilePath)
            {
                if (File.Exists(inputFilePath))
                {
                    using (TextReader tr = new StreamReader(inputFilePath))
                    {
                        string line = tr.ReadLine();
                        while (!string.IsNullOrEmpty(line))
                        {
                            string[] hotkeyInfo = line.Split(new[] {':'}, StringSplitOptions.RemoveEmptyEntries);
                            //NONE:179:touchplay:NONE:179:touchplay:HOTKEYS
                            Hotkey h = new Hotkey
                                           {
                                               ModifierPressed = hotkeyInfo[0],
                                               KeyCode = Convert.ToInt32(hotkeyInfo[1]),
                                               Key = hotkeyInfo[2],
                                               ModifierToSend = hotkeyInfo[3]
                                           };
                            if (hotkeyInfo.Length > 5)
                            {
                                h.SendTo = hotkeyInfo[5];
                            }
                            if (hotkeyInfo.Length > 6)
                            {
                                h.WhoTo = hotkeyInfo[6];
                            }
                            if (hotkeyInfo.Length > 7)
                            {
                                h.SpecialCommands = hotkeyInfo[7];
                            }
                            Hotkeys.Add(h);

                            line = tr.ReadLine();
                        }
                    }
                }
            }

            public static Hotkey GetKey(int keyCode)
            {
                //LINQ:
                //var ret = from hotkey in Hotkeys
                //          where hotkey.KeyCode == keyCode
                //          select hotkey;
                //return ret.FirstOrDefault();

                // Regular old foreach
                foreach (Hotkey hotkey in Hotkeys)
                {
                    if (hotkey.KeyCode == keyCode)
                    {
                        return hotkey;
                    }
                }
                return null;
            }

            /// <summary>
            /// Gets the GUI item. Nifty for using lstViewHotkeys.Items.Add(hotkey.GuiItem);
            /// </summary>
            /// <value>The GUI item.</value>
            public ListViewItem GuiItem
            {
                get
                {
                    // Obviously need to change this to match your list view.
                    
                    string[] texts = new[]
                                         {
                                             ModifierPressed, KeyCode.ToString(), Key, ModifierToSend, SendTo, WhoTo,
                                             SpecialCommands
                                         };
                    return new ListViewItem(texts);
                }
            }
        }

Probably not exactly what you're looking for, but it gets the point across.

[Cephalopod]

Thanks a ton.

Will change my code over so it doesn't pull data from forms.

[suicidity]

Yeah, the ONLY thing I pull data from when I'm doing a quick debug is Stuff displayed from my ListView's;

But I always make sure to go back and properly make the structures / classes then properly list<> them etc.

[Apoc]

Yeah, the ONLY thing I pull data from when I'm doing a quick debug is Stuff displayed from my ListView's;

But I always make sure to go back and properly make the structures / classes then properly list<> them etc.

That's perfectly ok. Pulling the text of a selected item to run in a method such as Search(string name) is perfectly viable. However, using it as a List<T> to store information critical to your app is just plain bad practice.

[suicidity]

That's what I'm agreeing with.

Things like my pathing information, I have a listview to display the information; But behind that is a List<Node> which holds the pathinformation, the ListView just displays that information. Any calls to generate a path or anything go through the List, not ListView.

[Cephalopod]

Another question.

I know using GoTo is generally poor practice but I can't figure out how to goto the end of the loop another way.

loop {

switch {
//break from here
}

}

I want to go to the end of the loop (as opposed to breaking the loop, so the loop continues) from inside the switch. I can't think of any ways to do it.

[Link_S]

you could use the break option

//Example with a while loop

bool b = true;
while(true)
{
if(b == true)
{
continue; //Continue the while loop
}

if(b == false)
{
break; //Break the while loop , ends the application in this case
}

//Code to handle input, and change the b value
}

[Cephalopod]

I thought break would break the switch, rather than the loop?

[Apoc]

The break keyword has the same uses in both switch statements and loops.

It 'breaks' out of switches, and also 'breaks' out of loops.

while (true)
{
    try
    {
        // Code here that will eventually throw an exception, such as EndOfStreamException
        // for file reads
    }
    catch
    {
        break; // Break out of the loop so it's no longer an infinite loop
    }
}

The same is true for 'for' loops, and 'foreach' loops.

[Cephalopod]

Yeah, but I've got a switch inside my loop & I need to break the loop cycle from within the switch.

break inside the switch would break out of the switch as they always break out of the innermost enclosed statement, so it'd still allow the rest of the code in the loop to be executed. Right now I've just created a label at the end of the loop titled EndOfLoop and when I need to break the loop cycle I just use GoTo EndOfLoop; but my first c# book I read (Illustrated C# 200 told me that I should use goto with extreme caution and it's looked down upon.

The only other option is to have a default in my switch which encloses all of the code for the routine, but I'd rather not do that.

[Apoc]

Yeah, but I've got a switch inside my loop & I need to break the loop cycle from within the switch.

break inside the switch would break out of the switch as they always break out of the innermost enclosed statement, so it'd still allow the rest of the code in the loop to be executed. Right now I've just created a label at the end of the loop titled EndOfLoop and when I need to break the loop cycle I just use GoTo EndOfLoop; but my first c# book I read (Illustrated C# 200 told me that I should use goto with extreme caution and it's looked down upon.

The only other option is to have a default in my switch which encloses all of the code for the routine, but I'd rather not do that.

The way you're doing it is perfectly valid. (As it's one of the only clean ways of breaking out of a switch, nested in a loop)

[Cephalopod]

Ahhh, great.

Sorry if these seem like stupid questions, I've just noticed that very few books don't focus on programming practices and I often get stuck in bad habits so everything I have second thoughts on I check out.

+rep, or not, need to spread it around :P