Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestions #3

Open
PonchoPowers opened this issue May 14, 2022 · 2 comments
Open

Suggestions #3

PonchoPowers opened this issue May 14, 2022 · 2 comments

Comments

@PonchoPowers
Copy link

First off, I would like to say thanks for the hard work and effort you've put into the library.

Before moving on I would like to say that I'm not bashing your code in any way, had you not carried out the work you did I would not be able to control my Wiz lights as I would have found it hard to translate the Python and Java code, whereas you have managed to, which I consider an achievement within itself.

I'm just reviewing it and I have some suggestions:

  • BulbMethod has a flag for methods that are inbound only, but has no flat to indicate outbound methods, I personally think the class should be split up so that inbound methods and outbound methods are defined into InboundBulbMethod and OutboundBulbMethod or something of that nature.
  • You should allow for people to pass an option to specify the number of bulbs they have if known, as this will allow you to break from the where loop if all the bulbs expected have been found, making scanning a lot quicker.
  • BulbCommand mixes both transformation and domain concerns, the properties should be split from the transformation methods into a separate class to reduce the complexity of the code.
  • The MAC address conversion is pointless, there is no real reason to overcomplicate it in the way you have, you could have just used PhysicalAddress
  • There is a bug somewhere in which if you know the MAC address of the bulb and have not performed a scan, the datagram is not sent to the bulb, I'm still looking into where this issue is being caused and will open a separate bug report for this.
  • The scanning of bulbs and caching of bulbs found should be separated out, as the two are currently mashed together in the ScanForBulbs method along with various other concerns, again to reduce the complexity of the code.
  • Your use of defining the constant DefaultPort and then the code setting the value of it to a variable int port = DefaultPort; seems pointless to me, you could have just made reference to the const.

I haven't finished going through everything yet, I only started doing so as I was having serious performance issues when trying to control 10 lights every 10ms, and I was trying to track down where they might be. I will add more suggestions as I work through the code and report back here.

Once again, a huge thanks for your efforts 👍

@PonchoPowers
Copy link
Author

  • When scanning for bulbs, udpClient.Send(buffer, buffer.Length, "255.255.255.255", port); should be udpClient?.Send(buffer, buffer.Length, "255.255.255.255", port); (checking to see if updClient is null or not) as you have done later in the code.
  • tdelc should be renamed to something people can understand, so they don't have to trawl through the code to understand what is gong on.
  • At the start of the while loop when scanning for bulbs, if udpClient is null you should break from the while loop as there isn't much you can do if udpClient ends up null.
  • if (from.Address.Equals(localAddr)) continue; should be used over if (from.Address == localAddr) continue;
  • Why are you cloning the local IP address when scanning? localAddr = localAddr.Clone();? The same thing is done elsewhere in the code too. Are you trying to account for if the IP address were to suddenly change and if so what difference would it make?
  • I think you should explain the point of registering the bulbs, I think the idea is to cloud sync the device settings across all devices from one main device as they are not all stored on the bulb, the name of the light being one of the pieces of information you would expect to be stored on the light, as is the case with Lifx bulbs, but isn't.

@PonchoPowers
Copy link
Author

I've spotted a couple more suggestions:

  • allUdpActive is set but not checked to see if it is already true when calling ScanForBulbs, meaning you can potentially carry out multiple scans at the same time.
  • Because ScanForBulbs is an async method it should be named "ScanForBulbsAsync"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant