Get all members from each group I have a table of groups, and a table of members. Each member belongs to one group. This is how it looks (more groups then 2 are possible):

Groups

id | name | year |
------------------
1  | Nice | 1966 |
2  | Hard | 2012 |

Member

id | name  | idGroup |
----------------------
1  | Bernd | 1       |
2  | Max   | 1       |
3  | Tom   | 2       |
4  | Bob   | 1       |
5  | Alice | 2       |

I want to compute a table, where each group is listed once, together with all its members, like this:

name | year | members         |
-------------------------------
Nice | 1966 | Bernd, Max, Bob |
Hard | 2012 | Tom, Alice      |

At the moment, I use two queries to get this table:

    Groupname
    Year
    Members

<?php
$db           = new mysqli("localhost","user","pass","database");
$sql          = "SELECT id, name, year FROM groups";
$groupsResult = $db->query($sql);

while($group = $groupsResult->fetch_assoc())
{
  $groupName = htmlspecialchars($group['name']);
  $groupYear = htmlspecialchars($group['year']);
  $members   = "";

  $stmt         = $db->prepare('SELECT name FROM member WHERE idGroup = ?');
  $stmt->bind_param('i',$group['id']);
  $stmt->execute();
  $memberResult =   $stmt->get_result();

  while($member =   $ergebnis->fetch_assoc())
  {
    if(!empty($members)) $members .= ", ";
    $members .= htmlspecialchars($member["name"]);
  }

  ?>

    <?php echo $groupName; ?>
    <?php echo $groupYear; ?>
    <?php echo $members; ?>

<?php
}
?>

But I have the feeling that this is not a good approach.

I know that I can get with an INNER JOIN the result.

id | name  | groupName | year
------------------------------
1  | Bernd | Nice      | 1966
2  | Max   | Nice      | 1966
3  | Tom   | Hard      | 2012
4  | Bob   | Nice      | 1966
5  | Alice | Hard      | 2012

but this does not seem to help me. Is it possible to get with one SQL directly the result:

name | year | members         |
-------------------------------
Nice | 1966 | Bernd, Max, Bob |
Hard | 2012 | Tom, Alice      |

or is there another way how I can optimize this code?

$sql = <<<'EOSQL'
SELECT g.name, g.year, GROUP_CONCAT(m.name SEPARATOR ', ') AS members
FROM groups g INNER JOIN members m on g.id = m.idGroup
GROUP BY m.idGroup
EOSQL;

The key insight here is that GROUP_CONCAT and GROUP are what you need to combine multiple rows of the result into one.  Note that GROUP is a SQL name.  It is just coincidence that you also have a table named groups.

I also used a Nowdoc because the SQL string is using multiple lines.  I picked Nowdoc because we aren't doing any interpolation or other escaping in the string.

It's obsolete now (assuming you use the INNER JOIN version), but in

  while($member =   $ergebnis->fetch_assoc())
  {
    if(!empty($members)) $members .= ", ";
    $members .= htmlspecialchars($member["name"]);
  }

It looks like you forgot to change a variable name.  I think that $ergebnis should be $memberResult.  But the main point I wanted to make is that at the expense of some mildly duplicate code, you don't need to check every iteration to see if it is the first.

  if ($member = $memberResult->fetch_assoc())
  {
    $members = htmlspecialchars($member['name']);

    while ($member = $memberResult->fetch_assoc())
    {
      $members .= ', ' . htmlspecialchars($member['name']);
    }
  }

On the first row, you just set the variable.  On subsequent rows, you append the separator and the new value to the variable.  This produces the same result without all the checks to see if you are on the first iteration.

