Skip to content

Code is quite hard to read. #51

@H3wastooshort

Description

@H3wastooshort

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions