3

I use common procedures for all Virtual Treeviews (TVirtualStringTree) so I only have 1 code to maintain, like for OnClick I use Common_VST_OnClick which all VST controls has set:

procedure TForm1.Common_VST_OnClick(Sender: TObject);

And to execute code based on which VST calls this on click procedure, I realized I use many different ways to recognize which control is Sender:

if Sender = VST1 then   

if Sender.Name = VST1.Name then    

if TVirtualStringTree(Sender) = VST1 then

if TVirtualStringTree(Sender).Name = VST1.Name then

if TVirtualStringTree(Sender).Name = 'VST1' then

The last is probably worst as the name is hardcoded, so I'm trying to only use 1 type of identification, in all procedures.

What is the best way to identify which control is Sender?

David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
Mike Torrettinni
  • 1,816
  • 2
  • 17
  • 47
  • 3
    Your first one is correct - `if Sender = VST1` is the most correct way to do so. However, if you're writing supposedly generic code that depends on identifying a specific component, you're probably doing something wrong. – Ken White Jan 02 '16 at 01:31
  • @KenWhite This is only to identify which VST called the procedure, not any other types of components. – Mike Torrettinni Jan 02 '16 at 02:08
  • 1
    Yes, I understand that, and I gave you the information in the first sentence. You're still restricting the use of your *generic* function to components of a specific name: `if Sender = VST1 then...else if Sender = VST2 then...` etc. If you have code that is specific to a control, use a separate event handler for each control. If the code should act on any VST control, then you don't need to know which one it is by name. – Ken White Jan 02 '16 at 02:14
  • I added a simplified example why I prefer one common procedure over multiple ones. This way I can maintain one common code instead of multiple different ones. – Mike Torrettinni Jan 02 '16 at 02:35
  • 4
    After your edit, I can say more certainly that your code is wrong. You should have a single procedure that accepts a VST and an array as parameters, and performs that common operation on that VST (no matter which one it is), and then the OnClick handler for each separate VST would call that procedure passing itself and the appropriate array. Your repeated `if Sender =` indicates that you're writing control-specific code and trying to make it generic, and it's simply not the right way to do things. You'll eventually realize that when it becomes difficult (or impossible) to maintain. – Ken White Jan 02 '16 at 02:38
  • @KenWhite In my case, as explained in Edit 2, it seems maintaining 1 common location is easier than all OnClick procedures. With an option to refactor a bit. – Mike Torrettinni Jan 02 '16 at 16:11
  • I don't understand the edits. You seem to have moved a very long way from the question. – David Heffernan Jan 02 '16 at 16:14
  • 2
    @MikeTorrettinni Your second edit does not solve your problem. In fact it only makes it worse. How? From one method you have now made three. And I assume you are still using that `if sender = ...` in your `GetData` and `GetLinkedData` methods. You should avoid having what I call `mile long if's or case statements` why because sooner or later different parts of them will start to look the same to you. So while you will be thinking that you are making changes for one VST you might be making changes that affect another VST or instead of fixing a bug you will be creating a new one. ... – SilverWarior Jan 02 '16 at 18:03
  • 1
    .. I have been there, done that and I didn't like it. Now correct approach would be to use object oriented approach and derive custom classes of your VST components so that you can add custom methods to them. These methods can then replace your current `GetData` and `GetLinkedData` from your second Edit. And because these methods are specific to each VST and are executed inside the scope of custom VST you no longer need all those `if's` to check which control is the sender instead you just call `Sender.GetData` or `Sender.GetLinkedData`. – SilverWarior Jan 02 '16 at 18:11
  • 1
    Or perhaps even better instead of directly integrating methods in the derived classes you can use method properties to gain the ability to assign custom methods to your VST in a similar way as you are assigning different event methods to your components. – SilverWarior Jan 02 '16 at 18:14
  • @SilverWarior I do not use custom VCL classes. Impossible for me to start implementing this right now (times-wise). I assume I will be able to one day convert all those long IFs, as they are all in the same location, into separate methods as needed. Right now, I need to cleanup the code from beginner level, to one level up and then, when is time, to the correct level. Starting with cleaning up these odd checking of who is Sender. – Mike Torrettinni Jan 02 '16 at 18:22
  • @MikeTorrettinni It is ultimately your call but trust me on this. Getting that mess sorted is best by making a large jump rather than taking small steps. Why? Becouse I'm afraid that by making small steps you could actually get lost. Figuratively speaking. I'm a game developer and as I have sad I have been in similar position as you are. In my case I have been using mile long nested if statements to determine the damage taken when one unit attacked the other. ... – SilverWarior Jan 02 '16 at 18:35
  • ... Because at that time I had 10 unit types and the damage dealt was calculated considering both attacker and defender unit type I had to cover 100 different possible scenarios. So the code was anything but easy understandable. Once I moved unit type specific parts to their appropriate classes my code become much more readable. But not only that I have also gained other advantages. So if I compare my current code to that before I have got next advantages. First: code is much more readable now. ... – SilverWarior Jan 02 '16 at 18:40
  • ... Second: code now allows me to easily add new nit type without having to do mayor change of existing code. Third: the current code provides about 60% better performance as it did before and no longer presents a large bottleneck that it did before. So I can easily add even more unit types and thus increase the in-game depth even further without any performance impact. And in games performance does play a very important factor. – SilverWarior Jan 02 '16 at 18:43
  • Now I won't say that achieving this would be simple but it is not so hard as you may initially think. I'm prepared to help you out on this by guiding you through the process if you want. And for you to get a better idea on this approach I want you to check my code. I tried to keep it well comented. You should specifically check TCharacter.Attack and TCharacter.Defend methods as they employ this approach. You can get my code here: https://dl.dropboxusercontent.com/u/65300398/Programiranje/RpgBattleSystem.rar – SilverWarior Jan 02 '16 at 18:50
  • 1
    Sub classing VST in the way that @Silver says is an appalling idea. Really shockingly bad. – David Heffernan Jan 02 '16 at 19:32
  • I reverted the edits to the question to avoid the spec creep. It's really not right to drift away from a clean precise question to a code review. If you want to have a code review, use the SE site for code review. – David Heffernan Jan 02 '16 at 21:24
  • Good, I agree about edits. Did not know about SE code review site. – Mike Torrettinni Jan 02 '16 at 22:35

1 Answers1

4

You should prefer the test that uses object identity. That is, the first test in your question:

if Sender = VST1 then

An object reference such as Sender or VST1 is the address of the object. If two such addresses are equal, then the references point to same object. And vice versa.

The tests based on control name can work but are brittle. It is possible for multiple controls to have the same name. It is possible to change the control name but not update all the uses of the name in the program.

As for the type casting option

if TVirtualStringTree(Sender) = VST1 then

the type cast has no impact on object identity and so is needless. Don't ever type cast an operand to an object identity test since doing so is spurious.

David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
  • Great, around 200 LOC changed and works as expected! Code much cleaner. – Mike Torrettinni Jan 02 '16 at 22:36
  • @DavidHeffernan sorry for reviving this but its better to just ask here than make a new post. If I do if Sender = Button1 then can I do ButtonTag = (Sender as TButton).tag and use that tag then to delete an object from a list? so ObjectList.delete(ButtonTag-1) I'd have the -1 because my Tags start at 1. –  Apr 10 '17 at 18:17
  • @miketorrettinni do you know? –  Apr 10 '17 at 18:24
  • @Not surely you know how to count – David Heffernan Apr 10 '17 at 18:24
  • @davidheffernan I do yeh, I thought TObjectList start at index 0? –  Apr 10 '17 at 18:26
  • Yes it does. Is that your question. You could just read the documentation. Or perhaps the documentation is not helpful. In which case read around, and experiment to test your hypothesis. – David Heffernan Apr 10 '17 at 18:32
  • @Davidheffernan I was trying to figure out why I kept getting an error when deleting a button. I thought that might have been the problem but then i realised i was trying to delete the button after deleting it, causing the error. However, the main problem is differentiating which button has been deleted. As you probably know, deleting an object in a list moves everything up one place so i thought ill just take away the amount of deleted buttons. Main problem with that is if they delete e.g button 3,4,5 and then try delete button 1, it will try to delete button-2 which doesn't exist. Any ideas? –  Apr 10 '17 at 19:16
  • @DavidHeffernan would it be deemed off topic if i were to ask a question on the maths forum. I believe my problem involves maths but because it involves programming it might be deemed off topic –  Apr 10 '17 at 19:31