I find it more readable to put a space between while and the opening parenthesis.  That makes it easier to see that we are not making a function call.  The additional whitespace also breaks up the line and makes it easier for a human to parse (the compiler doesn't care either way).

In PHP, a double-quoted string allows variable interpolation.  So if I'm not using variable interpolation, I use a single-quoted string.  There's a slight performance benefit.  But the real advantage is that when I see a single-quoted string, I know that I don't have to look for variables being interpolated.

  ?>

I find it more readable to always put the ?> at the far left (no indent) when the PHP block spans multiple lines.  Perhaps that is just me, but I find it easier to scan for ?> if they are always in the same place.


ASP MVC Controller Unit Test So I've been writing a unit test using Rhino Mocks to a controller in my ASP MVC application. The test passes, but I'm interested in your opinion and if there's something I should be doing differently or what it could be.

Here's my test method:

    [TestMethod]
    public void TestSlideView()
    {
        // Arrange
        var repositoryMock = MockRepository.GenerateMock<ISlideRepository>();
        var controller = new SlideController(repositoryMock);
        var expectedSlides = new List<SlideModel>();
        expectedSlides.Add(new SlideModel
        {
            Id = "id"
        });
        repositoryMock.Stub(x => x.GetSlides("id")).Return(expectedSlides);

        // Act
        var actualView = controller.Slide("id") as ViewResult;
        var actualData = actualView.Model;

        // Assert
        Assert.IsNotNull(actualView);
        Assert.IsNotNull(actualData);
        Assert.AreEqual("Slide", actualView.ViewName);
    }

Here's the controller under test:

    public ActionResult Slide(string slideid)
    {
        var slides = slideRepository.GetSlides(slideid);

        return View("Slide", slides);
    }

The repository the controller uses is here:

public interface ISlideRepository
{
    List<SlideModel> GetSlides(string id);
}

public class SlideRepository : ISlideRepository
{
    public SlideModel Slides { get; set; }
    public List<SlideModel> SlideList { get; set; }

    public List<SlideModel> GetSlides(string id)
    {
        string cs = dbPath;
        string slideid = id;

        using (SQLiteConnection con = new SQLiteConnection(cs))
        {
            var listOfSlides = new List<SlideModel>();
            string stm = "SELECT * FROM Slide WHERE ID = " + slideid;
            con.Open();

            using (SQLiteCommand cmd = new SQLiteCommand(stm, con))
            {
                using (SQLiteDataReader rdr = cmd.ExecuteReader())
                {
                    while (rdr.Read())
                    {
                        listOfSlides.Add(new SlideModel
                        {
                            Id = rdr["ID"].ToString()
                        });
                    }

                    rdr.Close();
                    SlideList = listOfSlides;
                }
            }

            con.Close();
        }

        return SlideList;
    }
}

Any input is greatly appreciated!

The string "id" is used three times in TestSlideView(): make it a const string instead.

Same for "Slide", as used in TestSlideView() and Slide(string slideid).

Don't name something SlideList, name it Slides. And yes, I know you named it SlideList because you already have a property called Slides, but Slides isn't an appropriate name for a single SlideModel anyway.

Even worse: listOfSlides.

Your variable names don't tell me what they contain: cs, stm, etc. I can figure out that "cs" means "connectionString", but I shouldn't be wasting effort on such things.

Also, why bother assigning dbPath to cs? Why not simply use dbPath (and perhaps rename that to connectionString)?

Same for assigning id to slideid -- which should be named slideId since it is a compound word. Matter of fact, why isn't the parameter named slideId in the first place?

"Danger, Will Robinson!" You code is now vulnerable to SQL injection:

"SELECT * FROM Slide WHERE ID = " + slideid;

Use the Parameters collection instead.

Also: looping through a SQLiteDataReader seems quaint to me. Why not store the result in a DataTable and work with that? Or even better: why not work with an ORM like NHibernate or Entity Framework?


How to merge two descending singly linkedlist into one ascending linkedlist using Recursion without any helper method? Here is my solution:
I want to know if my solution fits the requirement 100%? Or if there is any better solution?
Constraint of the question:

must be singly linkedlist
must use recursion, and no helper methods allowed.
The method must return the head node of the merged list.

class Solution{
    Node dummy=new Node(0,null);// the dummy node to store the merged result

    public   Node mergeAscend(Node a,Node b){
        if(a==null&&b==null){//base case
            return null;
        }

        else{
            if((a!=null&&b==null)||a.value>=b.value){// insert "b" after dummy
                //store the next node of current a, before pointing a.next to dummy.next;
                Node store_a_next_node=a.next;
                //insert Node "a" between dummy and dummy.next
                a.next=dummy.next;
                dummy.next=a;
                mergeAscend(store_a_next_node,b);
            }
            else if((a==null&&b!=null)||a.value<b.value){//insert "a" after dummy
                Node store_b_next_node=b.next;
                b.next=dummy.next;
                dummy.next=b;
                mergeAscend(a,store_b_next_node);
            }
        }
        return  dummy.next;
    }
}

class Node {
    public int value;
    public Node next;

    public Node(int value, Node next) {
        this.value = value;
        this.next = next;
    }
}

Compilation Error
The first problem is to solve the compiling error.
Just follow the interface:
    public Node<Integer> mergeAscend(Node<Integer> a, Node<Integer> b)

Warnings and Code convention
You should add the type information to the store_next_a_node variable (and store_next_b_node). Additionally, in Java naming conversion, variable names use camel case. Therefore, I suggest substitute:
            Node store_next_a_node = a.next;
           ...
            Node store_next_b_node = b.next;

By...
            Node<Integer> nextNode = a.next;

Simple "Test Case"
The code bellow is just an example and not a full automated test (for more information, look for Java automated tests. eg: junit tutorial).But it is enough to  give us some hints what is going on. For example, we can see the original code does not solve the problem. Let us write a simple test case:
    public static void main(String[] args) {
        Solution solution = new Solution();
        int[] a = { 6, 4, 2 };
        Node<Integer> h1 = new Node<>(8, null);
        Node<Integer> previous = h1;
        for (int value : a) {
            Node<Integer> node = new Node<>(value, null);
            previous.next = node;
            previous = node;
        }

        int[] b = { 7, 5, 3, 1 };
        Node<Integer> h2 = new Node<>(9, null);
        previous = h2;
        for (int value : b) {
            Node<Integer> node = new Node<>(value, null);
            previous.next = node;
            previous = node;
        }
        solution.mergeAscend(h1, h2);
        Node<Integer> iterator = solution.dummy;
        while (iterator != null) {
            System.out.println(iterator.value);
            iterator = iterator.next;
        }
    }

The expected output would be the numbers printed in ascending order as the exercise suggests.
NullPointerException
With the test case, we can execute the code and and we will get a null pointer exception during the execution:
Exception in thread "main" java.lang.NullPointerException
    at Solution.mergeAscend(Solution.java:13)
    at Solution.mergeAscend(Solution.java:19)
    at Solution.mergeAscend(Solution.java:24)
    at Solution.mergeAscend(Solution.java:19)
    at Solution.mergeAscend(Solution.java:24)
    at Solution.mergeAscend(Solution.java:19)
    at Solution.mergeAscend(Solution.java:24)
    at Solution.mergeAscend(Solution.java:19)
    at Solution.mergeAscend(Solution.java:24)
    at Solution.main(Solution.java:50)

Bug
If you look the code, the following statement is suspicious:
if((a!=null&&b==null)||a.value>=b.value)

What if a is null and b is not null? First condition is false and Java interpreter will try to evaluate the second condition. Unfortunately, a.value will throw a NullPointerException.
Next Step: How to fix it ?
Clean Code makes our life easier
Let's make the code cleaner . In our case, we want to preserve the original code ideas and just simplifying what is possible. Hopefully we will remove the conditions that make the code to fail.
Remove unnecessary else-statement
First, after the base case, we don't need a else-statement. Change..
        if (a == null && b == null) {//base case
            return null;
        }
        else {
        ...
        }

to
        if (a == null && b == null) {//base case
            return null;
        }
        ...

Nullability test only once
Second, remove the complication of testing a nullability all the time.
Just add the following check after the base case.
        if (a == null)
            return mergeAscend(b, a);

With that, now we know that a is the longest list and we don't need to check again and again that condition.
So, your second "if-statement" can be change from
        if ( (a!=null && b==null) || a.value >= b.value)

to:
        if (b == null || a.value >= b.value) // insert "b" after dummy

Remove unnecessary if-statement
After that, we might think about it... we don't need the third "if-statement". Just remove it.
 else if((a==null&&b!=null)||a.value<b.value){
     ...
 }

To...
 else {
      ...
  }

Congrats, your code now works!
    public Node<Integer> mergeAscend(Node<Integer> a, Node<Integer> b) {
        if (a == null && b == null) {//base case
            return null;
        }
        if (a == null)
            return mergeAscend(b, a);
        if (b == null || a.value >= b.value) {// insert "a" after dummy
            //store the next node of current a, before pointing a.next to dummy.next;
            Node<Integer> nextNode = a.next;
            //insert Node "a" between dummy and dummy.next
            a.next = dummy.next;
            dummy.next = a;
            mergeAscend(nextNode, b);
        } else {
            Node<Integer> nextNode = b.next;
            b.next = dummy.next;
            dummy.next = b;
            mergeAscend(a, nextNode);
        }
        return dummy.next;
    }

Test it!
PS: Another Minor Thing - Comment misleading
Your code comments are misleading. In the second if-statement, if the statement evaluate the expression as true, code below add a between dummy and dummy.next.
change
// insert "b" after dummy

To
// insert "a" after dummy

PS2: Symmetric symplification
Evaluating the code again, we might found the code is a little redundant. It means, part of the code is doing exactly the same thing, but the variable names are just swapped. In that case, we can simplify it further.
    public Node<Integer> mergeAscend(Node<Integer> a, Node<Integer> b) {
        if (a == null && b == null) {//base case
            return dummy.next;
        }
        if (a == null || (b != null && a.value < b.value)) { // symmetric case
            return mergeAscend(b, a);
        }
        //store the next node of current a, before pointing a.next to dummy.next;
        //insert Node "a" between dummy and dummy.next
        Node<Integer> nextNode = a.next;
        a.next = dummy.next;
        dummy.next = a;
        return mergeAscend(nextNode, b);
    }


How to design interface of deep-copy behaving pointer containers? I want to make a container which manages big objects. which performs deep copies on copy construction and copy assignment. I also like the interface of the std containers, so I wanted to implement it using public inheritance:

template <class TBigObject>
class Container : public std::vector< std::shared_ptr<TBigObject> >
{
public:
    Container(int nToAllocate){ /* fill with default constructed TBigObjects */}
    Container(const Container& other){ /* deep copy */ }
    Container(Container&&) = default;
    Container& operator = (const Container& population){ /* deep copy */ }
    Container& operator = (Container&&) = default;
};

I heard of the "Thou shalt not inherit from std containers" maxim and the reasons behind it. So I decided to make an alternative:

template <class T>
using Container = std::vector< std::shared_ptr<T> >;

template <class T>
Container<T> defaultAllocate(int nItems);

template <class T>
Container<T> deepCopy(const Container<T>& other);

This looks more expressive, but using the code feels strange and verbose:

class Big;
Container<Big> someContainer = defaultAllocate(12);
Container<Big> copy = deepCopy(someContainer);

instead of:

class Big;
Container<Big> someContainer(12);
Container<Big> copy = someContainer;

I am a beginner programmer and I don't want to make errors early on. I would like to ask your advice on which choice to make. Or even better, if there is a third option.

I think you are going about it the wrong way.

Containers already perform copy on copy construction/assignment because they are designed for value objects not pointers. What you really want is a wrapper object for pointers that performs a deep copy when that object is copied.

This has the added advantage of working with all containers.

If you define the move operators then it also works well with re-size operations as the move semantics will be applied to the wrapper when the the container is re-sized.

The other advantage here is you can make the wrapper behave like the underlying type. So you get easy access to all the algorithms.

template<typename T>
class Deep
{
   T*     value;
   public:
       Deep():                      value(NULL)         {}
       // Take ownership in this case.
       Deep(T const* value):        value(value)        {}
       Deep(T const& value):        value(new T(value)) {}
       ~Deep()                      {delete value;}
       Deep(Deep&& move)            {std::swap(value, move.value);}
       Deep(Deep const& copy)       {T* tmp = new T(*(copy.value));swap(tmp,value);delete tmp;}
       // Note this does do a deep copy via copy and swap
       Deep& operator=(Deep copy)   {std::swap(value, copy.value);return *this;}
       Deep& operator=(Deep&& move) {std::swap(value, move.value);return *this;}

       // Note: undefined behavior if used when value is NULL.
       //       You may want to add logic here depending on use case.
       operator T&()                {return *value;}
       operator T const&() const    {return *value;}

       T* release()                 {T* tmp = value;value = NULL;return tmp;}
};
int main()
{
    std::vector<Deep<int>>   deep(5,7);
}


Listing people with their phone numbers (if any) I have the following tables in my database:

people:          phones:
+----+------+    +-----------+------------+-------------+
| id | name |    | person_id | number     | description |
+----+------+    +-----------+------------+-------------+
|  1 | Jade |    |         1 | 1234567890 | home        |
|  2 | Ben  |    |         1 |  987654321 | office      |
+----+------+    +-----------+------------+-------------+

I use a query with INNER JOIN to select the name and phone numbers for each person:

SELECT
   id,
   name,
   group_concat(
      concat(
         number,
         ' (',
         description,
         ')'
      ) ORDER BY description SEPARATOR ','
   ) AS phones
FROM people INNER JOIN phones
ON person_id = id
GROUP BY id;

Results from the above query:

+----+------+--------------------------------------+
| id | name | phones                               |
+----+------+--------------------------------------+
|  1 | Jade | 1234567890 (home),987654321 (office) |
+----+------+--------------------------------------+

But I want to use this query to go through all the people in my database and not just the ones with phone numbers, so I added the following row to the phones table:

INSERT INTO phones VALUES (0, 0, NULL);

And then modified the ON statement of the query to:

ON person_id = id OR person_id = 0

That way every person gets selected even if he has no phone numbers registered in the database:

+----+------+--------------------------------------+
| id | name | phones                               |
+----+------+--------------------------------------+
|  1 | Jade | 1234567890 (home),987654321 (office) |
|  2 | Ben  | NULL                                 |
+----+------+--------------------------------------+

Is this a proper way to achieve this? I don't really like the idea of having to make sure that the 0, 0, NULL row will always stay in the database in order to ensure the queries will work as intended. (since the company I develop this for might decide to clear the database)

This implementation requires calling an ensureNullRow() function every time before using this query, is this a good solution or would you do it differently?

What you are describing is known asLEFT OUTER JOIN

SELECT
   id,
   name,
   group_concat(
      concat(
         number,
         ' (',
         description,
         ')'
      ) ORDER BY description SEPARATOR ','
   ) AS phones
FROM people LEFT OUTER JOIN phones
ON person_id = id
GROUP BY id;

inner join => give me only rows with records in both tables
left outer join => give me all rows of from the left table along with any matching rows in the right..


Calculate the statistical mean of different files and save it into a CSV file In this code I calculate the statistical functions of values from different files and save them into CSV files (one filer for each statistical function).

def mean(data):
    pass

def standard_deviation(data):
    pass

def afd(data):
    pass

def norma_afd(data):
    pass

def asd(data):
    pass

def norma_asd(data):
    pass

def calculation(file):
    """
    Get the DataFrame and calculate for ever column the different statistical mean
    :param file: DataFrame 56x7681
    :return: 6 different 56x1 lists for (mean, std, afd, norm_afd, asd, norm_asd)
    """
    m, sd, afd_l, nafd, asd_l, nasd = ([] for _ in range(6))
    for column in file:
        data = file[column].to_numpy()
        m.append(mean(data))
        sd.append(standard_deviation(data))
        afd_l.append(afd(data))
        nafd.append(norma_afd(data))
        asd_l.append(asd(data))
        nasd.append(norma_asd(data))
    return m, sd, afd_l, nafd, asd_l, nasd

def run(load_path, save_path):
    """
    Get (yield) all the different DataFrame from a folder
    and calculate for each file the statistical mean and save it in a csv file

    :param load_path: the folder path to load all the different files
    :param save_path: the folder save path
    :return: none
    """
    m, sd, afd_l, nafd, asd_l, nasd = ([] for _ in range(6))
    for current_path, file in yield_data(load_path, data_type="data"):
        a, b, c, d, e, f = calculation(file)
        m.append(a)
        sd.append(b)
        afd_l.append(c)
        nafd.append(d)
        asd_l.append(e)
        nasd.append(f)

    if not os.path.exists(save_path):
        os.makedirs(save_path)

    pd.DataFrame(m).to_csv(save_path + os.path.sep + "mean.csv", index=False, header=False)
    pd.DataFrame(sd).to_csv(save_path + os.path.sep + "std.csv", index=False, header=False)
    pd.DataFrame(afd_l).to_csv(save_path + os.path.sep + "afd.csv", index=False, header=False)
    pd.DataFrame(nafd).to_csv(save_path + os.path.sep + "norm_afd.csv", index=False, header=False)
    pd.DataFrame(asd_l).to_csv(save_path + os.path.sep + "asd.csv", index=False, header=False)
    pd.DataFrame(nasd).to_csv(save_path + os.path.sep + "norm_asd.csv", index=False, header=False)

Is there a better and more efficient way to write this code? This may be a stupid question, but I would be really interested to know if there is a better way.

As the code cannot be run, here is a wild guess on a way to restructure it:

def mean(fileResults, data):
    result = 0
    # do computation
    fileResults["mean"].append(result)

def standard_deviation(fileResults, data):
    pass

def afd(fileResults, data):
    pass

def norma_afd(fileResults, data):
    pass

def asd(fileResults, data):
    pass

def norma_asd(fileResults, data):
    pass

def calculation(file):
    """
    Get the DataFrame and calculate for ever column the different statistical mean
    :param file: DataFrame 56x7681
    :return: 6 different 56x1 lists for (mean, std, afd, norm_afd, asd, norm_asd)
    """
    fileResults = {
        "mean": [],
        "std": [],
        "afd": [],
        "norm_afd": [],
        "asd": [],
        "norm_asd": [],
        }

    functionCallList = [
        mean,
        standard_deviation,
        afd,
        norma_afd,
        asd,
        norma_asd,
        ]

    for column in file:
        data = file[column].to_numpy()

        for functionCall in functionCallList:
            functionCall(fileResults, data)

    return fileResults

def run(load_path, save_path):
    """
    Get (yield) all the different DataFrame from a folder
    and calculate for each file the statistical mean and save it in a csv file

    :param load_path: the folder path to load all the different files
    :param save_path: the folder save path
    :return: none
    """
    results = {}

    for current_path, file in yield_data(load_path, data_type="data"):
        fileResults = calculation(file)

        for key, value in fileResults.items():
            if key not in results:
                results[key] = []

            results[key].append(value)

    if not os.path.exists(save_path):
        os.makedirs(save_path)

    for key, value in results.items():
        pd.DataFrame(value).to_csv(save_path + os.path.sep + key + ".csv", index=False, header=False)

So, instead of repeating function call, mostly when function prototypes are the same, you can use a list of function callbacks. Then just iterate it.

You can also use a dictionnary to store your data, instead of n lists. It's a little bit more scalable, and clearer than returning a 6-tuple. It also avoids a lot of copy-paste when you save csv files.


best design pattern for refactoring a set of classes that do calculation base on many parameters I'm refactoring a set of classes which does some price calculations.
the calculation is done based on many parameters.
The code is :

public interface IParcel {
    int SourceCode { get; set; }
    int DestinationCode { get; set; }
    int weight{get;set;}
    decimal CalculatePrice();
}

public abstract class GeneralParcel : IParcel {
    //implementation of inteface properties
    //these properties set with in SourceCode & DestinationCode
    //and are used in CalculatePrice() inside classes that inherit from GeneralParcel
    protected SourceProvinceCode{get; protected set;}
    protected DestinationProvinceCode{get;protected set;}

    //private variables we need for calculations
    private static ReadOnlyDictionary<int, List<int>> _States_neighboureness;
    private static ReadOnlyCollection<City> _Citieslist;
    private static ReadOnlyCollection<Province> _Provinceslist;

    protected ReadOnlyCollection<City> Citieslist {get { return _Citieslist; }}

    protected ReadOnlyCollection<Province> ProvincesList {get { return _Provinceslist; }}

    protected ReadOnlyDictionary<int, List<int>> StatesNeighboureness {get {return _States_neighboureness; }}

    //constructor code that initializes the static variables

    //implementation is in concrete classes
    public abstract decimal CalculatePrice();
}

public ExpressParcel : GeneralParcel {
    public decimal CalculatePrice() {
        //use of those three static variables in calculations
        // plus other properties & parameters
        // for calculating prices
    }
}

public SpecialParcel : GeneralParcel {
    public decimal CalculatePrice() {
        //use of those three static variables in calculations
        // plus other properties & parameters
        // for calculating prices
    }
}

Right now, the code uses "Strategy pattern" efficiently.

my question is that those three static properties, really are not part of parcel object, they are need only for price calculations, so which design pattern or wrapping(refactoring), is suggested?

Is having another interface as below necessary (& then wrap those static properties inside it?, even make static that class, because it is basically only some calculations), then how to connect it to IParcel?
Doing so, how best to implement CalculatePrice() in SpecialParcel & ExpressParcel classes?

public interface IPriceCalculator {
    decimal CalculatePrice();
}

Depending on the rest of your design, having a Parcel know how to calculate it's ownworth is perfectly acceptable behaviour.  That said, the method of calculation is now tied directly to the parcel class itself (what happens if your local government decides that ExpressParcels under a specific weight should be charged like a GeneralParcel - or worse, some other subclass?).

Also, the three dictionaries likely do not belong in the parcel class at all.  I can't imagine that is the only location that type of information is relevant.  Shipping/truck routing procedures also come to mind.  You should see if this is the case, or if the logic/data is indeed unique.

With that stated, here's some possibilities for refactoring in this situation.  You're on the right track with IPriceCalculator; you should modify it to take an IParcel object.

1) Implement IPriceCalculator, store as class (static) variable in IParcel subclasses.

public class GeneralParcel : IParcel {

    private static IPriceCalculator calc = new IPriceCalculator() {

                                               public decimal Calculate(IParcel parcel) {
                                                  return weight * .1;
                                               }
                                           }
}

This allows price calculators to be shared between classes, looked up from a static factory, etc.  However, it doesn't allow easy change of the calculator used for a specific instance of that class (such as, say, applying coupon rules).  And you still have to ask the parcel how much it is.

2)  Implement IPriceCalculator, store as instance variable in IParcel subclasses.

public class GeneralParcel : IParcel {

    private IPriceCalculator calc;

    public decimal Price {
       get {
          return calc.Calculate(this);
       }
    }

    public GeneralParcel(IPriceCalculator calc) {
       this.calc = calc;
    }

}

This allows prices calculators to be shared and swapped out as necessary.  The catch is that an individual parcel can't know all the possible restrictions to make the choice, and a calculator shouldn't know about any of the others.  Parcel still knows how much it is.

3) Implement IPriceCalculator, lookup from stored cache, store result in IParcel subclasses.

public class GeneralParcel : IParcel {

    private decimal price = -1;

    public decimal Price {
       get {
          if (price < 0) {
              price = CalculatorLookup.find(this).calculate(this);
          }
          return price;
        }
    }

    public GeneralParcel() {

    }
}

This of course caches out the information, preventing costly recalcs.  However, getting line-item charges may be dificult, and some of the Lookup rules might be interesting.

Finally, my favorite, and the most flexible;

4) Create (and implement) a series of *PriceCalculation*s, store IPriceCalculation in IParcel subclass instances.

public interface IPriceCalculation {
   public decimal Calculate {get;}

   public IPriceCalculation AddRule(IPriceCalculation calc, IParcel parcel);
}

impl

public struct BaseFeeCalculation {
   private readonly decimal price;

   public decimal Calculate {
      get {
         return price;
      }
   }

   private BaseFeeCalculation (IPriceCalculation calc) {
      if (calc == null) {
          price = 5;
      } else {
          price = 5 + calc.Calculate;
      }
   }

   public IPriceCalculation AddRule(IPriceCalculation calc, IParcel parcel) {
      return new BaseFeeCalculation(calc);
   }

}

I recommend that the calculations be structs, and immutable.  You're probably going to want to add other things to them, like the ability to report their individual price (for line items), and not just the total.  Additionally, while immutablility will help avoid circular references, you may want to add something to check for existing calculations (probably shouldn't have two BaseFees, after all).

Parcel basically stays the same; just add a property that returns the IPriceCalculation used.

When adding rules, try something along the lines of the following:

public struct CalculationBuilder {

   // You'll need to initialize this somehow, obviously.
   private static readonly IList<IPriceCalculation> calculations;

   private CalculationBuilder() {}

   private static IPriceCalculation Build(IParcel parcel) {
      IPriceCalculation calc;

      foreach(IPriceCalculation calcer : calculations) {
         calc = calc.AddRule(calcer, parcel);
      }

      return calc;
   }
 }

This works well for anything short of dependent rules (although can be adapted fairly easily for that).  Usually, the list of calculations can/should be initialized through a DI/Autowiring framework (like Spring).

Oh, and thank you for using decimal, and not double for your price calculation.

... that was a bit long, wasn't it.


Is this the Clojure way to web-scrape a book cover image? Is there a way to write this better or more Clojure way? Especially the last part with with-open and the let. Should I put the -> form into a separate function? Is this function to overloaded (to much stuff) or is this still ok?

 (defn scrape-book-cover [url image-filename]
   (let [out-stream (io/output-stream image-filename)
         image (-> url
                   (java.net.URL.)
                   (html/html-resource)
                   (html/select [:div.bookcoverXXL :> :div :> :img])
                   (first) (:attrs) (:src)
                   (->> (str "http:"))
                   (http-client/get {:as :byte-array})
                   (:body))]
     (with-open [out out-stream]
       (.write out image))))

@abuzittingillifirca made a good point in that you can leave the parentheses off of the -> forms that are just a single element.

html/html-resource is actually quite flexible; it can take a variety of different kinds of arguments, including strings, URLs, input streams, etc. Assuming the url argument to your function is a string, you can take out the java.net.URL. part and just feed url directly into html/html-resource. In fact, I would begin the -> part like this:

