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

Code is quite hard to read. #51

Open
H3wastooshort opened this issue May 29, 2024 · 0 comments
Open

Code is quite hard to read. #51

H3wastooshort opened this issue May 29, 2024 · 0 comments

Comments

@H3wastooshort
Copy link

H3wastooshort commented May 29, 2024

I am currently trying to add functionality to parts of you library. Doing that, i constantly run into endles if-else chains big enough for the code to go off the right side of my screen...
These almost always are just checking if something succeeded, and if not exiting the function with an error. This can be written more cleanly like this: if (notFoundOK()) return "ERROR";. If you are just planning to return an error, this has the same effect. As soon as a return is executed, the function exits and code following it will not be executed.

Here is an example:

GSMSimHTTP.cpp get(String) - original

String GSMSimHTTP::get(String url) {

	if (isConnected()) {
		// Terminate http connection, if it opened before! Otherwise method not run correctly.
		gsm.print(F("AT+HTTPTERM\r"));
		_readSerial();

		gsm.print(F("AT+HTTPINIT\r"));
		_readSerial();
		if (_buffer.indexOf(F("OK")) != -1) {
			gsm.print(F("AT+HTTPPARA=\"CID\",1\r"));
			_readSerial();
			if (_buffer.indexOf(F("OK")) != -1) {
				gsm.print(F("AT+HTTPPARA=\"URL\",\""));
				gsm.print(url);
				gsm.print("\"\r");
				_readSerial();

				if (_buffer.indexOf(F("OK")) != -1) {
					gsm.print(F("AT+HTTPACTION=0\r"));
					_readSerial();
					if (_buffer.indexOf(F("OK")) != -1) {
						_readSerial(10000);
						if (_buffer.indexOf(F("+HTTPACTION: 0,")) != -1) {
							String kod = _buffer.substring(_buffer.indexOf(F(","))+1, _buffer.lastIndexOf(F(",")));
							String uzunluk = _buffer.substring(_buffer.lastIndexOf(F(","))+1);

							String sonuc = "METHOD:GET|HTTPCODE:";
							sonuc += kod;
							sonuc += "|LENGTH:";
							sonuc += uzunluk;

							// Bağlantıyı kapat!
							gsm.print(F("AT+HTTPTERM\r"));
							_readSerial();

							sonuc.trim();

							return sonuc;
						}
						else {
							return "HTTP_ACTION_READ_ERROR";
						}
					}
					else {
						return "HTTP_ACTION_ERROR";
					}
				}
				else {
					return "HTTP_PARAMETER_ERROR";
				}

			}
			else {
				return "HTTP_PARAMETER_ERROR";
			}
		}
		else {
			return "HTTP_INIT_ERROR";
		}
	}
	else {
		return "GPRS_NOT_CONNECTED";
	}
}

GSMSimHTTP.cpp get(String) - modified

String GSMSimHTTP::get(String url) {
	if (!isConnected()) return "GPRS_NOT_CONNECTED";
	// Terminate http connection, if it opened before! Otherwise method not run correctly.
	gsm.print(F("AT+HTTPTERM\r"));
	_readSerial();

	gsm.print(F("AT+HTTPINIT\r"));
	_readSerial();

	if (_buffer.indexOf(F("OK")) != -1) return "HTTP_INIT_ERROR";
	gsm.print(F("AT+HTTPPARA=\"CID\",1\r"));
	_readSerial();
	if (_buffer.indexOf(F("OK")) == -1) return "HTTP_PARAMETER_ERROR";

	gsm.print(F("AT+HTTPPARA=\"URL\",\""));
	gsm.print(url);
	gsm.print("\"\r");
	_readSerial();
	if (_buffer.indexOf(F("OK")) == -1) return "HTTP_PARAMETER_ERROR";
	
	gsm.print(F("AT+HTTPACTION=0\r"));
	_readSerial();
	if (_buffer.indexOf(F("OK")) == -1) return "HTTP_ACTION_ERROR";
	
	_readSerial(30000);
	if (_buffer.indexOf(F("+HTTPACTION: 0,")) == -1) return "HTTP_ACTION_READ_ERROR";
	
	String kod = _buffer.substring(_buffer.indexOf(F(","))+1, _buffer.lastIndexOf(F(",")));
	String uzunluk = _buffer.substring(_buffer.lastIndexOf(F(","))+1);
	kod.trim();
	uzunluk.trim();
	
	String sonuc = "METHOD:GET|HTTPCODE:";
	sonuc += kod;
	sonuc += "|LENGTH:";
	sonuc += uzunluk;

	// Bağlantıyı kapat!
	gsm.print(F("AT+HTTPTERM\r"));
	_readSerial();

	sonuc.trim();

	return sonuc;
}

At least in my opinion, the modified version is much easier to understand. The error message follows right after the condition that could cause it.

You could also replace all checks for OK in the buffer with one global checkOKinBuffer() defined in GSMSim.h

All that does not seem too hard to automate. A python script could probably rearrange the code automatically.

Another thing: having multiple functions with nearly-identical code and (even worse) identical strings affects the code size quite significantly. see #52

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