EarnForex / MarketProfile

Market Profile indicator for MT4, MT5, and cTrader by EarnForex.com
https://www.earnforex.com/metatrader-indicators/MarketProfile/
Apache License 2.0
155 stars 68 forks source link

Style fix #8

Closed BRMateus2 closed 3 years ago

BRMateus2 commented 3 years ago

This is just a styling fix using Style "Linux", spaces "4", convert tab to spaces, delete empty lines, insert space after commas and semicolons, insert space around statement operators.

I would like to improve the indicator performance in the future, but I can't guarantee - the styler is working fine for Metatrader 5, indicator ok - I don't have Metatrader 4, please compile it for MT4 to test if the styler didn't break it.

BRMateus2 commented 3 years ago

Also this changes the Copyright from 2020 to 2021.

EarnForex commented 3 years ago

Thanks for the help! The space thingy is great. However, removing the empty lines and the styler's placement of curly brackets is a complete deal-breaker for me.

BRMateus2 commented 3 years ago

Thanks for the help! The space thingy is great. However, removing the empty lines and the styler's placement of curly brackets is a complete deal-breaker for me.

I see, I will look into this and maybe try a new approach - I got used to developing with that style for a long time, which is a mixture of Google C++ guidelines and a less restrictive version of the Linux Kernel guidelines; for my eyes, the compact coding makes easier to read and less scrolling, but indeed results in convoluted code. Thank you for the indicator :).

BRMateus2 commented 3 years ago

I went line by line, and found a single functional difference (always .mq4 /// .mq5):

EventSetMillisecondTimer(500); /// EventSetMillisecondTimer(1000);

I updated my fork with a new styler called "Allman", and fixed one of the issues you described - yeah, I didn't see that, when I was testing the styles, the Metaquotes style had broken the comments.

Looks fine now :), the only problem is that I was not able to preserve the empty lines.

EarnForex commented 3 years ago

It is much better now, but I also prefer spaces between if/switch/while and parentheses.

As for the timer difference - MT5 is much slower, so I've decided to increase the period there.

BRMateus2 commented 3 years ago

I've added a space between if/while/for/switch and (), is there any more improvement which might work fine?

This makes it able to be styled without fear of breaking the guidelines.

We can also use tabs in place of space-indentation method, if filesize is of matter - reduces 123kB to 107kB - but MetaEditor does not support tabs natively (they have a bug with the styler, changing everything to 4 spaces/user defined).

BRMateus2 commented 3 years ago

Might wanna change returns to not be like-functions - return(x); changed to return x; and return(x + y); changed to return (x + y); or return x + y;

What is the better choice, MQL5 styling or any of the others? Ref https://stackoverflow.com/questions/4216723/function-return-writing-style-in-java/4216741#4216741

EarnForex commented 3 years ago

The styling is all good now and I agree with your idea on the return statement. But why did you change this comment: // Different from _SessionsToCount when working with Intraday sessions and for RaysUntilIntersection != Stop_No_Rays. to this? // Different from _SessionsToCount when working with Intraday sessions and for RaysUntilIntersection == true. RaysUntilIntersection is an enum, not a bool.

Also, I saw you added some more comments - that's great! It would be even better to add a period at the end of each.

Thanks a lot for your help!

BRMateus2 commented 3 years ago

The styling is all good now and I agree with your idea on the return statement. But why did you change this comment: // Different from _SessionsToCount when working with Intraday sessions and for RaysUntilIntersection != Stop_No_Rays. to this? // Different from _SessionsToCount when working with Intraday sessions and for RaysUntilIntersection == true. RaysUntilIntersection is an enum, not a bool.

Indeed that specific comment was updated at different times, where the types from the .mq4 and the .mq5 at older file versions were different - on .mq4 it was updated to the newest type enum, but on .mq5 it maintained the old comment bool, that RaysUntilIntersection was a bool before? I wrongly mixed those comments with both files when changing.

I see some differences between .mq4 and .mq5; 1) At .mq4 line 1199 ObjectSetInteger(0, rectangle_prefix + "Median" + Suffix + LastName, OBJPROP_BACK, false); At .mq5 line 1206 is missing, should we add that line to set OBJPROP_BACK, explicitly false (to explicitly not set object as background in Chart, behind Bars)? I've added it to .mq5 to retain guaranteed-functionality;

2) At .mq4 line 1680 if (RaysUntilIntersection == Stop_No_Rays) continue; At .mq5 line 1749, is wrongly comparing enum Rays to supposedly older bool Rays if (!RaysUntilIntersection) continue; I believe the correct code is the .mq4 variant, I changed .mq5 file to be the same line as .mq4;

3) There is also a large piece of code difference at .mq4 and .mq5 at CRectangleMP::Process(), the .mq4 line has the Rectangle Session boundaries (7 lines) not at the beggining, but starts forward at 2072, what shall we do in this special case? Are those to be merged in which order? The .mq5 order prevails? Those are also related with missing lines in .mq4 file at the function above that, CRectangleMP::CRectangleMP, where that function sets t1 and t2 to zero in .mq5 but that code is non-existant in .mq4. // Calculate rectangle session's actual time and price boundaries.

