Problem
How would you simplify/improve the readability of this code? Because even after my attempt to improve the readability I think it still looks messy.
private bool INTOP = false;
private bool INLEFT = false;
private bool INRIGHT = false;
private bool INBOTTOM = false;
private bool BEYOND = false;
private List<DependencyObject> BEYONDDO = new List<DependencyObject>();
private List<DependencyObject> FULLOUT = new List<DependencyObject>();
private List<DependencyObject> TOPLEFTDO = new List<DependencyObject>();
private List<DependencyObject> TOPDO = new List<DependencyObject>();
private List<DependencyObject> TOPRIGHTDO = new List<DependencyObject>();
private List<DependencyObject> LEFTDO = new List<DependencyObject>();
private List<DependencyObject> CURRENTPAGEDO = new List<DependencyObject>();
private List<DependencyObject> RIGHTDO = new List<DependencyObject>();
private List<DependencyObject> BOTTOMLEFTDO = new List<DependencyObject>();
private List<DependencyObject> BOTTOMDO = new List<DependencyObject>();
private List<DependencyObject> BOTTOMRIGHTDO = new List<DependencyObject>();
private bool ForeachChildIn(DependencyObject myPage, IList<DependencyObject> childList)
{
var result = false;
foreach (var item in childList)
{
var itemresult = false;
if (item is Visual && myPage is Visual)
{
var myVisual = item as Visual;
//relative Position des myVisual zu VisualElement
Point relativeSartPoint = myVisual.TransformToAncestor((Visual)myPage)
.Transform(new Point(0, 0));
var aw = (double)myVisual.GetValue(FrameworkElement.ActualWidthProperty);
var ah = (double)myVisual.GetValue(FrameworkElement.ActualHeightProperty);
var relativeEndHorizontal = relativeSartPoint.X + aw;
var relativeEndVertikal = relativeSartPoint.Y + ah;
Point relativeEndPoint = new Point(relativeEndHorizontal, relativeEndVertikal);
INTOP = (PageRect.Top <= relativeSartPoint.Y);
INBOTTOM = (PageRect.Bottom >= relativeEndPoint.Y);
INLEFT = (PageRect.Left <= relativeSartPoint.X);
INRIGHT = (PageRect.Right >= relativeEndPoint.X);
BEYOND = (PageRect.Top >= relativeEndPoint.Y)
|| (PageRect.Bottom <= relativeSartPoint.Y)
|| (PageRect.Left >= relativeEndPoint.X)
|| (PageRect.Right <= relativeSartPoint.X);
if (IsIN())
{
result = true;
itemresult = true;
}
else if (BEYOND)
BEYONDDO.Add(item);
else if (((relativeSartPoint.X == 0 && relativeSartPoint.Y == 0) || (IsTopOverlap()) && IsLeftOverlap()) && IsRightOverlap() && IsBottomOverlap())
FULLOUT.Add(item);
else if (IsTopOverlap() && IsLeftOverlap())
TOPLEFTDO.Add(item);
else if (IsTopOverlap() && IsRightOverlap())
TOPRIGHTDO.Add(item);
else if (IsTopOverlap())
TOPDO.Add(item);
else if (IsBottomOverlap() && IsLeftOverlap())
BOTTOMLEFTDO.Add(item);
else if (IsBottomOverlap() && IsRightOverlap())
BOTTOMRIGHTDO.Add(item);
else if (IsBottomOverlap())
BOTTOMDO.Add(item);
else if (IsRightOverlap())
RIGHTDO.Add(item);
else if (IsLeftOverlap())
LEFTDO.Add(item);
}
if (!itemresult)
if (ForeachChildIn(item, item.getChilds().ToList()))
result = true;
else if ((Visibility)item.GetValue(FrameworkElement.VisibilityProperty) == Visibility.Visible)
//item.SetValue(FrameworkElement.VisibilityProperty, Visibility.Hidden);
//this part is just for testing
if (item is Control)
((Control)item).Background = Brushes.LimeGreen;
else if (item is Panel)
((Panel)item).Background = Brushes.LimeGreen;
}
return result;
}
My attempt to improve readability by encapsulating conditions in meaningful methods:
private bool IsIN()
{
return INTOP && INLEFT && INRIGHT && INBOTTOM;
}
private bool IsTopOverlap()
{
return !BEYOND && !INTOP;
}
private bool IsLeftOverlap()
{
return !BEYOND && !INLEFT;
}
private bool IsRightOverlap()
{
return !BEYOND && !INRIGHT;
}
private bool IsBottomOverlap()
{
return !BEYOND && !INBOTTOM;
}
Disclaimer
This code isn’t in production it is just for testing purposes.
To answer the question about what and how many combinations are possible here are all possible combination to make it a bit more visual think about a Numpad:
We have 9 basic positions.
Number 5 represents IsIN(). Number 1, 2, 3, 4, 6, 7, 8, 9 are represented by BEYOND. After these basic positions we come to the overlapping if a DO covers Number 5 (partly or full) +
7,8,9 it is represented by IsTopOverlap()
or
1,4,7 it is represented by IsLeftOverlap()
or
9,6,3 it is represented by IsRightOverlap()
or
1,2,3 it is represented by IsBottomOverlap()
Now we get nearly all possible combinations because we can check for intersections on the corners, but there could DO which covers more than that they are represented by
((relativeSartPoint.X == 0 && relativeSartPoint.Y == 0) || (IsTopOverlap()) && IsLeftOverlap()) && IsRightOverlap() && IsBottomOverlap()
If some of you is interested in what the current code looks like
here a link to SO Creating an Intelligent DocumentPaginator …
Solution
Please, pretty please, use braces. They are totally lacking in your code.
if (!itemresult)
if (ForeachChildIn(item, item.getChilds().ToList()))
result = true;
else if ((Visibility)item.GetValue(FrameworkElement.VisibilityProperty) == Visibility.Visible)
if (item is Control)
((Control)item).Background = Brushes.LimeGreen;
else if (item is Panel)
((Panel)item).Background = Brushes.LimeGreen;
This bit of code is so hard to read, that it hurts my eyes!
There are two major issues with your code.
The first one is obviously the one you pointed out, which is the way you are doing your if’s.
The second is the amount of lists that you have in your code.
You also have some instance fields that you shouldn’t have, like those pesky boolean values. They can be declared inside a method.
I would replace all those lists with a dictionary. This makes the code become much simpler, as you only have to manage one variable instead of managing n variables.
So your code could be something along those lines.
public enum Position{
LEFT, TOP_LEFT, TOP, TOP_RIGHT, RIGHT,
BOTTOM_RIGHT, BOTTOM, BOTTOM_LEFT,
FULL_OUT, BEYOND, IN
}
private Position[] avaliablePositions = new Position[]{
LEFT, TOP_LEFT, TOP, TOP_RIGHT, RIGHT,
BOTTOM_RIGHT, BOTTOM, BOTTOM_LEFT,
FULL_OUT, BEYOND, IN
};
private Dictionary<Position, List<DependencyObject> objectsForPosition = new Dictionary<Position, List<DependencyObject>>();
private Position GetPositionWithinPageRect(Point startPoint, Point endPoint){
bool inTop = (PageRect.Top <= startPoint.Y);
bool inBottom = (PageRect.Bottom >= endPoint.Y);
bool inLeft = (PageRect.Left <= startPoint.X);
bool inRight = (PageRect.Right >= endPoint.X);
bool beyond = (PageRect.Top >= endPoint.Y)
|| (PageRect.Bottom <= startPoint.Y)
|| (PageRect.Left >= endPoint.X)
|| (PageRect.Right <= startPoint.X);
if(inTop && inLeft && inRight && inBottom){
return Position.IN;
}
if (startPoint.X == 0 && startPoint.Y == 0 || !beyond && !inTop && !inLeft && !inRight && !inBottom)
return Position.FULL_OUT;
if(!beyond){
if(!inTop){
if(!inRight){
return Position.TOP_RIGHT;
}
if(!inLeft){
return Position.TOP_LEFT;
}
return Position.TOP;
}
if(!inBottom){
if(!inRight){
return Position.BOTTOM_RIGHT;
}
if(!inLeft){
return Position.BOTTOM_LEFT;
}
return Position.BOTTOM;
}
if(!inRight){
return Position.RIGHT;
}
if(!inLeft){
return Position.LEFT;
}
}else{
return Position.BEYOND;
}
}
private bool ForeachChildIn(DependencyObject myPage, IList<DependencyObject> childList)
{
var result = false;
//move this on you ctor
for(int i = 0; i < avaliablePositions.Length; ++i){
objectsForPosition.Add(avaliablePositions[i], new List<DependencyObject>());
}
foreach (var item in childList)
{
var myVisual = item as Visual;
if(myVisual == null) continue;
Point relativeSartPoint = myVisual.TransformToAncestor((Visual)myPage)
.Transform(new Point(0, 0));
var aw = (double)myVisual.GetValue(FrameworkElement.ActualWidthProperty);
var ah = (double)myVisual.GetValue(FrameworkElement.ActualHeightProperty);
Point relativeEndPoint = new Point(relativeSartPoint.X + aw, relativeSartPoint.Y + ah);
Position position = GetPositionWithinPageRect(relativeSartPoint, relativeEndPoint);
if(position != Position.IN){
if (!ForeachChildIn(item, item.getChilds().ToList())){
if ((Visibility)item.GetValue(FrameworkElement.VisibilityProperty) == Visibility.Visible){
if (item is Control)
((Control)item).Background = Brushes.LimeGreen;
else if (item is Panel)
((Panel)item).Background = Brushes.LimeGreen;
}
}
return false;
}else{
objectsForPosition[position].add(item);
return true;
}
}
}
About your question:
All of your bool variables are assigned to for each iteration without any concern to what the previous value was. This is a good indication that making the instance variables is not the correct scoping of this information.
The reason you have put them there is so that you can call the various helper methods for later if clauses. A better solution would be to make a class for this information. The only thing this class would be concerned with will be how the location of the point relates to PageRect. By doing this, you are not polluting the class with multiple variables and methods that are only used to try and make a single method cleaner.
These boolean values are being used to decide what specific category the value belongs to. This means you have a fixed set of categories and want to perform a simple decision based on the chosen category (list to add to). This is a perfect case to use an enum and a switch statement.
You have a single function that performs the boolean logic to decide which enum should be returned. Then, one method that switches on the returned value in order to add the value to the correct list.
Other parts of the code that should be improved:
ALL_CAPS is a common naming convention for constant values. Don’t use it for variables that change or are mutated during a method call. When using the all caps naming convention, use underscores separate words. It makes reading the name easier.
For C#, the naming convention for constants, like many other things in the language, is PascalCase. However, ALL_CAPS is also generally acceptable because of its prevalence across other languages.
The name ForeachChildIn() tells me nothing about what this method actually does. Sure it iterates over the collection, but that is the boring part. The interesting part is sorting the items into categories. The name should talk about that.
IsIn() has similar problems. What does “in” mean?
There is nothing to say what the return value means. It could be all children were added or it could be at least one was added. Does the caller need to know this information? Is it important to know which items were added? These are things to think about when picking a return value and documenting it.
The code block starting with
if (IsIN())
{
result = true;
itemresult = true;
}
else if (BEYOND)
BEYONDDO.Add(item);
is indented incorrectly. If you just look at the columns it seem like it is a new if after item is Visual && myPage is Visual block as finished.
This problem is made worse by the fact that you some times use braces.
else if (IsLeftOverlap())
LEFTDO.Add(item);
}
If you look at just these 3 lines, it is clearly a syntax error. But it is actually valid in your code.
Deep nested if else trees are very easy to get wrong when you are not explicitly including the braces.
if (!itemresult)
if (ForeachChildIn(item, item.getChilds().ToList()))
result = true;
else if ((Visibility)item.GetValue(FrameworkElement.VisibilityProperty) == Visibility.Visible)
//item.SetValue(FrameworkElement.VisibilityProperty, Visibility.Hidden);
//this part is just for testing
if (item is Control)
((Control)item).Background = Brushes.LimeGreen;
else if (item is Panel)
((Panel)item).Background = Brushes.LimeGreen;
Changing the indentation will make this code seem like it does something very different. Also, once you are done testing, you will go back an uncomment item.SetValue(FrameworkElement.VisibilityProperty, Visibility.Hidden);. But if you forget to remove the following lines, they will always execute.
Always include braces. It will make your life much easier.
if (myVisual is Button)
{
var k = myVisual as Button;
}
You never do anything with this section of code.
If myPage is not an instance of Visual you will do a whole bunch of work to simply reject all of the children. Changing the function definition will prevent you from even attempting to do this. Similarly, you should look to see if you can do that same thing with the collection of children.
I don’t see a problem with your if statement. I would simplify by putting all that if statement logic in a separate method that returns a VisualPosition enum value. This would massively cut down the code in ForeachChildIn and remove the need for all those separately named lists.
enum VisualPosition
{
Beyond,
In,
Top,
Left,
TopLeft,
In,
Etc
}
VisualPosition GetPosition(Rectangle pageRect, DependencyObject object)
{
}
private bool ForeachChildIn(DependencyObject myPage, IList<DependencyObject> childList)
{
VisualPosition position = GetPosition(PageRect, myVisual);
_positions[position].Add(myVisual);
}
Dictionary<VisualPosition, List<DependencyObject>> _positions;
The problem here is not in the if-else statement itself, but the way it is embedded in the middle of a bunch of other concerns. The fact is that these kind of position checks in 2D require testing against every compass direction one after the other. In these cases it is best to just encapsulate such logic in a method, like GetPosition.
I will start with this:
if (IsIN())
{
result = true;
itemresult = true;
}
.
.
if (!itemresult) ...
Cannot see it being set to true anywhere else => should be joined + continue, no need for the variable at all, if I see correctly. Please, update your post and indent properly.
Now about helpers and possible switch-case:
[Flags] enum StateFlags {
InTop = 1<<0,
InLeft = 1<<1,
... }
This can help instead of your private helpers.
if((flags & (InTop|InLeft)) == (InTop|InLeft)) ...
if((flags & InTop) != 0) ...
note: consider using some helper struct instead.
I would use some variable for the list
List<DependencyObject> list = null;
if(...) list = XYZDO;
else if((flags & InTop) != 0) switch(flags & (InLeft|InRight)) {
case StateFlags.InLeft: ...
if(list != null) list.Add(...);
I still do not fully understand the intent and which combinations are possible and which are not.
After update:
switch(flags) {
case State.Left: ...
case State.Right: ...
default:
// whatever cannot be solved by above
But I am still really not sure about the combinations. You can at least switch-case those easy and leave the other logic on the default.
Instead of looking for the condition in the main function, why not evaluate it in the the smaller functions and return true if an item was added or not.
Perhaps something like bool TryAddX(item)
You can then use or to force the order of the calls.
if (IsIN())
{
result = true;
itemresult = true;
}
else if (!(
TryAddBeyond(item) ||
TryAddFullout(item, relativeSartPoint) ||
TryAddTopLeft(item) ||
TryAddTopRight(item) ||
TryAddTop(item) ||
TryAddBottomLeft(item) ||
TryAddBottomRight(item) ||
TryAddBottom(item) ||
TryAddRight(item) ||
TryAddLeft(item)
))
{
// case if no condition found
}
Then some example functions would look like:
private bool TryAddBeyond(var item)
{
if (BEYOND)
{
BEYONDDO.Add(item);
return true;
}
return false;
}
private bool TryAddFullout(var item, Point relativeSartPoint)
{
if (((relativeSartPoint.X == 0 && relativeSartPoint.Y == 0) || (IsTopOverlap()) && IsLeftOverlap()) && IsRightOverlap() && IsBottomOverlap())
{
FULLOUT.Add(item);
return true;
}
return false;
}
//... etc