Archive for the ‘czytelny kod’ Category

Introduction (OOP principles)

Today I’d like to present you a problem that most especially rookie developers find compelling. This is strongly connected with OOP basics, specifically with 2 fundamental truths:

  1. Use inheritance whenever possible.
  2. Prefer composition over inheritance.
So at first I had problems understanding how these 2 rules are related but as the time passed I learned to distinguish the two and recently in one of my projects I came across piece of code that is perfect example on why object composision/aggregation might be a better solution over inheritance.

Old way – using inheritance

Background – code usage

To start with some background – the code referred to is part of application that displays daily trend graph of some piece of data. It’s responsible for drawing coordinate system, x-axis, y-axis, some labels, legend lines, text-labels and of course data-curves. The code is executed on client-side and one of requirements is to support ie8 – which is why there was a need to support both SVG and VML drawing formats.

Classes responsibilites

The old apporach used in inheritance to support both formats and below you can find class diagram for the described classes. To start from the top:

  • GrapDrawer is a drawing interface – it’s main goal is to provide high-level methods for drawing different parts of graph used by othere pieces of application
  • AbstractGraphDrawer which is an interesting beast – because it provides a bridge between GraphDrawer interface and client-specific implementations – that is VML/SVG achieved set of abstract protected methods that encapsulate client-specific drawing API
  • SVGDrawer/VMLDrawer – these are both client-specific drawing apis.

Problem description

You might be asking yourselves why is there any problem with this approach. It has all the good parts:

  • there’s an interface for accessing public methods,
  • there’s an abstract method that acts as a bridge between drawer and
  • client-specific implementation.

Everything is encapsulated so user has no knowledge of what’s going on behing the secenes – so what’s the big deal.

This is all true but due to AbstractGraphDrawer dual nature it’s code is really hard to maintain. Both high and low-level API collide with each other – the former changes frequently where the latter very rarely. If you look closer you can see that all protected methods are there only to be overridden by implementation and they’re completely hidden from user. While all parts of public interface are not used in any way by the implementation.

New way – use object aggregation

Solution description

These two worlds interact with each other only through their interfaces. So it’s a perfect candidate for split – low-level methods should be hidden behind an interface that must be implemented by client-specific code, whereas high-level methods, used by some other code go to a public class which has reference to client-specific low-level drawing api.

This is all presented in the diagram below, where you can find:

  • GrapDrawer class which is used for high-level operations. Low level operations are executed by graphDrawerWorker
  • GraphDrawerWorker interface which defines set of required low-level operations
  • SVGDrawer/VMLDrawer classes which only implement GraphDrawerWorker and thus loose all coupling with high-level code

Summary

There’s still at least one question that needs some attention: how did this code creep into the application? The answer is fairly simple – it was built incrementally as business needs rose and suddenly AbstractGraphDrawer became a beast with two heads. Unfortunately for everyone who’s been working with it for longer time it was hidden. Only when someone with fresh look apporached a project, not only could this be spotted but also addressed.

All of you that can spot more basic OOP principles broken in original code (or a new one :P) please leave them in comments 🙂

Ostatnio podczas pracy nad jednym projektem w wielu miejscach w kodzie należało przepisać przychodzące żadanie z jednego transfer object’a na drugiego (powody tegoż przepisywania są dla tego artykułu zupełnie nieistotne) w związku z czym w wielu miejscach w kodzie pojawiły się elementy do siebie łudząco podobne:

target.setSomeProperty(source.getSomeProperty())

Co oczywiście nasunęło mi myśl czy nie dałoby się tego jakoś wydzielić, ale ograniczała mnie myśl, że java to przecież nie taki dajmy na to JavaScript w którym takie figle to chleb powszedni.

Rozwiązanie podsunął mi dopiero Tomek swoim artykułem nt. BeanUtils – wystarczy przecież pobrać własności z jednego beana za pomocą metody describe i następnie korzystając z metody populate je zaaplikować – no i niby wszystko cacy, ale jest parę niuansów które mi przeszkadzają:

  • całe api jest statyczne…
  • dodawanie Converter‚ów jakioś tak topornie wygląda
  • nie ma możliwości definiowania własności, która zmieniła nazwę pomiędzy Beana’mi
  • i jak śię jakaś własność nie przepisze, to nie dowiemy się o tym dopóki nie będziemy jej próbowali pobrać, czyli znaaacznie za późno