(-> (html/html-resource url)
    ...

So (html/html-resource url) is like the "first step" in the threading macro. Less steps makes your code appear simpler and easier to read, just as long as the steps aren't overly complicated.

For the (first) (:attrs) (:src) part, you might consider using get-in. This function comes in handy for navigating nested associative structures like vectors and maps -- it's perfect for something like enlive. You could re-word this part of your code as:

(get-in [0 :attrs :src])

It isn't much shorter than the original, but I think it reads better, and it "feels" shorter because you're only calling one function.

It's probably not very Clojurian to have a ->> macro among the steps to a -> macro. If you have a situation like this, where one of the steps to the -> requires you to put the argument last instead of second, you can do something like (#(str "http:" %)) -- create an anonymous function literal with the % wherever you want the argument to go, and then call it by putting parentheses around it.

So, here is how I would refactor your code:

(defn scrape-book-cover [url image-filename]
  (let [out-stream (io/output-stream image-filename)
        image (-> (html/html-resource url)
                  (html/select [:div.bookcoverXXL :> :div :> :img])
                  (get-in [0 :attrs :src])
                  (#(str "http:" %))
                  (http-client/get {:as :byte-array})
                  :body)]
    (with-open [out out-stream]
      (.write out image))))

EDIT: To answer your question: No, I don't think this function is overloaded. When you simplify like I did above, it doesn't seem too long or complicated.

EDIT 2: Of course, there's nothing stopping you from pulling out the -> part and making it a separate function. It might make your code easier to read. You could call the function something like imgurl->bytes.

(defn imgurl->bytes [url]
  (-> (html/html-resource url)
      (html/select [:div.bookcoverXXL :> :div :> :img])
      (get-in [0 :attrs :src])
      (#(str "http:" %))
      (http-client/get {:as :byte-array})
      :body))

(defn scrape-book-cover [url image-filename]
  (let [out-stream (io/output-stream image-filename)
        image (imgurl->bytes url)]
    (with-open [out out-stream]
      (.write out image))))

Come to think of it, I like this better. If you look at a lot of Clojure code on github, you will see a lot of things broken up into simple helper functions. I suppose it is generally idiomatic in Clojure to break things up in helper functions.

EDIT 3 (5/16/14): Someone just upvoted my answer, which made me remember this code and have a quick re-visit. You could actually further refactor this version of the scrape-book-cover function by taking out the let statement:

(defn scrape-book-cover [url image-filename]
  (with-open [out (io/output-stream image-filename)]
    (.write out (imgurl->bytes url))))


Diffing two lists of business objects dataListOne and dataListTwo will be full of BO objects (around 4000 - 5000).

Lets say dataListOne comes with objects with business keys: "one,two,three,one"
and dataListTwo has: "three,four,five".

At the end I must have:

insertList: "one,two"
updateList: "three"
deactivateList: "four,five"

There may be 2 BO's with same business key but different properties but I do not care about that.. So once I process a BO which has a businessKey "one" in dataListOne, if there comes another, I will not care about it..

What kind of approach should I take?

/*
Base Business Objects are considered to be same if they have the same businessKey !
 */
public class DataDiffContainer<BO extends BaseBusinessObject> {

    private List<BO> insertList = new ArrayList<BO>();
    private List<BO> updateList = new ArrayList<BO>();
    private List<BO> deactivateList = new ArrayList<BO>();

    private Set<String> handledString = new HashSet<String>();

    public void performDiff(List<BO> dataListOne, List<BO> dataListTwo) {
        for (BO dataOne : dataListOne) {
            if (handledString.contains(dataOne.getBusinessKey())) {
                // See the comment above class declaration
                continue;
            }
            boolean found = false;
            for (BO dataTwo : dataListTwo) {
                if (dataTwo.equals(dataOne)) {
                    updateList.add(dataOne);
                    dataListTwo.remove(dataTwo);
                    // we already know what to do with this object,
                    // so we may as well remove from the List that we need to check..
                    found = true;
                    break;
                }
            }
            if (!found) {
                insertList.add(dataOne);
            }
            handledString.add(dataOne.getBusinessKey());
        }

        // What is remaining in dataListTwo data should be dectivated:
        for (BO bo : dataListTwo) {
            if (bo.isActive())
                deactivateList.add(bo);
        }
    }
}

I would go down the simple road:

1) Converting both lists to sets

2) Use set operations:

    Set<Integer> update = new HashSet<>(list1);
    Set<Integer> insert = new HashSet<>(list1);
    Set<Integer> delete = new HashSet<>(list2);
    update.retainAll(delete);
    insert.removeAll(delete);
    delete.removeAll(update);

Done.

Even if it is not the fastest solution, it should not be too bad for only 5k objects. It is simple, readable and not errorprone.


Singly Linked List implementation in Java Here's my implementation of a Singly Linked List. Would appreciate a review.

public class SList<T> {

    private Node head;
    private int size;

    private class Node<T>{
       public  Node(){
            item = null;
            next = null;
        }
        public  Node(T item){
            this.item = item;
            this.next = null;
        }
        private T item;
        private Node next;
    }//end of node

    //constructor
    public SList(){
        head = null;
        size = 0;
    }
    public void add(T item){
        Node p = new Node(item);
        if(head == null){
            head = p;
            size++;
            return;
        }
        p.next = head;
        head = p;
        size++;
    }
    public boolean isEmpty(){
        return size == 0;
    }

    public int getSize(){
        return size;
    }
    public void remove(T item){
        Node current = head;
        while(current.next != item){

            if(current.next == null) {
                System.out.println("Item doesn't exist in the list:");
                return;
            }
            current = current.next;
        }
        current.next = current.next.next;
        size--;
    }

    public void insertAfter(T item1, T item2){
        Node current = head;
        Node p = new Node(item2);
        do{
            if(current == null) {
                System.out.println("Item doesn't exist in the list:");
                return;
            }
            current = current.next;
        }while(current.item != item1);
        Node q = current.next;
        current.next = p;
        p.next = q;
        size++;

    }

    public void insertBefore(T item1, T item2){
        Node current = head;
        Node p = new Node(item2);
        do{
            if(current.next == null) {
                System.out.println("Item doesn't exist in the list:");
                return;
            }
            current = current.next;
        }while(current.next.item != item1);
        Node q = current.next;
        current.next = p;
        p.next = q;
        size++;
    }
}

General

Default Values

Java uses default values for all its fields. Meaning :

public  Node(){
    item = null;
    next = null;
}

Does nothing except take up space for you. I would remove it, and the other lines / constructors like it. Some consider it bad practice to rely on the default values, you can then explicitly state the default value of a field on the same row as you are declaring it.

private Node head = null;

Reference

Inconsistency

You are inconsistent at spacing your code. I also recommend having fields at the right below the class declaration. I should not have to look for the fields, I want to find them instantly.

Bug in remove, insertBefore

If head is null, then current.next will throw a NullPointerException. I suggest guarding against null for head first in the method.

if (head == null) {
    System.out.println ("Item doesn't exist in the list.");
    return;
}

Nitpicks

public void add(T item)

You always execute the size++; why have it in two different places? And not directly under Node p = new Node (item);

public boolean isEmpty()

Looks perfect, why can't every method turn out to be like this? One option is to match against head == null as well, however in terms micro-optimization, it is slower.

public int getSize()

Nothing needs to be said, a simple getter.

public void remove(T item)

Instead of letting it handle the error printing, let remove return a boolean, true if removed, false if not, making it more flexible to use in future applications.


Firefox link-highlighting extension This code is executed very often, so I'm wondering if it's already ideal or if it's possible to optimize it further.

var highlightLinks = function(e)
{
  var anchors = e.target.getElementsByTagName("a");
  let file = Components.classes["@mozilla.org/file/directory_service;1"]
                  .getService(Components.interfaces.nsIProperties)
                  .get("ProfD", Components.interfaces.nsIFile);
  file.append("test.sqlite");

  var storageService = Components.classes["@mozilla.org/storage/service;1"]
          .getService(Components.interfaces.mozIStorageService);
  var conn = storageService.openDatabase(file);

  var statement = conn.createStatement("select * from links where url=?1");
  for (var i = 0; i < anchors.length; i++) {
    statement.bindStringParameter(0, anchors[i].href);
    statement.executeAsync({
      anchorIndex: i,
      handleResult: function(aResultSet) {
        for (let row = aResultSet.getNextRow();
             row;
             row = aResultSet.getNextRow()) {

          let value = row.getResultByName("url");
          if (value == anchors[this.anchorIndex]) {
           anchors[this.anchorIndex].innerHTML += "+";
          }
        }
      },

      handleError: function(aError) {
        print("Error: " + aError.message);
      },

      handleCompletion: function(aReason) {
        if (aReason != Components.interfaces.mozIStorageStatementCallback.REASON_FINISHED)
          print("Query canceled or aborted!");
      }
    });
  }
  statement.finalize();
  conn.close();
}

window.addEventListener("DOMContentLoaded", highlightLinks, false);

I believe you could improve your code by using prototypes like this:

function handler(anchorIndex, anchor) {
    this.anchorIndex = anchorIndex;
    this.anchor = anchor;
}

handler.prototype = {
    handleResult: function(aResultSet) {
        for (let row = aResultSet.getNextRow();
        row;
        row = aResultSet.getNextRow()) {

            let value = row.getResultByName("url");
            if (value == anchor) {
                anchor.innerHTML += "+";
            }
        }
    },

    handleError: function(aError) {
        print("Error: " + aError.message);
    },

    handleCompletion: function(aReason) {
        if (aReason != Components.interfaces.mozIStorageStatementCallback.REASON_FINISHED) {
            print("Query canceled or aborted!");
        }
    }
};

Instead of creating a new object, which has to create new instances of the same functions, you create those functions once. This will improve performance and reduce memory usage across the board verified using http://jsperf.com/new-function-vs-object

Your for loop would be drastically reduced to look like

for (var i = 0; i < anchors.length; i++) {
    statement.bindStringParameter(0, anchors[i].href);
    statement.executeAsync(new handler(i, anchors[i]));
}

EDIT:
You'll find this method of creating new instances of objects is faster in IE7-9 by ~3x, and roughly 2x as fast in FF5. My dev Chrome shows strange results in that the original method is 20% as fast as IE, yet shows ~155x improvement (10x faster than all other browsers) when using the prototype way.


Check if 2 rectangles fit in another one, given their length and height I was very intrigued with the manner to solve this problem. The program needs to know if 2 rectangles fit in another one, considering the lines of the 2 rectangles are always parallel to the other one's.

I know I could just put everything in a single if, but is that more readable? I want to know a good approach for this situation, using only conditions.

NOTE: x and y represent the first rectangle's width and height (the one that will fit the other 2) and l1, h1, l2, and h2 represent the other 2 rectangles' width and height.

#include <stdio.h>

int main(void)
{
    int x, y, l1, h1, l2, h2;

    scanf("%d %d %d %d %d %d", &x, &y, &l1, &h1, &l2, &h2);

    if ((l1 + l2 <= x && h1 <= y && h2 <= y) ||
    (h1 + h2 <= x && l1 <= y && l2 <= y))
    {
        puts("They fit.");
    }
    else if ((l1 <= x && h2 <= x && h1 + l2 <= y) ||
    (h1 <= x && l2 <= x && l1 + h2 <= y))
    {
        puts("They fit.");
    }
    else if ((l1 <= y && h2 <= y && h1 + l2 <= x) ||
    (h1 <= y && l2 <= y && l1 + h2 <= x))
    {
        puts("They fit.");
    }
    else if ((l1 + l2 <= y && h1 <= x && h2 <= x) ||
    (h1 + h2 <= y && l1 <= x && l2 <= x))
    {
        puts("They fit.");
    }
    else
    {
        puts("They don't fit.");
    }
    return 0;
}

Any two rectangles can fit into a third rectangle (without overlapping with each other) only under 2 cases:

As mentioned in the question, consider:

x,y  -> length and height of first rectangle into which below two rectangles must fit.
l1,h1 -> length/width and height of second rectangle.
l2,h2 -> length and height of third rectangle.

Note: You could use structures or 2-dimensional arrays for better readability.

Case 1:

When x is greater than or equal to l1+l2
Here the height y must be greater than or equal to maximum of h1 and h2 for the rectangles to fit in, else it doesn't fit in.

Note: x >= l1+12 can be written as ((x >= l1) && (x-l1 >= l2)). This is to avoid the overflow while adding integers.

Case 2:

When y is greater than or equal to h1+h2
Here the length x must be greater than or equal to maximum of l1 and l2 for the rectangles to fit in, else it doesn't fit in.

Note: similarly, even here y >= h1+h2 can be written as (y >= h1) && (y-h1 >= h2). This is to avoid the overflow while adding integers.

Now, keeping the above 2 cases in mind, a simple function can be constructed this way:

int fit( int x, int y, int l1, int h1, int l2, int h2)
{
    //return 1 for fit and 0 if does not fit

    //case 1 or case 2
    if( ( ((x >= l1) && (x-l1 >= l2)) && y >= max(h1,h2)) ||
        ( ((y >= h1) && (y-h1 >= h2)) && x >= max(l1,l2)) )
    {
            return 1;
    }

    return 0;
}

Note: here max() is a function which returns the larger value among the two sent arguments.

int max(int a, int b)
{
    if(a>b)
    {
        return a;
    }
    return b;
}

Now, the 2 rectangles can be sent in 4 different ways to check if they can fit in the rectangle of dimensions x and y:

//sending as entered by the user
fit( x, y, l1, h1, l2, h2)

//interchanging length and height of anyone rectangle
fit( x, y, h1, l1, l2, h2)

//interchanging length and height of other rectangle
fit( x, y, l1, h1, h2, l2)

//interchanging lengths and heights of both rectangles
fit( x, y, h1, l1, h2, l2)

Now, you can easily know whether the rectangles fit or does not fit by checking for the return value of fit() function.
If return value of anyone of the above four function calls is 1 then the rectangles fit in!
You could use the main() function as:

int main(void)
{
    int x, y, l1, h1, l2, h2;

    scanf("%d %d %d %d %d %d", &x, &y, &l1, &h1, &l2, &h2);

    //tests for fit
    if(fit( x, y, l1, h1, l2, h2) ||
       fit( x, y, h1, l1, l2, h2) ||
       fit( x, y, l1, h1, h2, l2) ||
       fit( x, y, h1, l1, h2, l2))
    {
        printf("fits");
    }
    else
    {
        printf("doesn't fit");
    }

}

The above if block functions similarly as an else-if ladder because, || stops evaluation on the first true (i.e, function returns 1).
So, if any of the function returns a 1, then then "fits" gets printed out and program terminates
In case, if none of the function calls return 1 the "doesn't fit" gets printed out and program terminates

Altogether your code would be:

#include <stdio.h>

int max(int a, int b)
{
    if(a>b)
    {
        return a;
    }
    return b;
}

int fit( int x, int y, int l1, int h1, int l2, int h2)
{
    //return 1 for fit and 0 if does not fit

    //case 1 or case 2
    if( ( ((x >= l1) && (x-l1 >= l2)) && y >= max(h1,h2)) ||
        ( ((y >= h1) && (y-h1 >= h2)) && x >= max(l1,l2)) )
    {
            return 1;
    }

    return 0;
}

int main(void)
{
    int x, y, l1, h1, l2, h2;

    scanf("%d %d %d %d %d %d", &x, &y, &l1, &h1, &l2, &h2);

    //tests for fit
    if( fit( x, y, l1, h1, l2, h2) ||
        fit( x, y, h1, l1, l2, h2) ||
        fit( x, y, l1, h1, h2, l2) ||
        fit( x, y, h1, l1, h2, l2))
    {
        printf("fits");
    }
    else
    {
        printf("doesn't fit");
    }

}


Count the accumulated rainfall in a 2D mountain range Challenge:

  Count how much rain will be collected in the valleys between the peaks
  of a mountain range in a 2D world.

  The mountain range is defined by a single array of positive ints,
  where each element defines the height a peak. IE the array [1,2,1,2]
  would create the 2D slice:

 . m . m
 m m m m

m = mountain
. = open space

  As the rain falls, it will fill up to the height of its surrounding
  peaks, then overflow. If the rain reaches the edge of the grid, it
  disappears. In this case the amount of water collected will be 1:

 . m r m
 m m m m
 r = rain

  More examples:

  Input: [2, 4, 2, 2, 5, 1, 2, 2, 1, 3]

  Output: 10

  2D visualization:

. . . . m . . . . .
. m r r m . . . . .
. m r r m r r r r m
m m m m m r m m r m
m m m m m m m m m m

  Input: [1, 2, 3, 2, 1]

  Output: 0

  2D visualization:

. . m . .
. m m m .
m m m m m

Constraints:

The input array will have size at least 1
Each element of the array will be greater than or equal to 1
Must be in Java 7

My Code:

//Time Complexity: worst case O(n^2)?
//Space Complexity: O(1)
public class Solution
    public static int solution(int[] heights) {
       int fillCount = 0, minHeight = heights[0];

       for(int i = 1; i<heights.length; i++){
           if(heights[i] < minHeight){
               minHeight = heights[i];
           }
           else{
               minHeight=heights[i];
               int fillHeight = findFillHeight(heights, i);
               if(fillHeight == -1) continue;
               fillCount+= fillIn(heights, fillHeight, i);
           }
       }

       return fillCount;
    }

    static int findFillHeight(int[] heights, int rightEdgePosition){
        int maxHeight = -1, j=0;

        for(int i=rightEdgePosition-1; i-1 >=0 && heights[i] < heights[rightEdgePosition]; i--, j=i){
            if(heights[i] >= heights[rightEdgePosition]){
                return heights[rightEdgePosition];
            }
            if(heights[i] > maxHeight){
                maxHeight = heights[i];
            }
        }

        if(heights[j] >= heights[rightEdgePosition]){
            maxHeight = heights[rightEdgePosition];
        }

        return maxHeight;
    }

    static int fillIn(int[] heights, int fillInHeight, int startHeightPosition){
        int fillCount = 0;
        for(int i=startHeightPosition-1; heights[i] < fillInHeight; i--){
            fillCount+= fillInHeight-heights[i];
            heights[i]=fillInHeight;
        }
        return fillCount;
    }
}

