Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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:

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 ...

share|improve this question
2  
Could you please replace the title with something that describes the code itself? It shouldn't solely contain the review request. –  Jamal Sep 2 at 16:54
5  
As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. –  Simon André Forsberg Sep 3 at 0:35
    
@SimonAndréForsberg this code is work in progress, so the only thing i want to know is how to improve the if else part because i think my if else problem is different from already answered Questions on this topic –  WiiMaxx Sep 3 at 6:09
    
@WiiMaxx: That request can still be in the question body. The title should still have some context about the code's purpose. –  Jamal Sep 3 at 6:46
2  
Not on CR question, though. By default, specific questions here would be very generic, and we don't need nearly all of the questions asking the same exact thing. –  Jamal Sep 3 at 6:53

7 Answers 7

up vote 11 down vote accepted

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;
      }
    }
}
share|improve this answer
    
Excuse me but i have to inform you a Panel is a FrameworkElement, by the way i thought like you until my Panel doesn't change there color :) –  WiiMaxx Sep 2 at 14:29
2  
@WiiMaxx As you didn't specify wich UI you were using I just assumed Win forms... Updated. –  Bruno Costa Sep 2 at 14:35

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.

share|improve this answer
1  
actually c# doesn't have a ALL_CAPS convention. But this is acceptable for constants and enums for the ones that have background in Java. But never in instance fields just like you pointed. –  Bruno Costa Sep 2 at 15:09
2  
@BrunoCosta Clarified my answer based on this point. –  unholysampler Sep 2 at 15:29

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.

share|improve this answer
    
wow nice catch, you are absolutely right about your Continue. i will also update my Question to show you all possible combinations –  WiiMaxx Sep 2 at 14:45
6  
@WiiMaxx you shouldn't not update your question because you would be invalidating answers. –  Bruno Costa Sep 2 at 14:46
    
+1 Nice answer :-) –  AquaAlex Sep 2 at 14:52
    
@WiiMaxx: Try to refactor it a bit, small part you focus on on top, more code next, don't change the meaning of the question please. You may enlarge it, add new content (and mark it as added), but again, don't change it completely, please. Describe few more thinks to make it more understandable. –  firda Sep 2 at 15:00
1  
@WiiMaxx That's ok then, I was just trying to warn you to not edit it in such way you could invalidate answers. –  Bruno Costa Sep 2 at 15:17

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.

share|improve this answer
2  
"I don't see a problem with your if statement." Really? –  RubberDuck Sep 2 at 22:18
1  
Yes really. 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. None of the other answers have improved the if-else in any substantial sense ("indents and braces" are just layout), some answers have even made things worse by over-complicating it. 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. –  James Sutherland Sep 3 at 9:38
    
Put that in your answer. –  RubberDuck Sep 3 at 9:44

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
share|improve this answer

Of the bat if we use nested if / else if we always replace it with a SWITCH / CASE statement, easier to read and more efficient

Sometimes we have to pull small tricks like combining 1 or more variables when comparing. Also you can nest SWITCH statements.

EXAMPLE:

switch (v)
    {
      case 0:
        return 10;
      case 1:
        return -1;
      case 2:
        return 20;
      default:
        return 0;
    }
share|improve this answer
    
i was also thinking about a switch case, but i'm not sure how to implement this approach in this case. Could you please elaborate your suggetion –  WiiMaxx Sep 2 at 13:45
    
I think firda has already showed how a switch can be used ;) –  AquaAlex Sep 2 at 14:54

I am going to show you a little thing.

//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);

All you really want here is the relativeEndPoint don't create 4 variables that you aren't going to use again, they may help to layout what you are doing in the first place, but then get rid of them.

Point relativeSartPoint = myVisual.TransformToAncestor((Visual)myPage).Transform(new Point(0, 0));

Point relativeEndPoint = 
    new Point(
        (relativeSartPoint.X + (double)myVisual.GetValue(FrameworkElement.ActualWidthProperty)),
        (relativeSartPoint.Y + (double)myVisual.GetValue(FrameworkElement.ActualHeightProperty)));

It's a little longer of a command, but it lays out exactly what you are doing and makes 4 variables disappear, when you do this 100 times in a program (or more) it adds up and slows things down, so get straight to the point.

share|improve this answer
    
you are right, but in my current code Link i need at least aw and ah :) –  WiiMaxx Sep 3 at 15:01
    
no you don't, same thing there as here, you only use them in one place, use (double)myVisual.GetValue(FrameworkElement.ActualWidthProperty) in the new Size(w, h) for your w and (double)myVisual.GetValue(FrameworkElement.ActualHeightProperty) for your h –  Malachi Sep 3 at 15:09

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.