Stąd pomysł na klasę BeanTransformer, która powinna umożliwiać:

  • definiowanie własności ze zmienioną nazwą
  • definiowanie własności do pominięcia
  • definiowanie Coverter’a z wykorzystaniem generycznego interfejsu, który sam będzie przenosił informację o typie dla jakiego ma być aplikowany
  • sprzątanie konwerterów po zakończonej konwersji

I tak właśnie narodziła się klasa BeanConverter, którą omówię poprzez opis jej metod i klas wewnętrznych upraszczających całą zabawę 🙂

No to zacznijmy od początku – zadeklarujmy klasę:


public class BeanTransformer {

}

Przydałaby się metoda pozwalająca definiować własności, które nie bedą nam potrzebne i przechowująca je w Set‚cie:

	private Set<String> skipProperites = new HashSet<String>();
	public BeanTransformer skip(String firstProperty, String... propertyNames) {
		skipProperites.add(firstProperty);
		if (propertyNames != null) {
			skipProperites.addAll(Arrays.asList(propertyNames));
		}
		return this;
	}

Jak widać przy okazji umożliwia chain’y i dodawanie więcej niż jednej własności za jednym zamachem.

Następnie potrzebujemy metodę pozwalającą zdefiniować własności, których nazwa się zmieniła:

	private Map<String, String> renameProperties = new HashMap<String, String>();
	public BeanTransformer rename(String fromProperty, String toProperty) {
		renameProperties.put(fromProperty, toProperty);
		return this;
	}

I teraz wystarczy przejechać się po własnościach, sprawdzić które na które przechodza, usunąć zbędne i sprawdzić czy coś zostało i ew. zgłosić błąd.

	private Set<String> filterCorrectProperties(final Map<String, Object> properties) {
		// first remove properties that should be skipped
		properties.keySet().removeAll(skipProperites);

		// than go over the rest of them
		Set<String> foundProperties = Sets.filter(renameProperties.keySet(),
				new Predicate<String>() {
					public boolean apply(String fromProperty) {
						if (properties.containsKey(fromProperty)) {
							Object v = properties.remove(fromProperty);
							String newKey = renameProperties.get(fromProperty);
							properties.put(newKey, v);
							return false;
						}
						return true;
					}
				});

		return foundProperties;
	}

Teraz gdy już mamy listę przefiltrowanych własności możemy je przepisać do docelowego beana

	@SuppressWarnings("unchecked")
	public <F, T> T transform(F fromBean, T toBean) throws NotUsedPropertiesException {
		try {
			Map<String, Object> properties = (Map<String, Object>) PropertyUtils.describe(fromBean);

			Set<String> foundProperties = filterCorrectProperties(properties);

			if (!foundProperties.isEmpty()) {
				throw new NotUsedPropertiesException(foundProperties);
			}

			BeanUtils.populate(toBean, properties);

			return toBean;
		} catch (IllegalAccessException e) {
			throw new NotUsedPropertiesException(e);
		} catch (InvocationTargetException e) {
			throw new NotUsedPropertiesException(e);
		} catch (NoSuchMethodException e) {
			throw new NotUsedPropertiesException(e);
		}
	}

Zakładamy, że reszta własności przepisuje się 1-1.

Jeszcze miłą opcją byłaby możliwość dodania własnego converter’a dla typów innych niż standardowe – tutaj skorzystam również z klasy szablonowej, która będzie rozszerzać interfejs Converter, ale dodatkowo definicja zawiera typ klasy który dany konwerter obsługuje, co daje nam 2 zalety:

  • Klasa niesie cały zasób informacji nt. konwersji
  • Część wspólnej logiki może być wydzielona na zewnątrz

Zacznijmy od samego interfejsu, który będzie prosty do bólu:

	public static interface Converter<T> extends org.apache.commons.beanutils.Converter {}

Teraz metoda rejestrująca takiego konwerterka nie musi mieć explicite podanej klasy, pobierze ją sobie z szablonu korzystając z metody getGenericInterfaces:

	public <T> BeanTransformer addConverter(Converter<T> c, boolean skipConverterImpl) {
		Type[] types = c.getClass().getGenericInterfaces();
		Class<T> clz = (Class<T>) ((ParameterizedType) types[0]).getActualTypeArguments()[0];
		if (!skipConverterImpl) {
			c = new ConverterImpl(clz, c);
		}
		converters.put(clz, c);
		return this;
	}

Jeszcze tylko pozostaje wyjaśnić znaczenie tajemniczej klasy ConverterImp – ona to mianowicie jest sposobem na wydzielenie części wspólnej funkcjonalności dla wszystkich converter’ów a jednocześnie nie wymusza na użytkowniku jej znajomości, wiec po prostu dekoruje metodę convert i dopiero dopuszcza do głosu konkretna implementację, jeżeli wspólna implementacja nie wie jak dany obiekt utworzyć, no ale dośc gadania:

	private static class ConverterImpl<T> implements Converter<T> {
		private Class<T> self;
		private Converter<T> target;
		public ConverterImpl(Class<T> self, Converter<T> target) {
			this.self = self;
			this.target = target;
		}
		@SuppressWarnings("unchecked")
		public Object convert(Class arg0, Object arg1) {
/*
			corrected after comment by bob
			if (self.isAssignableFrom(arg1.getClass())) {
*/
			if (self.isInstance(arg1)) {
				return arg1;
			}
			return target.convert(arg0, arg1);
		}
	}

Mając convertery możemy je sobie włączać i wyłączac przed i po konwersji:

	private void registerConverters() {
		for (Class<?> clz : converters.keySet()) {
			ConvertUtils.register(converters.get(clz), clz);
		}
	}

	private void deregisterConverters() {
		for (Class<?> clz : converters.keySet()) {
			ConvertUtils.deregister(clz);
		}
	}

// w metodzie transform dodajemy wywołania:
			registerConverters();
			BeanUtils.populate(toBean, properties);
			deregisterConverters();

Dla tych, których zaciekawiłem tym wpisem wrzuciłem projekt (wraz z unitami!) na githuba.

W pewnym projekcie, miałem mały problem związany z przejrzystością kodu – mianowicie w kodzie przewijały się różnego rodzaju strategie przechodzenia po elementach:

  • kolekcje (java.util.Collection) – for-each
  • enumeracja (java.util.Enumeration) – for z licznikiem i metoda hasMore elements
  • listy node’ów (org.w3c.dom.NodeList) – for z licznikiem i metoda getLength wyznaczająca koniec

Dodatkowym problemem zaciemniającym kod w przypadku ostatniego elementu jest fakt, że metoda NodeList.item zwraca ZAWSZE node’a, nawet jeżeli użytkownik WIE, że wszystkie node’y będą typu Element i tak musi je sobie zrzutować.

Rozwiązaniem problemu było dodanie metody szablonowej, która na wejsciu przyjmie obiekt, po ktorym będzie sobie chodzić iterator i będzie go zwracać, no to do dzieła:

package pl.bedkowski.util;

import java.util.Enumeration;
import java.util.Iterator;

import org.w3c.dom.Node;
import org.w3c.dom.NodeList;

public class IterGenerator {

	public static <T> Iterable<T> iter(final Enumeration<T> e) {
		return new Iterable<T>() {

			public Iterator<T> iterator() {
				return new Iterator<T>() {

					public boolean hasNext() {
						return e.hasMoreElements();
					}

					public T next() {
						return e.nextElement();
					}

					public void remove() {
						throw new UnsupportedOperationException();
					}

				};
			}
		};

	}
}

Tutaj jak widać sprawa była prosta – interfejs Enumeration jest szablonem, więc zamiana jednego na drugi odbyła się w miarę bezboleśnie. Z NodeListą już nie pójdzie tak łatwo, no ale do dzieła 🙂

	public static <T extends Node> Iterable<T> iter(final NodeList e, Class<T> clazz) {
		final int size = e.getLength();
		return new Iterable<T>() {

			public Iterator<T> iterator() {
				return new Iterator<T>() {

					private int current = 0;

					public boolean hasNext() {
						return current < size;
					}

					@SuppressWarnings("unchecked")
					public T next() {
						return (T) e.item(current++);
					}

					public void remove() {
						throw new UnsupportedOperationException();
					}
				};
			}
		};
	}

Jak widać w tym przypadku już nie jest tak pięknie, potrzeba dodać adnotację, żeby ukryć warninga, ale przynajmniej użytkownik nie musi sobie tym zaprzątać głowy. Dodajmy jeszcze małe ułatwienie:

	public static Iterable<Node> iter(final NodeList e) {
		return iter(e, Node.class);
	}

I teraz tylko jakiś przykładzik:


import static pl.bedkowski.util.IterGenerator.iter;

import org.w3c.dom.Element;
import org.w3c.dom.Node;
import org.w3c.dom.NodeList;

public class Example {
	public static void main(String[] args) {
		NodeList list = null; // wiem, wiem NPE, ale nie mam zadnej list pod reka
		for(int i=0;i<list.getLength();i++) {
			Node n = list.item(i);
			System.out.println(n.getLocalName());
		}

		// teraz wykorzsytamny nasz iter
		for(Node n : iter(list)) {
			System.out.println(n.getLocalName());
		}

		//teraz gdy wiemy, ze lista zawiera same obiekty typu Element
		for(int i=0;i<list.getLength();i++) {
			Element e = (Element) list.item(i);
			System.out.println(e.getTagName());
		}

		// z wykorzytaniem itera
		// teraz wykorzsytamny nasz iter
		for(Element n : iter(list, Element.class)) {
			System.out.println(n.getTagName());
		}
	}

}

Jak widać pracy wcale nie trzeba włożyć dużo a korzyść spora 🙂

Ten wpis będzie dotyczył zwiekszania czytelności kodu podczas dynamicznego nadpisywana metody w JavaScript’cie.

Problem jest banalny – mamy sobie jakas klase, która oferuje 2 metody, getName/setName i chcielibyśmy w zupełnie innym miejscu dla jednej instancji zamienić implementację setName (poprzednia nas nie interesuje).


function MyClass(){}

MyClass.prototype.getName(){}

MyClass.prototype.setName(){}

Jak widać na powyższym kawałku kodu, przykład jest banalny, teraz utwórzmy sobie instancję MyClass:


// pierwsza instancja
var m1 = new MyClass();

// druga z nadpisana metoda
var m2 = new MyClass();

m2.setName = function newSetName() {}

Na pozór wszystko wydaje się cacy – tworzymy sobie 2 instancje, z których jedna ma troche zmienione zachowanie, wszystko przecież jest czytelne. Owszem, ale co sie stanie jeżeli tworzenie instancji m1 i m2 będzie w znacznie oddalone od miejsca ich uzycia? Wtedy deklaracja MyClass nie da nam żadnej wskazówki, że jej zachowanie może sie zmienić. Jak temu zaradzić – można przesłać metode w konstruktorze, oto pomysł:


//dodajmy parametr i jakies jsdocki

/**
 *@param Function [setName=undefined]
 */
function MyClass(setName) {
   if (setName) {
      this.setName = setName;
   }
}

// reszta bez zmian

Jak widać mamy domyślną implementacje w prototypie a dodatkowo podczas tworzenia klasy, przesyłamy jej funkcję zastepująca setName, wtedy utowrzenie m1 i m2 wyglądałoby tak:


var m1 = new MyClass();

var m2 = new MyClass(function newSetName(){});

Jak widać dodanie parametru do konstruktora, któremu towarzyszy dopisanie krótkiego jsDoc’a zajmuje 5 minut a w zupełności wystarczya za dokumentację.

Dodalem nowa kategorie nazwana „czytelny kod”, bede w niej staral sie przedstawic przyklady zaczerpniete z projektow przy ktorych pracuje, wskazujace w jaki sposob czytac pisać kod z nastawieniem na jego czytelnosc. Najprostszy sposob to konwencje nazewnicze, ale postaram sie poglebic temat uzywajac przykladow roznych podejsc do tego samego problemu ze wskazaniem na to bardziej czytelne.