Verbesserungsvorschläge für Code



  • Hallo zusammen,
    mich hat in einem anderen Forum jemand angeschrieben, ob ich ihm etwas kleines programmieren kann. Da derjenige schon etwas älter ist, habe ich freundlich beschlossen ihm zu helfen. Und da ich bei Dritten möglichst sauberen Code/Anwendungen weitergeben möchte, wollte ich mal nach Verbesserungsvorschlägen fragen.

    Die Aufgabenstellung: Benötigt wird ein Tool, welches bei einer Tastenkombination (Kombinationen mit STRG sind ausreichend) ein Fenster in den Vordergrund bringt, die Maus positioniert und danach einmal klickt. Möglichst noch mit mehreren kombinationen und Fenstern.

    Folgendes habe ich daraufhin geschrieben:

    #include <iostream>
    #include <string>
    #include <fstream>
    #include <Windows.h>
    #include <thread>
    #include <nlohmann/json.hpp>
    
    
    
    class KeyHandler {
    
    
    public:
    	KeyHandler(const std::string &windowTitle, const std::string &key, const unsigned int &xPos, const unsigned int &yPos)
    		: windowTitle(windowTitle), key(key), xPos(xPos), yPos(yPos) {
    
    	}
    
    	bool keyPressed() const {
    		if (GetAsyncKeyState(key[0]) && GetAsyncKeyState(VK_LCONTROL))
    			return true;
    		else
    			return false;
    	}
    
    	void doClick() const {
    
    		//Fenster suchen
    		HWND wndH = FindWindow(NULL, windowTitle.c_str());
    
    		if (wndH == 0) {
    			std::cout << "Fenster nicht gefunden!" << std::endl;
    			return;
    		}
    
    		//Fenster in den Vordergrund bringen
    		SetForegroundWindow(wndH);
    		BringWindowToTop(wndH);
    
    		//Maus positionieren
    		SetCursorPos(xPos, yPos);
    
    
    		//Mausklick simulieren
    		INPUT Input = { 0 };
    
    		Input.type = INPUT_MOUSE;
    		Input.mi.dwFlags = MOUSEEVENTF_LEFTDOWN;
    		::SendInput(1, &Input, sizeof(INPUT));
    
    		::ZeroMemory(&Input, sizeof(INPUT));
    		Input.type = INPUT_MOUSE;
    		Input.mi.dwFlags = MOUSEEVENTF_LEFTUP;
    		::SendInput(1, &Input, sizeof(INPUT));
    	}
    	
    
    private:
    	std::string windowTitle;
    	std::string key;
    	unsigned int xPos;
    	unsigned int yPos;
    
    };
    
    
    
    std::vector<KeyHandler> readConfig() { //config lesen und parsen
    
    	std::ifstream input("config.cfg");
    	std::string jsonString((std::istreambuf_iterator<char>(input)),(std::istreambuf_iterator<char>()));
    	nlohmann::json config = nlohmann::json::parse(jsonString);
    	std::vector<KeyHandler> retVec;
    
    	for (unsigned int i = 0; i < config["shortcuts"].size(); i++) {
    		KeyHandler tmp(config["shortcuts"][i][0], config["shortcuts"][i][1], config["shortcuts"][i][2], config["shortcuts"][i][3]);
    		retVec.push_back(tmp);
    	}
    
    	return retVec;
    }
    
    
    
    int main() {
    
    	std::vector<KeyHandler> keyHandler = readConfig();
    	
    	while (true) {
    		for (const auto &keys : keyHandler) {
    			if (keys.keyPressed())
    				keys.doClick();
    		}
    
    		std::this_thread::sleep_for(std::chrono::milliseconds(100)); //delay damit nicht zuviele Klicks ausgeführt werden
    	}
    }
    

    Link zur JSON Lib: https://github.com/nlohmann/json
    Damit man Fenstertitel schneller herausfindet habe ich folgendes beigelegt:

    #include <iostream>
    #include <Windows.h>
    #include <thread>
    
    int main() {
    
    	std::this_thread::sleep_for(std::chrono::seconds(5));
    
    	char wnd_title[1024];
    	HWND hwnd = GetForegroundWindow();
    	GetWindowTextA(hwnd, wnd_title, sizeof(wnd_title));
    	std::cout << wnd_title << std::endl;
    	std::cin.get();
    }
    

    Die JSON-Config sieht z.B. so aus:

    {
      "shortcuts" : [
        
        [ "Fenstername_1", "X", 600, 500],
        [ "Fenstername_2", "Y", 800, 400]
    
      ]
    }
    

    Da ich den Code auch verstehen muss und meine Kenntnisse noch nicht so ausgeprägt sind, sollten keine Templates, Smart Pointer und Lambdas vorkommen^^.
    Ich freue mich schon auf eure Tipps 👍🏻



  • @Zhavok ich glaube nicht, daß du darauf viele Antworten bekommen wirst.
    Ups, das war eine Antwort 😕



  • @Zhavok sagte in Verbesserungsvorschläge für Code:

    #include <iostream>
    #include <string>
    #include <fstream>
    #include <Windows.h>
    #include <thread>
    #include <nlohmann/json.hpp>
    
    
    
    class KeyHandler {
    
    
    public:
    	KeyHandler(const std::string &windowTitle, const std::string &key, const unsigned int &xPos, const unsigned int &yPos)
    		: windowTitle(windowTitle), key(key), xPos(xPos), yPos(yPos) {
    
    	}
    
    	bool keyPressed() const {
    		if (GetAsyncKeyState(key[0]) && GetAsyncKeyState(VK_LCONTROL))
    			return true;
    		else
    			return false;
    	}
    
    	void doClick() const {
    
    		//Fenster suchen
    		HWND wndH = FindWindow(NULL, windowTitle.c_str());
    
    		if (wndH == 0) {
    			std::cout << "Fenster nicht gefunden!" << std::endl;
    			return;
    		}
    
    		//Fenster in den Vordergrund bringen
    		SetForegroundWindow(wndH);
    		BringWindowToTop(wndH);
    
    		//Maus positionieren
    		SetCursorPos(xPos, yPos);
    
    
    		//Mausklick simulieren
    		INPUT Input = { 0 };
    
    		Input.type = INPUT_MOUSE;
    		Input.mi.dwFlags = MOUSEEVENTF_LEFTDOWN;
    		::SendInput(1, &Input, sizeof(INPUT));
    
    		::ZeroMemory(&Input, sizeof(INPUT));
    		Input.type = INPUT_MOUSE;
    		Input.mi.dwFlags = MOUSEEVENTF_LEFTUP;
    		::SendInput(1, &Input, sizeof(INPUT));
    	}
    	
    
    private:
    	std::string windowTitle;
    	std::string key;
    	unsigned int xPos;
    	unsigned int yPos;
    
    };
    
    
    
    std::vector<KeyHandler> readConfig() { //config lesen und parsen
    
    	std::ifstream input("config.cfg");
    	std::string jsonString((std::istreambuf_iterator<char>(input)),(std::istreambuf_iterator<char>()));
    	nlohmann::json config = nlohmann::json::parse(jsonString);
    	std::vector<KeyHandler> retVec;
    
    	for (unsigned int i = 0; i < config["shortcuts"].size(); i++) {
    		KeyHandler tmp(config["shortcuts"][i][0], config["shortcuts"][i][1], config["shortcuts"][i][2], config["shortcuts"][i][3]);
    		retVec.push_back(tmp);
    	}
    
    	return retVec;
    }
    
    
    
    int main() {
    
    	std::vector<KeyHandler> keyHandler = readConfig();
    	
    	while (true) {
    		for (const auto &keys : keyHandler) {
    			if (keys.keyPressed())
    				keys.doClick();
    		}
    
    		std::this_thread::sleep_for(std::chrono::milliseconds(100)); //delay damit nicht zuviele Klicks ausgeführt werden
    	}
    }
    

    Die Referenzen auf unsigned int im Konstruktor sind unnötig. Solche Datentypen kannst du ruhig per Value übergeben:

    KeyHandler(const std::string &windowTitle, const std::string &key, const unsigned int &xPos, const unsigned int &yPos)
    

    Wenn sich diese Member nicht ändern, kannst du die "const" machen:

    std::string windowTitle;
    std::string key;
    unsigned int xPos;
    unsigned int yPos;
    

    Bei dem vector könntest du je nach Füllstand überlegen, ob du den mit "reserve" vorbereitest und dann das Anhängen nicht mit push_back sondern mit emplace_back machst.

    std::vector<KeyHandler> retVec;
    

    Der Konstruktoraufruf sieht blöd aus, was aber an dem JSON-Objekt ('config') liegt...

    KeyHandler tmp(config["shortcuts"][i][0], config["shortcuts"][i][1], config["shortcuts"][i][2], config["shortcuts"][i][3]);
    
    

    Eventuell lässt sich das noch etwas aufhübschen, falls du dir irgendwie eine const-Referenz auf das hier

    config["shortcuts"]
    

    beschaffen kannst.

    const auto &myref = config["shortcuts"][i];
    KeyHandler tmp( myref [0], myref [1], myref [2], myref [3]);
    


  • Reicht der Fenstertitel aus, um es eindeutig zu bestimmen? Fenstertitel können sich ändern, Instanzen können mehrfach vorkommen, eine Kombination aus Classname und Prozess-ID wäre hingegen eindeutig.



  • @It0101 omg sry. Ich habs direkt nach der Arbeit gepostet und dabei total vergessen das mit reserve etc. noch rein zu schreiben. Hab jetzt alle Vorschläge von dir umgesetzt

    #include <iostream>
    #include <string>
    #include <fstream>
    #include <Windows.h>
    #include <thread>
    #include <nlohmann/json.hpp>
    
    
    
    class KeyHandler {
    
    
    public:
    	KeyHandler(const std::string windowTitle, const std::string key, const unsigned int xPos, const unsigned int yPos)
    		: windowTitle(windowTitle), key(key), xPos(xPos), yPos(yPos) {
    
    	}
    
    	bool keyPressed() const {
    		if (GetAsyncKeyState(key[0]) && GetAsyncKeyState(VK_LCONTROL))
    			return true;
    		else
    			return false;
    	}
    
    	void doClick() const {
    
    		//Fenster suchen
    		HWND wndH = FindWindow(NULL, windowTitle.c_str());
    
    		if (wndH == 0) {
    			std::cout << "Fenster nicht gefunden!" << std::endl;
    			return;
    		}
    
    		//Fenster in den Vordergrund bringen
    		SetForegroundWindow(wndH);
    		BringWindowToTop(wndH);
    
    		//Maus positionieren
    		SetCursorPos(xPos, yPos);
    
    
    		//Mausklick simulieren
    		INPUT Input = { 0 };
    
    		Input.type = INPUT_MOUSE;
    		Input.mi.dwFlags = MOUSEEVENTF_LEFTDOWN;
    		::SendInput(1, &Input, sizeof(INPUT));
    
    		::ZeroMemory(&Input, sizeof(INPUT));
    		Input.type = INPUT_MOUSE;
    		Input.mi.dwFlags = MOUSEEVENTF_LEFTUP;
    		::SendInput(1, &Input, sizeof(INPUT));
    	}
    	
    
    private:
    	const std::string windowTitle;
    	const std::string key;
    	const unsigned int xPos;
    	const unsigned int yPos;
    
    };
    
    
    
    std::vector<KeyHandler> readConfig() { //config lesen und parsen
    
    	std::ifstream input("config.cfg");
    	std::string jsonString((std::istreambuf_iterator<char>(input)),(std::istreambuf_iterator<char>()));
    	nlohmann::json config = nlohmann::json::parse(jsonString);
    	std::vector<KeyHandler> retVec;
    	retVec.reserve(config["shortcuts"].size());
    
    	for (unsigned int i = 0; i < config["shortcuts"].size(); i++) {
    		const auto &myref = config["shortcuts"][i];
    		retVec.emplace_back(myref[0], myref[1], myref[2], myref[3]);
    	}
    
    	return retVec;
    }
    
    
    
    int main() {
    
    	std::vector<KeyHandler> keyHandler = readConfig();
    	
    	while (true) {
    		for (const auto &keys : keyHandler) {
    			if (keys.keyPressed())
    				keys.doClick();
    		}
    
    		std::this_thread::sleep_for(std::chrono::milliseconds(100)); //delay damit nicht zuviele Klicks ausgeführt werden
    	}
    }
    

    @yahendrik Das wäre theoretisch besser. Das Problem ist nur, dass die Prozess ID sich immer ändert wenn man die Zielanwendung neustartet. Das bedeutet man muss jedes mal die config neu anpassen. Das wollte ich absichtlich vermeiden. Ich weiß nicht genau was der jenige damit vor hat, aber ich denke es geht nur um ein paar Klicks auf Webseiten. Da dachte ich, ist es für ihn einfacher nur den Name vom Browser-Fenster einzutragen.



  • @Zhavok Ich hätte auch eher daran gedacht, das Fenster beim Programmstart auszuwählen (so ähnlich wie bei Spy++). Aber wenn es so ausreichend ist, ist es ja okay.



  • Man könnte noch meckern, dass Key als std::string geführt wird. Du greifst doch nur auf das erste Zeichen zu, warum dann kein char?



  • An irgendeiner Stelle muss ein cast kommen. Vom JSON bekomme ich nur einen String. Allerdings habe ich überlegt obs nen besseren weg gibt als einfach mit [0] darauf zuzugreifen. Das fühlt sich nicht gut an.



  • die Const-Referenzen der Strings im Konstruktor solltest du nicht wegmachen. Nur bei den "unsigned"-Parametern 😉

    Wenn du sauber arbeiten willst, prüfst du die Daten aus dem JSON-Objekt noch auf Gültigkeit. ( Key mindestens ein Zeichen, Windowtitle muss gesetzt sein, Zahlen müssen gültig sein... etc... Was auch immer für die Werte zulässig ist.

    Darüberhinaus kommen unter Windows in Multimonitorsystemen auch negative Koordinaten vor ( zumindest in QT ist das so ). Vielleicht wäre der Typ "unsigned" da doch nicht angebracht. Abgesehen davon könntest du für die Koordinaten ein schönes "std::pair" verwenden 😉



  • @It0101 sagte in Verbesserungsvorschläge für Code:

    Abgesehen davon könntest du für die Koordinaten ein schönes "std::pair" verwenden

    Brrr. Warum nicht eine benannte Klasse wie POINT, sodass man .x und .y statt .first und .second hat und außerdem das Ding eine klare Bedeutung hat?



  • @It0101 sagte in Verbesserungsvorschläge für Code:

    die Const-Referenzen der Strings im Konstruktor solltest du nicht wegmachen. Nur bei den "unsigned"-Parametern

    Das verstehe ich nicht ganz. Erkläre mir bitte den Unterschied. Wenn ich etwas per Referenz übergebe, dann übergebe ich doch eigentlich nur die Adresse dabei. dafür muss keine neue Kopie erstellt werden. Ich verstehe nicht ganz warum die Kopien bei Integern ok sind und bei der Klasse nicht. Generell sind Kopien doch nicht gut oder?

    @It0101 sagte in Verbesserungsvorschläge für Code:

    Wenn du sauber arbeiten willst, prüfst du die Daten aus dem JSON-Objekt noch auf Gültigkeit.

    wird gemacht.

    @It0101 sagte in Verbesserungsvorschläge für Code:

    Darüberhinaus kommen unter Windows in Multimonitorsystemen auch negative Koordinaten vor

    Oh, gut zu wissen. Hab das nur auf dem Laptop getestet^^.

    @wob sagte in Verbesserungsvorschläge für Code:

    Warum nicht eine benannte Klasse wie POINT

    Schau ich mir direkt mal an



  • @Zhavok sagte in Verbesserungsvorschläge für Code:

    Wenn ich etwas per Referenz übergebe, dann übergebe ich doch eigentlich nur die Adresse dabei. dafür muss keine neue Kopie erstellt werden. Ich verstehe nicht ganz warum die Kopien bei Integern ok sind und bei der Klasse nicht. Generell sind Kopien doch nicht gut oder?

    Wie groß ist denn so eine Adresse? Wie groß ist ein einfacher Integer?



  • @wob Der Integer je nach System meist 4 Byte. Bei Microcontrollern auch mal 2 Byte. Grundsätzlich >= short und <= long. Die Adresse ist wahrscheinlich wie ein Integer von der Größe her. So viele Bytes dürfte er nicht brauchen um die Adresse zu speichern. Bei Klassen ist das komplexer. Sie benötigen mehr Speicher. Richtig? Und wenn ja, dann kann ich verstehen warum es keinen Sinn macht und werde es ändern, aber direkt negative Auswirkungen dürfte es nicht haben oder?



  • also allgemein ist die adresse so groß, wie der adressbus breit ist, d.h. 1 byte bei 8-bit architekturen, 2 byte bei 16-bit architekturen, 4 byte bei 32-bit architekturen und 8 byte bei 64-bit architekturen.

    der grund, warum man klassen, strukturen usw. per referenz übergibt, ist einerseits, dass man sich damit bei großen objekten gerne mal den speicher zumüllt (zumindest bei java) und andererseits, dass die kopiererei echt ewig lange dauert.



  • @wob sagte in Verbesserungsvorschläge für Code:

    @It0101 sagte in Verbesserungsvorschläge für Code:

    Abgesehen davon könntest du für die Koordinaten ein schönes "std::pair" verwenden

    Brrr. Warum nicht eine benannte Klasse wie POINT, sodass man .x und .y statt .first und .second hat und außerdem das Ding eine klare Bedeutung hat?

    Das wäre die bessere Lösung, aber da der Standard meines Wissens keine anbietet, müsste man ja selber eine bauen 😃



  • @Zhavok sagte in Verbesserungsvorschläge für Code:

    @wob Der Integer je nach System meist 4 Byte. Bei Microcontrollern auch mal 2 Byte. Grundsätzlich >= short und <= long. Die Adresse ist wahrscheinlich wie ein Integer von der Größe her. So viele Bytes dürfte er nicht brauchen um die Adresse zu speichern. Bei Klassen ist das komplexer. Sie benötigen mehr Speicher. Richtig? Und wenn ja, dann kann ich verstehen warum es keinen Sinn macht und werde es ändern, aber direkt negative Auswirkungen dürfte es nicht haben oder?

    https://stackoverflow.com/questions/3009543/passing-integers-as-constant-references-versus-copying
    Depending on the underlying instruction set, an integer parameter can be passed as register or on the stack. Register is definitely faster than memory access, which would always be required in case of const refs (considering early cache-less architectures)



  • Alle Verallgemeinerungen sind falsch. Grundsätzlich.

    Da jede Referenz auch eine Indirektion bedingt (nicht laut Standart aber de facto) ist zwischen Kosten der Dereferenzierung (Anzahl?) und Kosten der Kopie (= bessere cache-locality) abzuwägen. Ich kann mir aber auch compiler vorstellen, die bei der Übergabe einer const & garnichts übergeben, sondern auf dem Original arbeiten.



  • Ok super. Das mit den Integer-Referenzen wusste ich noch nicht. Schön das zu wissen, da wäre ich nicht so schnell drauf gekommen. Ich merke, dass meine Fehler der Faulheit geschuldet sind. Einiges was ihr geschrieben habt, hatte ich schon im Kopf gehabt und mit einem "Ach naja, ist ja nicht ganz so wichtig" abgehackt. Das betrifft die Prüfung des JSONs und die Möglichkeit die Koordinaten unter einem Dach zu haben. Ich muss mich wohl zwingen auch die unschönen Vorgänge nicht zu vernachlässigen. Und da gehört die Überprüfung von Eingaben etc. bei mir dazu 🙂

    Ich hoffe jetzt alles beachtet zu haben was ihr geschrieben habt. Bei der Prüfung der Position ist mir nichts eingefallen, da jetzt negative Werte auch noch erlaubt sind und ich nicht weiß, bis wohin es sinnig ist die Größe zu begrenzen.

    #include <iostream>
    #include <string>
    #include <fstream>
    #include <Windows.h>
    #include <thread>
    #include <nlohmann/json.hpp>
    
    
    
    struct Point {
    	int xPos;
    	int yPos;
    };
    
    
    
    class KeyHandler {
    
    
    public:
    	KeyHandler(const std::string &windowTitle, const std::string &key, const Point position)
    		: windowTitle(windowTitle), key(key), position(position) {
    
    	}
    
    	bool keyPressed() const {
    		if (GetAsyncKeyState(key[0]) && GetAsyncKeyState(VK_LCONTROL))
    			return true;
    		else
    			return false;
    	}
    
    
    
    	void doClick() const {
    
    		//Fenster suchen
    		HWND wndH = FindWindow(NULL, windowTitle.c_str());
    
    		if (wndH == 0) {
    			std::cout << "Fenster nicht gefunden!" << std::endl;
    			return;
    		}
    
    		//Fenster in den Vordergrund bringen
    		SetForegroundWindow(wndH);
    		BringWindowToTop(wndH);
    
    		//Maus positionieren
    		SetCursorPos(position.xPos, position.yPos);
    
    
    		//Mausklick simulieren
    		INPUT Input = { 0 };
    
    		Input.type = INPUT_MOUSE;
    		Input.mi.dwFlags = MOUSEEVENTF_LEFTDOWN;
    		::SendInput(1, &Input, sizeof(INPUT));
    
    		::ZeroMemory(&Input, sizeof(INPUT));
    		Input.type = INPUT_MOUSE;
    		Input.mi.dwFlags = MOUSEEVENTF_LEFTUP;
    		::SendInput(1, &Input, sizeof(INPUT));
    	}
    	
    
    private:
    	const std::string windowTitle;
    	const std::string key;
    	const Point position;
    
    };
    
    
    
    std::vector<KeyHandler> readConfig() { //config lesen und parsen
    
    	std::ifstream input("config.cfg");
    	std::string jsonString((std::istreambuf_iterator<char>(input)),(std::istreambuf_iterator<char>()));
    	nlohmann::json config = nlohmann::json::parse(jsonString);
    	std::vector<KeyHandler> retVec;
    	retVec.reserve(config["shortcuts"].size());
    
    	for (unsigned int i = 0; i < config["shortcuts"].size(); i++) {
    		const auto &myref = config["shortcuts"][i];
    
    		if (myref[0].dump().length() == 2) { //dump() besteht leer aus einem "" string
    			std::cout << "Fehler: Keine Fenstertitel gesetzt!" << std::endl;
    			break;
    		}
    		if (myref[1].dump().length() == 2) {
    			std::cout << "Fehler: Keine Taste für Kombination nicht gesetzt!" << std::endl;
    			break;
    		}
    
    		Point position = { myref[2], myref[3] };
    		retVec.emplace_back(myref[0], myref[1], position);
    	}
    
    
    	return retVec;
    }
    
    
    
    int main() {
    
    	std::vector<KeyHandler> keyHandler = readConfig();
    	
    	while (true) {
    		for (const auto &keys : keyHandler) {
    			if (keys.keyPressed())
    				keys.doClick();
    		}
    
    		std::this_thread::sleep_for(std::chrono::milliseconds(100)); //delay damit nicht zuviele Klicks ausgeführt werden
    	}
    }
    


  • @Swordfish sagte in Verbesserungsvorschläge für Code:

    Alle Verallgemeinerungen sind falsch. Grundsätzlich.

    Da jede Referenz auch eine Indirektion bedingt (nicht laut Standart aber de facto) ist zwischen Kosten der Dereferenzierung (Anzahl?) und Kosten der Kopie (= bessere cache-locality) abzuwägen. Ich kann mir aber auch compiler vorstellen, die bei der Übergabe einer const & garnichts übergeben, sondern auf dem Original arbeiten.

    Konsens ist aber dass es im Grunde egal ist. Vorteil von Call-By-Value ist die mögliche Arbeit mit Literalen und weniger Schreibarbeit 😃



  • @It0101 sagte in Verbesserungsvorschläge für Code:

    da der Standard meines Wissens keine [POINT-Klasse] anbietet, müsste man ja selber eine bauen

    Es wird hier Windows.h includet... Warum sollt man selber eine bauen? Hier wird SetCurPos benutzt. Das Gegenstück dazu, GetCursorPos, liefert sogar einen (Pointer auf) POINT zurück.


Anmelden zum Antworten