4) At .mq4 line 1873 and .mq5 line 1940 OnTimer() calls, 11 lines are missing at .mq4 file related to a check for valid dates, what shall we do in this special case? double High[], Low[]; datetime Time[]; ArraySetAsSeries(High, true); ArraySetAsSeries(Low, true); ArraySetAsSeries(Time, true); int rates_total = iBars(Symbol(), Period()); int h = CopyHigh(Symbol(), Period(), 0, rates_total, High); int l = CopyLow(Symbol(), Period(), 0, rates_total, Low); int t = CopyTime(Symbol(), Period(), 0, rates_total, Time); // Data not yet ready. if ((h <= 0) || (l <= 0) || (t <= 0)) return;

5) At .mq4 line 361 and .mq5 line 364, OnCalculate() returns 0 on .mq4 and rates_total on .mq5, I changed the .mq4 file to return rates_total instead. // If we calculate profiles for the past sessions, no need to run it again. if ((FirstRunDone) && (StartDate != Time[0])) return rates_total;

All return statements are changed to be Java/C++ compliant, with parenthesis on math - not function-like syntax.

Line numbers are wrong, actually - because when you edit something, the line numbering changes..

EarnForex commented 3 years ago

1) OBJPROP_BACK works differently in MT5. In MT4, it fills the rectangle, while in MT5, it just pushes the object to the background (see this for more details).

2) Yes, that's worth changing in MT5. However, I am not sure why it is line 1749 (or 1751) in your file. It is 1854 in the original .mq5 file. Please make sure you are updating the latest version of Market Profile (1.16).

3) I guess it is best to leave that as it is to avoid breaking something.

4) That is MT5-specific code. It isn't needed in MT4.

5) Yes, that's a good one. Should be rates_total in MT4 too.

BRMateus2 commented 3 years ago

1) That's a pretty interesting difference, lol - OBJPROP_BACK was the "equivalent" as OBJPROP_FILL back then - I did read the documentation but they specify the same comment, maybe it was changed in newer builds and we don't need those lines at all nowadays.

2) About the line difference, is that there were 156 \r\n\r\n lines in the original .mq5, now it is only 58 - being \r\n the CRLF Carriage Return\Newline combo for Windows. That means there were 156 empty lines with no comments, and now there are empty lines only between functions - that made the line count go lower - with some comments that I understood in-between.

3) I agree.

4) Interesting, it is not changed then.

5) Then fixed the .mq4 file to be rates_total

Also the encoding in the original was automatically detected as UCS-2 LE BOM, for some unknown reason, which made impossible to compare with UTF-8 and the reason Github did not like to show the commits other than "Binary file".

I believe it is almost set for push now, we reviewed the key points - the functionality for .mq5 is guaranteed because I tested those changes. What I don't have is the MT4, please confirm if MT4 is working.

Now it is possible to stylize with the native stylizer, using mode UTF-8 for future-proof, mode "Allman", 4 spaces per indentation, convert tab to spaces, delete empty lines, insert space after commas and semicolons, insert spaces around statement operators. This makes guaranteed to not break anymore with a mix of \t and spaces, and comparisons will work flawess: diff -u -E -Z -b -B -w --color -i "$fileToEdit" "$fileToCompare" | diff-highlight # Difference by character or git diff --word-diff=color --word-diff-regex=. -w $fileToEdit $fileToCompare # Git difference

I tried to install MT4, but the website Download force-redirected to the MT5 installer and the installer almost broke my MT5 lol (it broke the file associations), pretty bad move of then to have this bug (if not purposefully); look at it, https://www.metatrader4.com/en/download/ the button "Download MetaTrader 4 for PC and create a demo account" actually redirects to mt5 unwillingly... mt4setup.exe has 1.322.168 bytes and mt5setup.exe has 1.363.000 bytes, but both are MT5.

EarnForex commented 3 years ago
  1. I wouldn't want those blank lines to be removed. They help to declutter the code.

Your version seems to be working fine in MT4.

Since quite a long time, you cannot install MT4 from the official website. MT4 installer will install MT5 for you. You can only install MT4 from some broker.

BRMateus2 commented 3 years ago

How would be the best approach about the empty lines? Explicitly: using the separator //+------------------------------------------------------------------+ so we define it as needed, or a wonky // line with nothing after the comments?

Or as in the original style, should I roll back all the empty lines, with them being a implicit separator? We won't be able to enable the "Remove empty lines" from stylizers this way.

EarnForex commented 3 years ago

I would prefer leaving them as is, without any comments. "Remove empty lines" shouldn't be required because every empty line was most likely placed on purpose.

BRMateus2 commented 3 years ago

I'll check it at the weekend, it's been some busy days- and holy moly the climate, here in the south hemisphere is so cold it's "hurting the bone", record lows while Canada had record highs.

EarnForex commented 3 years ago

Any update on this? I would like to start working on some bug fixes and new features and would like to start doing that in "fixed style" version.

BRMateus2 commented 3 years ago

Hello, I've rollback all empty lines without discrimination, and made parity with .mq4 and .mq5 empty lines.

I believe one of the biggest features, would be a object limiter, like 100 Objects per Session (and Interpolation between gaps) - this is what I am developing (in my own brain, it's not even in code) and might be a high-performant alternative (will load on any Timeframe pretty fast).

Maybe in up to a year I will have a repo with my own idea, while I try to improve my other repos.

EarnForex commented 3 years ago

Thanks a lot for your contribution!