The idea is to search the array from left to right, looking for rises in elevation. When we find one, we backtrack looking for how high up we can fill the valley, and then redo the backtrack, this time actually filling it (this prevents "double filling" in future elevation rises).

The worst case arises when we get a large elevation on element 1 and elements 2 through n are rising from 1 to the value of element 1. IE [10, 1, 2, 3.... 10]. In this case we do the maximum number of backtracks. If I have my math right, our time complexity would be

$$n + \sum_{i=0}^{n} {i}\ = n+\frac{1}{2}n(n+1) \implies O(n^2)$$

So quadratic time obviously a red flag, but I'm not sure I can avoid it since I'm dealing with 2D arrays. Other than that, the big ugly part is the j variable in findFillHeight(). The point of it is to check that the left edge we end the loop on is properly checked. The last notable issue is that much of this code would be very risky if run on its own because the assumption is that all inputs are valid. This was due to time constraints though, so I don't consider it an issue.

It's possible to do it in \$O(n)\$ using \$O(n)\$ additional memory. For that we should find the amount of rain collected at each index in \$O(1)\$. It can be calculated from the heights of the highest mountain to the right and to the left:

$$\tt{rainfall[i]} = \max(0, \min(\tt{leftHighest}, \tt{rightHighest}) - \tt{height[i]})$$

We can have the height to the left updated on the fly in the same loop, but we have to precalculate the heights to the right in a separate loop.

Here is the sample code:

import java.util.Arrays;

public class Solution {
    public static int getCollectedRainfall(int[] heights) {
        int[] highestToTheRight = Arrays.copyOf(heights, heights.length + 1);
        for (int i = heights.length - 1; i >= 0; --i) {
            highestToTheRight[i] = Math.max(highestToTheRight[i], highestToTheRight[i + 1]);
        }

        int collectedRainfall = 0;
        int highestToTheLeft = 0;
        for (int i = 0; i < heights.length; ++i) {
            collectedRainfall +=
                Math.max(0, Math.min(highestToTheLeft, highestToTheRight[i]) - heights[i]);
            highestToTheLeft = Math.max(highestToTheLeft, heights[i]);
        }

        return collectedRainfall;
    }
}


Intersection between arrays I'm a Python beginner (more used to code in R) and I'd like to optimize a function.

2 arrays ares filled by integers from 0 to 100. A number can't appear twice in a row and they're stored in an ascending order.

Array1: nrow = 100 000; ncol = 5
Array2: nrow = 50 000; ncol = 5

For each row of Array1 and each row of Array2 I need to count the number of similar values and store this result in a 3rd array.

Array3: nrow = 100 000; ncol = 50 000

Here is the current function, with a smaller array2 (50 rows instead of 50 000)

array1= np.random.randint(0,100,(100000,5))
array2 = np.random.randint(0,100,(50,5))

def Intersection(array1, array2):
    Intersection = np.empty([ array1.shape[0]  , array2.shape[0] ], dtype=int8)
    for i in range(0, array1.shape[0]):
        for j in range(0, array2.shape[0]):
            Intersection[i,j] = len( set(array1[i,]).intersection(array2[j,]) )
    return Intersection

import time
start = time.time()
Intersection(array1,array2)
end = time.time()
print end - start

23.46 sec. So it should take hours if array2 has 50 000 rows.

How can I optimize this function by keeping it simple to understand?

The main optimization I can think of is precomputing the sets. If array1 has shape [m,n] and array2 is [q,p], the original code is creating a set 2*m*q times. You can get this down to m+q.

In addition, you could follow the Python style guide in terms of naming conventions and whitespace, which will help working together with other Python programmers. Also it's nice to loop over the data structure itself instead of over a range of indices.

This would result in for example:

import numpy as np

def intersection(array1, array2):
    intersection = np.empty([array1.shape[0], array2.shape[0]], dtype=np.int8)
    array2_sets = map(set, array2)
    for i,row1 in enumerate(array1):
        set1 = set(row1)
        for j, set2 in enumerate(array2_sets):
            intersection[i,j] = len(set1.intersection(set2))
    return intersection

If you can forget about "keeping it simple to understand" you could also convert your data to sparse matrices and take the matrix product, e.g.:

from scipy.sparse import csr_matrix

def intersection2(A, B, sparse=True):
    """The rows of A and B must contain unique, relatively
    small integers
    """
    assert min(A.shape[1], B.shape[1]) < 256  # to fit inside uint8
    z = max(A.max(), B.max()) + 1

    def sparsify(A):
        m, n = A.shape
        indptr = np.arange(0, m*n+1, n)
        data = np.ones(m*n, dtype=np.uint8)
        return csr_matrix((data, A.ravel(), indptr), shape=(m,z))

    intersection = sparsify(A) * sparsify(B).T
    return intersection if sparse else intersection.todense()


Print length of longest sequence of consecutive numbers Problem H (Longest Natural Successors):

  Two consecutive integers are natural successors if the second is the
  successor of the first in the sequence of natural numbers (1 and 2 are
  natural successors). Write a program that reads a number N followed by
  N integers, and then prints the length of the longest sequence of
  consecutive natural successors.

  Example:

Input  7 2 3 5 6 7 9 10
Output 3

This is my code so far:

  import java.util.Scanner;

    public class Conse {

  public static void main(String[] args) {
    // TODO Auto-generated method stub
     Scanner scan = new Scanner(System.in);
     int x=scan.nextInt();
     int[] array= new int[x];
     for(int i=0;i<array.length;i++)
         array[i]=scan.nextInt();
     System.out.println(array(array));
      }
public static int array(int[] array){
    int count=0,temp=0;
    for(int i=0;i<array.length;i++){
        count=0;
        for(int j=i,k=i+1;j<array.length-1;j++,k++)
            if(array[j]-array[k]==1)
                count++;
            else{if(temp<count)
                temp=count;
            break;}
        }
    return temp+1;
 }

  }

Using static for the consecutive() function is appropriate.  However, the omission of braces, the inconsistent indentation, and lack of spaces between operators is just as bad as before.

Your function crashes on arrays of length 0 or 1.

The variable names f and counter could be improved.  I think that a different looping structure would be clearer as well.

public static int consecutive(int[] array) {
    if (array.length <= 1) {
        return array.length;
    }

    int maxRun = 0;
    for (int i = 1; i < array.length; i++) {
        int thisRun = 1;
        while (i < array.length && array[i - 1] + 1 == array[i]) {
            thisRun++;
            i++;
        }
        if (maxRun < thisRun) {
            maxRun = thisRun;
        }
    }
    return maxRun;
}


Get all remaining days of the week What this function does it's to return an array of remaining dates that are e.g. Mondays. Is this really the best way to do that?

function range(start, count) {
    return Array.apply(0, Array(count))
                .map(function (element, index) {
                         return index + start;
                     });
}

function GetAllRemainingDaysOfTheWeek(day)
{
    var today = new Date();
    var last = new Date(today.getFullYear(), 11, 31);
    today = new Date(today.getFullYear(), today.getMonth(), today.getDate());
    var diffInDays = (last.getTime() - today.getTime()) / (1000 * 3600 * 24);

    return range(1, diffInDays+1).map(function(x) {
       return new Date(today.getFullYear(), today.getMonth(), today.getDay()+x);
    }).filter(function(x){
       if(x.getDay() == 1) return true;
    });
}

Firstly, your code is buggy. You take a day argument, but you never use it for anything. You always return mondays.

But worse, you're incrementing dates like so:

new Date(today.getFullYear(), today.getMonth(), today.getDay()+x);

Spot the error? You're passing getDay()+x as the date argument. But getDay() returns the weekday number (0-6) - not the day-in-month.

So today, Thursday the 5th of November 20151, you're in effect saying:

new Date(2015, 10, 4 + x);

The year's correct, and the month's correct (January is zero), but the date is 1 day off. Thursday is day #4 in the 0-6 weekday scheme - but really, today's the 5th of November.

So if you were to run this on the, say, the 30th of November, you'd actually be using the 1st of November as your start date, since the 30th's a Monday. If you run it on a Sunday, getDay will return zero, meaning your start date will be the last of the previous month. And so on.

I also have to wonder why you've created a function, which builds an array just to hold a list of incrementing numbers, which you then map to dates. The simpler way of looping through incrementing numbers is a plain old for-loop.

Anyway, while calendars are complex, there's one thing that thankfully doesn't change: There are 7 days in a week. So you're creating 7 times more dates than you need, and filtering most of them away. You could consider simply skipping 7 dates at a time from the first occurrence.

Edit: The only thing in the above paragraph that holds true is that calendars are complex. As RobH points out in the comments, some pacific nations have been known to jump across the dateline. Presumably just to mess with programmers the world over!

Thirdly, don't mix calendar dates and raw time calculations like your division by milliseconds in a day. Switching back and forth between daylight savings time (aka winter/summer time) can cause all sorts of nonsense when mixing raw time and dates. Some calendar days are 25 hours long, some are 23. And since a date constructed from only year, month, and date sets its time to midnight, you risk skipping over a date, or repeating the same date twice.

Anyway, here's a simple solution that adds 1 day at a time until the year rolls over, storing dates that match what you're looking for:

function remainingDays(weekday) {
  var current = new Date,
      year = current.getFullYear(),
      dates = [];

  // a simple helper function
  function nextDay(date) {
    return new Date(date.getFullYear(), date.getMonth(), date.getDate() + 1);
  }

  // as long we're in the same year, keep adding 1 day,
  // and store the ones that match the weekday we're looking for
  while(current.getFullYear() === year) {
    if(current.getDay() === weekday) {
      dates.push(current);
    }
    current = nextDay(current);
  }

  return dates;
}

No need to build ranges, map, and filter them. Just a while loop and push.

Incidentally, if you want to learn more about why programming calendar and time things are just hideously complex, check out this video on youtube

1) Happy Guy Fawkes day everyone


Python program to get the first word Given a string the task it to find its first word with some rules:

The string can have points and commas
A word can start with a letter, a point or space
One word can contain one apostrophe and it stills being a valid one

For example:

assert first_word("Hello world") == "Hello"
assert first_word(" a word ") == "a"
assert first_word("don't touch it") == "don't"
assert first_word("greetings, friends") == "greetings"
assert first_word("... and so on ...") == "and"
assert first_word("hi") == "hi"
assert first_word("Hello.world") == "Hello"

The code:

def first_word(text: str) -> str:
    """
        returns the first word in a given text.
    """
    text = re.sub("[^A-Za-z'\s.]",'',text)
    words = text.split()
    for word in words:
        for i in range(len(word)):
            if word[i].isalpha() or word[i] == "'":
                if i == len(word) - 1:
                    if word.find('.') != -1:
                        return word.split('.')[0]
                    else:
                        return word

How could we improve it?

You could make the code better (and shorter) by using regex to split any delimiters that occur in the string, for example, in Hello.world, the string (list form) would then be like ['', 'Hello', ''] (after splitting the first word from delimiters) and then you can access the first word from index [1] (always). Like this,

import re
def first_word(s):
    return re.split(r"(\b[\w']+\b)(?:.+|$)", s)[1]

Here are some tests:

tests = [
"Hello world",
"a word",
"don't touch it",
"greetings, friends",
"... and so on ...",
"hi",
"Hello.world",
"Hello.world blah"]

for test in tests:
    assert first_word("Hello world") == "Hello"
    assert first_word(" a word ") == "a"
    assert first_word("don't touch it") == "don't"
    assert first_word("greetings, friends") == "greetings"
    assert first_word("... and so on ...") == "and"
    assert first_word("hi") == "hi"
    assert first_word("Hello.world") == "Hello"
    assert first_word("Hello.world blah") == "Hello"
    print('{}'.format(first_word(test)))

(\b[\w']+\b)(?:.+|$) is used above, where (\b[\w']+\b) calls the first word of the string (in list form). \b allows you to perform a "whole words only" search using a regular expression in the form of \b"word"\b. Note that using [\w'] (instead of [\w+]) leaves the apostrophe in don't. For (?:.+|$), you can take a look here.

Here are the expected outputs:

Hello
a
don't
greetings
and
hi
Hello
Hello

After timing it -

%timeit first_word(test)
>>> 1.54 µs ± 17.8 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

NOTE - A delimiter is a sequence of one or more characters used to specify the boundary between separate, independent regions in plain text or other data streams. An example of a delimiter is the comma character, which acts as a field delimiter in a sequence of comma-separated values.

Hope this helps!


C# enumerable property like .Any that accounts for nulls Brief Description:

A generic extension method that performs the same function as IEnumerable.Any(), but does not throw an error when passed a parameter with a null value.

Explanation:

Often times I will come across code that performs two steps on a nullable collection to make sure it contains elements. The first step checks to make sure the collection is not null, then the second step looks to see that there are greater than zero elements.

For example, to make sure a List<T> meets these qualifications before working through each element, code might look like this:

List<int> intList = new List<int>();

if (
    intList != null &&
    intList.Any()
    )
{
    foreach (int intElement in intList)
    {
        // logic here
. . .

To reiterate, just using .Any() above would be insufficient for the situation where intList hasn't yet been initialized.

Because this is a common occurrence in the code I maintain, I've come up with a generic extension method that performs the same function as the above, in one method:

static class CLR
{
    public static bool Populated<TSource>(this IEnumerable<TSource> source)
    {
        return source != null && source.Any();
    }
. . .

... which renders the first block a little simpler:

if (intList.Populated())
{
    foreach (int intElement in intList)
    {
        // logic here
. . .

Are there suggestions for why this should not be implemented, or why it should be implemented in a different form?

My point of view is different depending on whether the the code that's returning null enumerables is a 3rd-party API or your own code.

3rd-Party

If the code that's returning null enumerables is a 3rd-party API, then I only have a few minor points to mention:

CLR is a very bad and misleading name for a class. In the .NET world CLR stands for Common Language Runtime, the part that's responsible for JIT-compiling CIL code (which all .net languages compile to), a bit like the JVM does for byte code / compiled Java.
I would have named Populated with a little Is prefix to read like IsPopulated, because I like Boolean functions/properties to read that way. But that's personal preference.

The implementation itself is nice & clean, nothing to say about it (other than the C# 6 trick in t3chb0t's answer); I like that you named the this parameter source, it lines up with other framework-provided IEnumerable<T> extension methods, e.g. LINQ. TSource seems a bit verbose when T would have been sufficient, but I like descriptive generic type parameters so I'd leave it as is.

In the sample code, you have this:

if (
    intList != null &&
    intList.Any()
    )

That looks terrible; an if condition should fit one single line:

if (intList != null && intList.Any())

Very long conditions that actually need to be multiple lines, should be extracted out of the if statement and into their own function with a meaningful name that captures the meaning of all these conditions:

if (IsValid(intList))

You get the idea; that's usually more commonly seen with LINQ queries and predicates though, but the concept is the same: if it's too much clutter to be readable, move it elsewhere and abstract it behind a meaningful identifier.

Your own

On the other hand, if the code that's returning null enumerables is under your control, there's a bigger issue.

Per Framework Design Guidelines 2nd Edition, p.256:

  DO NOT return null values from collection properties or from methods returning collections. Return an empty collection or an empty array instead.

See this SO Q&A for more information about whether it's better to return null or an empty collection.

Instead of introducing an extension method to work around a serious design issue in your code base, you should be hunting down instances of IEnumerable<T>-returning methods and properties in that code base, and fixing them so that they return an empty enumerable instead of a null value:

return Enumerable.Empty<Foo>();

Then all you need to do is get rid of redundant but inoffensive null-checks everywhere in the code base, and new code wouldn't need any such null-checks.


PHP class for scanning filesystem I have a modelScan.php file, as you can see,

I have codes that repeated within the function/method. How can I make this code more readable?

<?php
/**
 * Class for Scanning files
 */
class modelScan
{
    /**
     * Get the total size of file in the directory
     * @param string $sPath and integer $iTotalCount
     */
    public function getBytesTotal($sPath, $iTotalCount)
    {
        $sPath = realpath($sPath);
        if($sPath!==false && $sPath!='' && file_exists($sPath)){
            foreach(new RecursiveIteratorIterator(new RecursiveDirectoryIterator($sPath, FilesystemIterator::SKIP_DOTS)) as $oObject){
                $iTotalCount += $oObject->getSize();
            }
        }
        return $iTotalCount;
    }

    /**
     * Get the total files in the directory
     * @param string $sPath and integer $iTotalCount
     */
    public function getFilesTotal($sPath, $iTotalCount)
    {
        $ite = new RecursiveDirectoryIterator($sPath);
        foreach (new RecursiveIteratorIterator($ite) as $filename=>$cur) {
            if(is_file($cur)){
                $iTotalCount++;
            }
        }
        return $iTotalCount;
    }

    /**
     * Get the total Class in the directory
     * @param string $sPath and integer $iTotalCount
     */
    public function getClassTotal($sPath, $iTotalCount)
    {
        $ite = new RecursiveDirectoryIterator($sPath);
        foreach (new RecursiveIteratorIterator($ite) as $filename=>$cur) {
            if(is_file($cur)){
                $fileToString = file_get_contents($cur);
                $token = token_get_all($fileToString);
                $tokenCount = count($token);

                //Class Count
                $sPathInfo = pathinfo($cur);
                if ($sPathInfo['extension'] === 'php') {
                    for ($i = 2; $i < $tokenCount; $i++) {
                        if ($token[$i-2][0] === T_CLASS && $token[$i-1][0] === T_WHITESPACE && $token[$i][0] === T_STRING ) {
                        $iTotalCount++;
                        }
                    }
                } else {
                    error_reporting(E_ALL & ~E_NOTICE);
                }
            }
        }
        return $iTotalCount;
    }

    /**
     * Get the total Method in the directory
     * @param string $sPath and integer $iTotalCount
     */
    public function getMethodTotal($sPath, $iTotalCount)
    {
        $ite = new RecursiveDirectoryIterator($sPath);
        foreach (new RecursiveIteratorIterator($ite) as $filename=>$cur) {
            if(is_file($cur)){
                $fileToString = file_get_contents($cur);
                $token = token_get_all($fileToString);
                $tokenCount = count($token);

                //Method Count
                $sPathInfo = pathinfo($cur);
                if ($sPathInfo['extension'] === 'php') {
                    for ($i = 2; $i < $tokenCount; $i++) {
                        if ($token[$i-2][0] === T_FUNCTION) {
                                $iTotalCount++;
                        }
                    }
                } else {
                    error_reporting(E_ALL & ~E_NOTICE);
                }
            }
        }
        return $iTotalCount;
    }

}

Welcome to Code Review. Your code is understandable, so we can, more or less, figure out what it does, but it is always nice for reviewers to hear you say what you use a piece of code for, what its purpose is, what the code itself specifically does, and to have some usage examples. This also helps you to reflect on the choices you made.

I couldn't get your getClassTotal() and getMethodTotal() to work. They assume that all the files are PHP files, and in my test directory they are not. You should have checked the PHP file extension before tokenizing the file content.

From what I can see this looks like a class that wraps four independent function. That only doesn't justify the existence of this class. Is there nothing common to these functions? I think we can agree that the RecursiveDirectoryIterator and RecursiveIteratorIterator are present in all four, so I made that part of the class.

There are also some simplifications I added in the file and bytes counter.

Using the build-in PHP tokenizer was a good idea, but clearly the way that classes and methods are counted is very similar. So I created a new method which will count either. I don't see the point of checking for a class name, so I left that bit out.

In your code a counter is the second argument to all four method. I fail to see why this is a good thing. I dropped it.

This gives me the following code:

<?php

class DirectoryScanner
{
    private $files;

    public function __construct($path)
    {
        $dirIterator = new RecursiveDirectoryIterator($path, FilesystemIterator::SKIP_DOTS);
        $this->files = new RecursiveIteratorIterator($dirIterator);
    }

    public function getFileCount()
    {
        return iterator_count($this->files);
    }

    public function getBytesUsed()
    {
        $bytes = 0;
        foreach ($this->files as $file) {
            $bytes += $file->getSize();
        }
        return $bytes;
    }

    private function getPhpTokenCount($token)
    {
        $counter = 0;
        foreach ($this->files as $file) {
            if ($file->getExtension() == 'php') {
                $contents = file_get_contents($file);
                $tokens   = token_get_all($contents);
                $values   = array_count_values(array_column($tokens, 0));
                $counter += isset($values[$token]) ? $values[$token] : 0;
            }
        }
        return $counter;
    }

    public function getPhpClassCount()
    {
        return $this->getPhpTokenCount(T_CLASS);
    }

    public function getPhpFunctionCount()
    {
        return $this->getPhpTokenCount(T_FUNCTION);
    }
}

?>

I left out all the comments to save some space. This code can be tested like this:

$path = $_SERVER['DOCUMENT_ROOT'];
$test = new DirectoryScanner($path);

echo 'getFileCount: ' . $test->getFileCount() . '
';
echo 'getBytesUsed: ' . $test->getBytesUsed() . '
';
echo 'getPhpClassCount: ' . $test->getPhpClassCount() . '
';
echo 'getPhpFunctionCount: ' . $test->getPhpFunctionCount() . '
';

In my code I take advantage of what a class is: An piece of code in which you share fields and methods in a local scope. I made $files and getPhpTokenCount() private intentionally to stress this point.

I think the first two methods have little in common with the last three. So I would split this up in two classes, like so:

class DirectoryScanner
{
    protected $files;

    public function __construct($path)
    {
        $dirIterator = new RecursiveDirectoryIterator($path, FilesystemIterator::SKIP_DOTS);
        $this->files = new RecursiveIteratorIterator($dirIterator);
    }

    public function getFileCount()
    {
        return iterator_count($this->files);
    }

    public function getBytesUsed()
    {
        $bytes = 0;
        foreach ($this->files as $file) {
            $bytes += $file->getSize();
        }
        return $bytes;
    }
}

class PhpFilesScanner extends DirectoryScanner
{

    public function getPhpTokenCount($token)
    {
        $counter = 0;
        foreach ($this->files as $file) {
            if ($file->getExtension() == 'php') {
                $contents = file_get_contents($file);
                $tokens   = token_get_all($contents);
                $values   = array_count_values(array_column($tokens, 0));
                $counter += isset($values[$token]) ? $values[$token] : 0;
            }
        }
        return $counter;
    }

    public function getPhpClassCount()
    {
        return $this->getPhpTokenCount(T_CLASS);
    }

    public function getPhpFunctionCount()
    {
        return $this->getPhpTokenCount(T_FUNCTION);
    }
}

?>

It just makes more sense this way. Note that PhpFilesScanner does rely on DirectoryScanner for the iterator functionality.

I think, depending on your usage, that result buffering would be a nice addition to these classes. For instance, by once buffering alle the token counts, the PhpFilesScanner class can return multiple requests for counts of certain tokens much quicker. Especially if you expand it to include more types of tokens. Buffering would make the class look something like this:

class PhpFileScanner extends DirectoryScanner
{
    private $tokenCounts = [];

    public function __construct($path)
    {
        parent::__construct($path);
        foreach ($this->files as $file) {
            if ($file->getExtension() == 'php') {
                $contents = file_get_contents($file);
                $tokens   = token_get_all($contents);
                $counts   = array_count_values(array_column($tokens, 0));
                foreach ($counts as $token => $count) {
                    if (!isset($this->tokenCounts[$token])) {
                        $this->tokenCounts[$token] = 0;
                    }
                    $this->tokenCounts[$token] += $count;
                }
            }
        }
    }

    public function getPhpClassCount()
    {
        return $this->tokenCounts[T_CLASS] ?? 0;
    }

    public function getPhpFunctionCount()
    {
        return $this->tokenCounts[T_FUNCTION] ?? 0;
    }
}

The ?? is the new Null coalescing operator of PHP 7.


Basic graph traversals Below is an implementation of two graph traversals. Graph constructor creates a random graph with a fixed number of edges (fixed as a proportion of the maximum number of vertices, number_nodes-1).

Any comments welcome, but especially data structure and algorithm comments. Tests are not implemented properly, but it is ok in this case.

import random
class Graph:
    def __init__(self, number_nodes, max_number_edges = 0.5):
        if (max_number_edges > 1) or (max_number_edges < 0):
            raise ValueError("max_number_edges is out of range")
        self.nodes_connections = {node_nb : range(node_nb+1, number_nodes) for node_nb in xrange(number_nodes)}
        for key, value in self.nodes_connections.iteritems():
            while len(value) > (max_number_edges * (number_nodes - key - 1)):
                index_to_remove = random.randint(0, len(value)-1)
                del value[index_to_remove]

    def Print(self):
        for key, value in self.nodes_connections.iteritems():
            print key, value

    def IterateInBreadth(self, start_node_id):
        visited_nodes_ids = []
        scheduled_nodes_ids = [start_node_id]
        while scheduled_nodes_ids:
            node_id = scheduled_nodes_ids[0]
            new_scheduled_nodes_ids = [node for node in self.nodes_connections[node_id]
                                        if not (node in visited_nodes_ids or node in scheduled_nodes_ids)]
            new_scheduled_nodes_ids += [key for key, value in self.nodes_connections.iteritems()
                                            if (node_id in value) and not (key in visited_nodes_ids or key in scheduled_nodes_ids)]
            scheduled_nodes_ids += new_scheduled_nodes_ids
            visited_nodes_ids.append(node_id)
            del scheduled_nodes_ids[0]
        return visited_nodes_ids

    def __IterateInDepth(self, start_node_id, visited_nodes):
        visited_nodes.append(start_node_id)
        for key, value in self.nodes_connections.iteritems():
            if start_node_id in value:
                if key not in visited_nodes:
                    self.__IterateInDepth(start_node_id = key, visited_nodes = visited_nodes)
        for value in self.nodes_connections[start_node_id]:
            if value not in visited_nodes:
                self.__IterateInDepth(start_node_id = value, visited_nodes = visited_nodes)

    def IterateInDepth(self, start_node_id):
        visited_nodes = []
        self.__IterateInDepth(start_node_id, visited_nodes = visited_nodes)
        return visited_nodes

if __name__ == '__main__':
    #random.seed(1000)

    g = Graph(number_nodes = 11, max_number_edges = 0.33)
    g.Print()
    print g.IterateInBreadth(8)
    print g.IterateInBreadth(0)
    print g.IterateInDepth(8)
    print g.IterateInDepth(0)

It's not very relevant for traversal, but a dict-of-sets may be a better overall structure than a dict-of-lists, as it would let you check for edge existence between two random nodes in amortized constant time, as opposed to worst case linear on the number of edges.
Using sets instead of lists for things like visited_nodes is bound to have a significant impact on performance for larger graphs though.
Another point to consider, since you are building an iterator, is to make your function return, well, an iterator of course! You can always materialize them calling list on the return, but for traversals of huge graphs it may spare you a generous amount of memory.

I'm going to skip the making it a class part, but with graph being a dict-of-iterables, and the above points in mind, you could implement depth-first iteration as:

def _dfs_iter(graph, node, visited_nodes):
    if node in visited_nodes:
        raise StopIteration

    visited_nodes.add(node)
    yield node

    for next_node in graph[node]:
        for n in _dfs_iter(graph, next_node, visited_nodes):
            yield n

def dfs_iter(graph, node):
    assert node in graph
    visited_nodes = set()
    return _dfs_iter(graph, node, visited_nodes)

There is no native Python data structure really well suited to create a FIFO queue. There is deque in collections, but it is a little obscure language feature, and also not ideally suited, so I'm going to pass on using it. Your implementation using a list and removing items from the front will lead to a terrible worse case performance, probably quadratic in the number of nodes. At the cost of not releasing the memory early, I think it is better to never remove items from the queue, and use an indexing pointer.

With the same caveats as before, you could do breadth-first iteration as:

def bfs_iter(graph, node):
    assert node in graph

    fifo_index = 0
    fifo_queue = [node]
    scheduled_nodes = set(fifo_queue)

    while fifo_index < len(fifo_queue):
        node = fifo_queue[fifo_index]
        fifo_index += 1
        fifo_queue.extend([n for n in graph[node] if n not in scheduled_nodes])
        scheduled_nodes.update(graph[node])
        yield node

Note that I'm keeping duplicate accounting on scheduled_nodes, both in fifo_queue and scheduled_nodes. This is to have the ordering of the FIFO queue and the fast membership check of a hash table. You could get rid of scheduled_nodes by checking against membership in fifo_queue, but you will again get quadratic performance, not a good thing.

A simple test on this graph:

6 - 0 - 1
| / | \ |
5   4 - 3 - 2 - 7

seems to yield correct results:

if __name__ == '__main__':

    graph = {0: set([1, 4, 5, 6,]),
             1: set([0, 3,]),
             2: set([3, 7]),
             3: set([0, 1, 2, 3, 4,]),
             4: set([0, 3,]),
             5: set([0, 6,]),
             6: set([0, 5,]),
             7: set([2,]),
             }
    assert list(dfs_iter(graph, 0)) == [0, 1, 3, 2, 7, 4, 5, 6]
    assert list(bfs_iter(graph, 0)) == [0, 1, 4, 5, 6, 3, 2, 7]

Note that, since we are using sets, the exact order of iteration over the connected nodes is implementation dependent, so a failure form the above tests doesn't necessarily mean that something is broken in the algorithm.


Generating alphanumeric combinations I was happy with my shell script that needed to generate alphanumeric combinations of length N (in this case, 3)

for i in {a..z}{a..z}{a..z}; do ...

Now I became less happy when I needed alphanumeric combinations (specially if I choose a larger value for N)

for i in {0..9}{a..z}{a..z} \
         {a..z}{0..9}{a..z} \
         {a..z}{a..z}{0..9} \
         {a..z}{a..z}{a..z} \
         {0..9}{0..9}{a..z} \
         {0..9}{a..z}{0..9} \
         {a..z}{0..9}{0..9} \
         {0..9}{0..9}{0..9}; do ...

So I decided to write it in Haskell:

import Data.List
c 0 _ = [[]]
c n xs = [ y:ys | y:xs' <- Data.List.tails xs, ys <- c (n-1) xs']
main = sequence_ . map putStrLn . c 3 $ ['a' .. 'z'] ++ ['0' .. '9']

Now, I couldn't find a way to incorporate the above code in my shell script, so I had to reduce it to a one-liner to run with ghc -e, finally giving me the code I have right now:

for i in $(ghc -e "
  let c n l = if n == 0 then [[]] else \
    [y:s | y:q <- Data.List.tails l, s <- c (n-1) q] in
      sequence_. map putStrLn . c 3 $ ['a'..'z'] ++ ['0'..'9']"); do ...

Now, I actually like this style (elsewhere I have inline calls to awk too, etc). But I think this code is too big and too ugly; for example, I'm using if n == 0 instead of guards, etc. Haskell is supposed to be a lot more concise than that.

My only requirement is that I may want to change the length to be larger than 3, but also ['a'..'z'] ++ ['0'..'9'] to another set (perhaps to include - or _).

In Haskell you can always use ;, see The Haskell Report.

So your original code could be written as an one-liner as follows:

import Data.List ; c 0 _ = [[]] ; c n xs = [ y:ys | y:xs' <- tails xs, ys <- c (n-1) xs'] ; main = mapM_ putStrLn . c 3 $ ['a' .. 'b'] ++ ['0' .. '1']

A small improvement - sequence_ . map f is mapM_ f.

Of course you'll lose the nice indentation.

IF you don't mind repeating elements (which is what you have in the original shell script), you can use replicateM:

import Control.Monad
main = mapM_ putStrLn . replicateM 3 $ ['a' .. 'b'] ++ ['0' .. '1']


Social network list management I have multiple drop down lists (select & option) that are populated by data from the server. They all have the same options, but I need the use to be able to select every option only once - it can appear as selected only on one list.

This is the HTML of a single list - simple select & options list:

<div class="social-option">
    <select name="hex_theme_options[social-service-1]">
        <option selected="selected" value="0"></option>
        <option value="facebook">facebook</option>
        <option value="twitter">twitter</option>
        <option value="linkedin">linkedin</option>
        <option value="e-mail">e-mail</option>
        <option value="phone">phone</option>
        <option value="instagram">instagram</option>
        <option value="flickr">flickr</option>
        <option value="dribbble">dribbble</option>
        <option value="skype">skype</option>
        <option value="picasa">picasa</option>
        <option value="google-plus">google-plus</option>
        <option value="forrst">forrst</option>
    </select>
</div>

And this is the JS code for managing them the way I described. It works, but it looks pretty ugly to me, and I'd like to improve it's structure. Any suggestions are welcome.

jsFiddle

(function($){
    $(document).ready(function() {

        var siblings = {
            lock: function (newSelected){
                var selectedSiblings = $('.social-option select').find("option[value=" + newSelected.val() + "]");
                selectedSiblings.not(newSelected).attr('disabled', 'disabled');
            },
            unlock: function (oldSelected){
                var selectedSiblings = $('.social-option select').find("option[value=" + oldSelected.val() + "]");
                selectedSiblings.removeAttr('disabled');
            },
            unlockZero: function (){
                $('.social-option select').find("option[value='0']").removeAttr('disabled');
            }
        };

        function checkSiblings(oldSelected, newSelected) {
            if (oldSelected === '0') {
                siblings.lock(newSelected);
            } else if (newSelected === '0') {
                siblings.unlock(oldSelected);
            } else {
                siblings.unlock(oldSelected);
                siblings.lock(newSelected);
            }
        }

        $('.social-option select').each(function() {
            siblings.lock($('option:selected', this));
            siblings.unlockZero();
        });

        $('.social-option select').on('focus', function () {
            var oldSelected = $('option:selected', this);

            $('.social-option select').on('change', function () {
                var newSelected = $('option:selected', this);
                checkSiblings(oldSelected, newSelected);
            });
        });

    });
})(jQuery);

I could be wrong, but I think you could shorten ALOT of the code as follows:

$(function () {
    $(".social-option select").on("change", function(e) {
        $(".social-option select option:disabled").prop("disabled", false);
        $(".social-option select option:selected").each(function(i) {
            var $val = $(this).val();
            if ($val !== '0') {
                $(".social-option select option[value="+$val+"]").prop("disabled", true);
            };
        });
    }).change();
});

See fiddle with dynamic drop down added here


Code for adding a user to the database I have a code that adds a user to the database, getting the username from the dialog.My fragment implements the interface with the onInputSend method, which is called by the dialog when the user clicks the desired button.
Here is the code that adds the user to the database. I want to hear criticism of my code.

UsersActivity

   public void onInputSend(@NonNull InputTextAlertDialog dialog, @NotNull String input) {
    userActionManager.isEmptyUser(input)
            .doOnSubscribe(disposable -> disposables.add(disposable))
            .subscribe(empty -> insertUser(dialog, input, empty));
}

private void insertUser(InputTextAlertDialog dialog, String userName, boolean isEmpty) {
    if (isEmpty) {
        if (!userName.trim().isEmpty()) {
            userActionManager.insertUser(new User(userName));
            dialog.cancel();
        } else {
            dialog.setErrorMessage(getString(R.string.user_not_have_name));
        }
    } else {
        dialog.setErrorMessage(getString(R.string.user_already_exists));
    }
}

InputTextAlertDialog

class InputTextAlertDialog(context: Context) : BaseAlertDialog(context) {

    var onInputOkListener: OnInputOkListener? = null
    private var input: EditText? = null
    private var error: TextView? = null

    var colorError: Int = Color.RED
        set(colorError: Int) {
            field = colorError
            error!!.setTextColor(colorError)
        }

    init {
        error = view.findViewById(R.id.error_text)
        input = view.findViewById(R.id.input)
        error!!.setTextColor(colorError)
    }

    fun setErrorMessage(errorMessage: String?) {
        error!!.text = errorMessage
        input!!.backgroundTintList = ColorStateList.valueOf(colorError);
    }

    override fun initOkClickListener() {
        ok.setOnClickListener { v: View? ->
            ok()
        }
    }

    override fun ok() {
        onInputOkListener?.onInputSend(this, input!!.text.toString())
    }

    override fun getLayout(): Int {
        return R.layout.input_text_layout
    }

    interface OnInputOkListener {
        fun onInputSend(dialog: InputTextAlertDialog, input: String)
    }
}

BaseAlertDialog

public abstract class BaseAlertDialog {
    AlertDialog alertDialog;

    protected Button cancel;
    protected Button ok;

    protected View view;
    private AlertDialog.Builder builder;

    protected Context context;

    BaseAlertDialog(Context context) {
        this.context = context;
        initDialog(context);
        initViews(view);
        initCancelClickListener();
        initOkClickListener();
    }

    private void initDialog(Context context) {
        if (getLayout() != 0) {
            view = LayoutInflater.from(context).inflate(getLayout(), null);
        } else {
            throw new RuntimeException("In getLayout () you should return the layout id");
        }
        builder = new AlertDialog.Builder(context);
        builder.setView(view);
        alertDialog = builder.create();
    }

    private void initViews(View view) {
        cancel = view.findViewById(R.id.cancel);
        ok = view.findViewById(R.id.ok);
    }

    private void initCancelClickListener() {
        cancel.setOnClickListener(v -> {
            alertDialog.cancel();
        });
    }

    protected void initOkClickListener() {
        if (ok != null) {
            ok.setOnClickListener(v -> {
                ok();
                alertDialog.cancel();
            });
        }
    }

    public void cancel(){
        alertDialog.cancel();
    }

    public void show(){
        alertDialog.show();
    }

    abstract void ok();

    abstract int getLayout();

}

Nullable
Your fields are optionals: you put a ? after the type.
This means every time you need to access them, you need to check if it is null...
If you want the fields to be always present, throw the NullPointerException when findViewById doesn't return a view.
Then you can make your field not nullable and you can remove all the (now) redundant checks...
oneline function
override fun getLayout(): Int {
    return R.layout.input_text_layout
}

This function doesn't need 3 lines of attention...
Change it to one:
override fun getLayout(): Int = R.layout.input_text_layout

you could remove the return-type as well:
override fun getLayout() = R.layout.input_text_layout

init
Personal choice:
I like to make everything as short as possible.
This can be done by declaring the fields straight away instead of in the init block:
class InputTextAlertDialog(context: Context) : BaseAlertDialog(context) {

    var onInputOkListener: OnInputOkListener? = null

    var colorError: Int = Color.RED
        set(colorError: Int) {
            field = colorError
            error!!.setTextColor(colorError)
        }

    private var input: EditText = view.findViewById(R.id.input)!!
    private var error: TextView = view.findViewById(R.id.error_text)!!

    init {
        error!!.setTextColor(colorError)
    }
}

also
We can make the code above a bit simpler by using also:
also is a function which is called upon a variable (called the receiver).
also will return the receiver and accepts a lambda as parameter.
Inside that lambda, it gives one parameter: the receiver.
Therefor the following code:
class InputTextAlertDialog(context: Context) : BaseAlertDialog(context) {
    private var error: TextView = view.findViewById(R.id.error_text)!!

    init {
        error!!.setTextColor(colorError)
    }
}

can be reduced to:
class InputTextAlertDialog(context: Context) : BaseAlertDialog(context) {
    private var error: TextView = view.findViewById(R.id.error_text)!!
        .also{ v: TextView -> v.setTextColor(colorError) }
}

And if there is only one param, it can be accessed using it:
class InputTextAlertDialog(context: Context) : BaseAlertDialog(context) {
    private var error: TextView = view.findViewById(R.id.error_text)!!
        .also{ it.setTextColor(colorError) }
}