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

Im Browser öffnen: intelligente Ersetzungslogik für Ticker, ISIN und WKN #740

Closed
wants to merge 6 commits into from

Conversation

sunnylars
Copy link

Im Browser öffnen: intelligente Ersetzungslogik für Ticker, ISIN und WKN

Die Reihenfolge der {} in dem Bookmark bestimmt die Priorität. Die erste Ersetzung, die in der Security einen Wert hat, wird genommen, alle weiteren verworfen.

Falls keine Ersetzung statt gefunden hat, wird der erste Wert in der Reihenfolge Ticker,Isin,wnk und Name, der in Security befüllt ist, hinten an die Url gehängt.

p.s.: ich bin 1. git und 2. github neuling. falls irgendwelche fehler hier sind, bitte einfach bescheid geben.

@sunnylars sunnylars changed the title Issue 698 Im Browser öffnen: intelligente Ersetzungslogik für Ticker, ISIN und WKN May 4, 2017
@buchen
Copy link
Member

buchen commented May 13, 2017

Hi @sunnylars,

Vielen Dank für den Change!

Ich bin allerdings nicht ganz glücklich, dass sich das Verhalten ändert. Bisher konnte man einfach eine URL konstruieren, in der sowohl der Name als auch die ISIN enthalten war. Ich weiß nicht wie oft / häufig das verwendet wird, aber ich sehe auch keine Grund warum man das jetzt ändern sollte.

Könnte man nicht sowas machen?
{isin} erst die ISIN, oder leer, falls es keine ISIN gibt
{isin,wkn} nimmt die ISIN, oder falls es keine ISIN gibt die WKN, oder eben leer

Mit dem "Komma-Syntax" ist dann auch die "eines dieser Werte" Semantik klar.




types.put("{tickerSymbol}", "getTickerSymbol");
Copy link
Member

@buchen buchen May 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warum keine Lambda Function? Per Reflection geht auch, aber man bekommt keine Compilefehler mehr, kann nicht in der IDE umbenennen.

Sowas wie

        Map<String, Function<Security, String>> types = new HashMap<>();
        types.put("isin", Security::getIsin);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich muss gestehen, gehört habe ich schon von Lambda Functions, aber beruflich arbeite ich mit Java 7.
Aber ich lerne gerne, ich schaue mir das an und baue es um.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vielleicht ist es auch einfacher die Werte direkt in die Map zu setzen - statt den Umweg über die Methode. Es ist ja nicht so als wären das teuere Methodenaufrufe die man nur machen möchte wenn tatsächlich eine Ersetzung stattfindet. Die Werte sind ja da. 😄

}
catch (NoSuchMethodException | SecurityException | IllegalAccessException | IllegalArgumentException | InvocationTargetException e)
{
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Die Exceptions würde ich in RuntimeExcepition verpacke und weiterwerfen. Ich weiß in dem "name.abuchen.portfolio" Bundle kann man nicht einfach Fehler loggen. Aber die StackTrace mit printStackTrace landen quasi im Nirwana (der err output stream wird in eine Datei in den Configuration Folder umgeleitet). Dann lieber werfen und das UI Modul loggt einen Fehler.

Und: vermutlich fallen die alle weg wenn man die Reflection ausbaut.

}

if(!replacementDone){
url = pattern.replace("{tickerSymbol}", encode("")); //$NON-NLS-1$
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ist das nicht doppelt? Wenn es pattern gibt, dann dürfte das doch hier nicht mehr ersetzt werden können?

url = url.replace("{isin}", encode("")); //$NON-NLS-1$
url = url.replace("{wkn}", encode("")); //$NON-NLS-1$
url = url.replace("{name}", encode("")); //$NON-NLS-1$
if(security.getTickerSymbol() != null && security.getTickerSymbol().length()>0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich würde die "magic" weglassen. Wenn man kein Pattern angibt, dann ist eben auch Wert in der URL. Einfach Werte hinten an die URL hängen finde ich zu "magic".

buchen pushed a commit that referenced this pull request May 21, 2017
Issue: #740
Signed-off-by: Lars Hercher <it@larshercher.de>
[cherry-picked and squashed commits]
Signed-off-by: Andreas Buchen <andreas.buchen@gmail.com>
@buchen
Copy link
Member

buchen commented May 21, 2017

Hi @sunnylars,

die Änderung ist drin - siehe e235583 👍

Ich habe noch einen kleinen Change hinterher geschoben:

  • ein Kontextmenü mit den Ersetzungen --> eine Art indirekte Dokumentation welche Ersetzungen möglich sind
  • Neue "tickerSymbolPrefix": Dazu habe ich schon mal eine Anfrage bekommen weil es eine Webseite gibt, die nicht mit ganzen symbol umgehen kann
  • Noch ein paar neue Tests --> darum musst ich die Logik noch mal leicht anpassen. Bei einer URL wie "isin={isin}" wurden beide Teile ersetzt weil zunächst die Klammern entfernt wurden. Also habe ich die Ersetzung direkt nach dem Matcher gesetzt.

@buchen buchen closed this May 21, 2017
@buchen
Copy link
Member

buchen commented May 21, 2017

Und weil wir gerade dran waren: Ich habe noch die Möglichkeit eingebaut, dass man auch beliebige Attribute der Wertpapiere für die Ersetzung nutzen kann. Das ging relative einfach weil Du die generische Ersetzungslogik gebaut hast.

https://github.com/buchen/portfolio/blob/master/name.abuchen.portfolio/src/name/abuchen/portfolio/model/Bookmark.java#L58-L64

@sunnylars
Copy link
Author

Ok, danke Dir.

Sieht gut aus. Besonders das mit dem Kontextmenu gefällt mir.

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

Successfully merging this pull request may close these issues.

None yet

2 participants