Fisher–Yates shuffle in C# Is this a proper C# implementation of the Fisher–Yates shuffle?

//Fisher–Yates shuffle en.wikipedia.org/wiki/Fisher–Yates_shuffle
//for i from 0 to n−2 do
//j ← random integer such that i ≤ j < n
//exchange a[i] and a[j]
int count = 52;
int[] deck = new int[count];
for (byte i = 0; i < count; i++)
    deck[i] = i;
Random rand = new Random();
// next(max) Returns a non-negative random integer that is less than the specified maximum.
for (byte i = 0; i <= count - 2; i++)
{
    int j = rand.Next(count - i);
    if (j > 0)
    {
        int curVal = deck[i];
        deck[i] = deck[i+j];
        deck[i+j] = curVal;
    }
}
//suffle the other directinon to be sure
//for i from n−1 downto 1 do
//j ← random integer such that 0 ≤ j ≤ i
//exchange a[j] and a[i]
for (int i = count - 1; i >= 1;  i--)
{
    int j = rand.Next(i + 1);
    if (j != i)
    {
        int curVal = deck[i];
        deck[i] = deck[j];
        deck[i] = curVal;
    }
}

Since I'm not sure what you are asking by if you are doing it correctly I decided the quickest and easiest answer is to put your code to use. I created a method called Yates and put your code in it. I then called it 19 times in a simple for loop noted the output. (I limited my output to the first 5 numbers so it is more clear here)

35 22 23 7 25...
35 22 23 7 25...
35 22 23 7 25...
35 22 23 7 25...
35 22 23 7 25...
35 22 23 7 25...
35 22 23 7 25...
35 22 23 7 25...
35 22 23 7 25...
35 22 23 7 25...
35 22 23 7 25...
35 22 23 7 25...
35 22 23 7 25...
35 22 23 7 25...
35 22 23 7 25...
35 22 23 7 25...
35 22 23 7 25...
35 22 23 7 25...
35 22 23 7 25...

You'll notice it is 19 of the same thing. The problem with Random is that it isn't truly random sometimes, and you just ran across this. simple fix for me was to move the creation of the Random rand outside of the method now when I run it.

13 48 16 39 44...
20 3 10 27 18...
20 38 6 10 9...
1 28 6 33 7...
40 20 7 23 37...
31 22 0 34 18...
51 50 49 1 48...
24 1 44 37 50...
23 1 30 41 3...
41 32 45 23 11...
31 47 32 25 34...
49 8 16 33 32...
21 12 17 32 37...
8 2 40 41 49...
0 15 29 42 39...
39 0 46 20 22...
13 24 3 34 26...
9 10 43 2 39...
36 4 40 45 48...

I know it's not much to go on and I normally give much longer reviews but there isn't much code to review.

Here is the code I used to test against

void Main()
{
    for (int i = 0; i < 19; i++)
    Console.WriteLine($"    {string.Join(" ", Yates().Take(5))}...");
}
Random rand = new Random();
private int[] Yates()
{
    int count = 52;
    int[] deck = new int[count];
    for (byte i = 0; i < count; i++)
    deck[i] = i;
    for (byte i = 0; i <= count - 2; i++)
    {
        int j = rand.Next(count - i);
        if (j > 0)
        {
            int curVal = deck[i];
            deck[i] = deck[i + j];
            deck[i + j] = curVal;
        }
    }
    for (int i = count - 1; i >= 1; i--)
    {
        int j = rand.Next(i + 1);
        if (j != i)
        {
            int curVal = deck[i];
            deck[i] = deck[j];
            deck[j] = curVal;
        }
    }
    return deck;
}

EDIT

After having read everything and considered what you are trying to accomplish I believe that you are getting what are trying to accomplish. That being it looks like you are trying to shuffle a deck of 52 cards. Also with the answer you provided this gives me a little more clue as to what to try to tell you for a better review other than just spitting out the output. So here are my suggestions:

Unit Testing

All programs that are going to be used for "production" (even if production means you are playing your own game) should have some tests in place that exercise your code. Testing randomness isn't always easy but you already thought of 3 tests that you could have written outside of your win forms application.

    for (byte i = 0; i < 52; i++)
    {
        if (hsDeck.Contains(deck[i]))
            Debug.WriteLine("Houston we have a problem");
        if (deck[i] < 0)
            Debug.WriteLine("Houston we have a problem");
        if (deck[i] > 51)
            Debug.WriteLine("Houston we have a problem");
        hsDeck.Add(deck[i]);
    }

This information would have been better served in your question. But the test is simple and is simple to reproduce using MSTest, NUnit, or xUnit. The latter two being among the more popular but MSTest is built into Visual Studio. I see that you put Debug.WriteLine in your "test" so that you can watch the debug screen. Well how about instead of reading the debug screen you watch the test runner screen for a green check or red x for a pass or fail? You could even copy and paste your code but you'd run into an immediate hicup. You have testable code inside a non-testable class. Solution: extract that method into its own very small class.

public class FisherYates
{
    private static readonly Random rand = new Random();
    public static byte[] Shuffle(byte count)
    {
        //removed for brevity
        return deck;
    }
}

Now you can write your test

[TestClass]
public class FisherYatesTests
{
    [TestMethod]
    public void ShuffleWillNotPutAValueBelowZeroOrAboveCountOrReturnDuplicates()
    {
        for (int i = 0; i < 254; i++)
        {
            var shuffled = FisherYates.Shuffle(52)
            if (shuffled.Any(x=> x<0))
                Assert.Fail("Algorithm returned value below zero");
            if (shuffled.Any(x=> x>51)
                Assert.Fail("Algorithm returned value higher than largest index");
            if (shuffled.GroupBy(x => x).Any(x=> x.Count() > 1))
                Assert.Fail("Algorithm returned duplicate values");
        }

    }
}

That test runs very fast. Total time on my pc to compile and run those tests as many times I want is under 3 seconds. Understanding the green or red marks takes next to nothing to understand.

Closeness to algorithm

So you noticed already that there was a bug in your code. I noticed (after having re-read the wiki) page that you implement the algorithm AS WELL AS an alternative.

  The modern version of the Fisher–Yates shuffle, designed for computer use, was introduced by Richard Durstenfeld in 1964[2] and popularized by Donald E. Knuth in The Art of Computer Programming as "Algorithm P".[3] Neither Durstenfeld nor Knuth, in the first edition of his book, acknowledged the work of Fisher and Yates; they may not have been aware of it. Subsequent editions of The Art of Computer Programming mention Fisher and Yates' contribution.[4]

  The algorithm described by Durstenfeld differs from that given by Fisher and Yates in a small but significant way. Whereas a naïve computer implementation of Fisher and Yates' method would spend needless time counting the remaining numbers in step 3 above, Durstenfeld's solution is to move the "struck" numbers to the end of the list by swapping them with the last unstruck number at each iteration. This reduces the algorithm's time complexity to O(n), compared to O(n2) for the naïve implementation.[5] This change gives the following algorithm (for a zero-based array).

-- To shuffle an array a of n elements (indices 0..n-1):
for i from n−1 downto 1 do
    j ← random integer such that 0 ≤ j ≤ i
    exchange a[j] and a[i]

  An equivalent version which shuffles the array in the opposite direction (from lowest index to highest) is: (emphasis mine)

-- To shuffle an array a of n elements (indices 0..n-1):
for i from 0 to n−2 do
    j ← random integer such that i ≤ j < n
 exchange a[i] and a[j]

So to answer your question did you implement it... Yes and No. Yes: you did it twice. No: You did it twice in the same method but the algorithm says once. Does it really matter? No. You want a random deck of cards you'll get it.


Searching for pseudo-palindromes ("semordnilaps") I was interested to know which words of the English language, when spelled backwards, are still valid words (e.g. "drawer", which when spelled backwards is "reward"), so I wrote a small program to go through a dictionary text file to search for them.

I don't know if there's a word to describe them, but because of the similarity to palindromes, I decided to call them pseudo-palindromes.

So, I wanted to know:

Is there a more efficient way to search for pseudo-palindromes? I believe my approach has \$O(n*log(n))\$ complexity.  Is that right?
Are there ways to make my code more readable and/or maintainable?
Are there any other tips to enhance the performance or style?

#include<iostream>
#include<fstream>
#include<vector>
#include<algorithm>

using namespace std;

// Load a dictionary file (line-separated strings) into a vector<string> and return the vector.
vector<string> loadFromDic(string filename)
{
  ifstream file;
  vector<string> strings;
  string aux;

  file.open(filename.c_str());
  while(!file.eof())
  {
    getline(file,aux);
    strings.push_back(aux);
  }

  return strings;
}

int main()
{
  // Load the dictionary from memory.
  vector<string> strings = loadFromDic("english_dic.txt");

  // Sort it alphabetically.
  sort(strings.begin(),strings.end());

  vector<string> ok_strings; // Will hold the strings which passed the test.
  string aux;
  vector<string>::iterator it;

  // For each string:
  for(it = strings.begin() ; it != strings.end() ; it++)
  {
    // Reverse the string.
    aux = *it;
    reverse(aux.begin(),aux.end());

    // If the reversed string is a word contained in the dictionary, add it to the list.
    if(binary_search(strings.begin() , strings.end() , aux))
    {
      ok_strings.push_back(*it);
    }
  }

  // Sort ok_strings based on string length first and then alphabetically.
  sort
  (ok_strings.begin() , ok_strings.end() ,
    [](string a, string b)->bool
    {
      if(a.size() == b.size())
    return a < b;
      return a.size() < b.size();
    }
  );

  // Display the strings which passed the test.
  for(it = ok_strings.begin() ; it != ok_strings.end() ; it++)
  {
    cout << *it << endl;
  }

  return 0;
}

The dictionary file I'm using is inside this .zip (renamed the US.dic -> english_dic.txt)

I'd try to break some habits, and make others in their places.

One habit to break is using while (!file.eof()) -- it's nearly always a problem (including this time).
In its place, I'd consider initializing the array of input strings from a pair of std::istream_iterators.
I'd try to form the habit of creating fstream objects initialized with the name of the name of the file you want to read/write, rather than creating an uninitialized object, then using open open the file afterwards.
I'd also try to get in the habit of using better/more efficient data structures when possible. As pointed out below, in this case it doesn't make a big difference, but it does help a little, and it actually simplifies the code--a clear win.
Finally, get in the habit of using standard algorithms where they apply. In this case, I think a non-standard algorithm really makes more sense--a transform_if, which I end up using quite a bit, as a matter of fact.

Putting these together, we could end up with something on this order:

#include <iostream>
#include <unordered_set>
#include <string>
#include <iterator>
#include <fstream>
#include <algorithm>

template <class InIt, class OutIt, class F, class P>
void transform_if(InIt b, InIt e, OutIt o, P p, F f) {
    while (b != e) {
        auto v = f(*b);
        if (p(v))
            *o++ = v;
        ++b;
    }
}

int main(){
    std::ifstream in("us.dic");
    std::unordered_set<std::string> words{
        std::istream_iterator<std::string>(in),
        std::istream_iterator<std::string>()};

    std::vector<std::string> reversed;
    transform_if(words.begin(), words.end(),
        std::back_inserter(reversed),
        [&](std::string const &s) { return words.find(s) != words.end(); },
        [](std::string const &s) { return std::string(s.rbegin(), s.rend()); });

    std::sort(reversed.begin(), reversed.end(),
        [](std::string const &a, std::string const &b) {
            if (a.length() < b.length())
                return true;
            return a < b;
        });
    for (auto const &s : reversed)
        std::cout << s << "\n";
}

I haven't tried to move much out of main, because in this case we've already reduced main to only 6 statements (though, admittedly, some of those are fairly long).

A few other miscellaneous points:

I'd prefer to see #include directives formatted with a space before the header name (as I've done in my code above).
Using an unordered_set helps the run time somewhat, but not drastically--on my machine, the CPU time is reduced from ~.35 seconds to ~.24 seconds. That's clearly faster, but not immensely so.
Another possibility to consider would be to put the inputs into a sorted vector, but use an interpolation search instead of a binary search to find the pseudo-palindromes.


Optimization of a Qt QByteArray conversion I wrote a function to convert a hexadecimal string representation (like 0x00) of some binary data to the data itself.

How can I improve this code?

QByteArray restoreData(const QByteArray &data, const QString prepender = "x")
{
    QByteArray restoredData = data;

    return QByteArray::fromHex(restoredData.replace(prepender, ""));
}

Try to avoid deep copying of source array, this can be achived with right method:

QByteArray restoreData(const QByteArray &data, char prepender = 'x')
{
    return QByteArray::fromHex(data.right(data.size() - data.indexOf(prepender) - 1);
}


Usability of this Observable class I created an Observable class that allows me to subscribe to a wrapped value. The implementation looks like this:

class Observable<ValueType> {
    typealias DidChangeHandler = (oldValue: ValueType?, newValue: ValueType) -> ()

    // MARK: Properties

    var value : ValueType {
        didSet {
            for (owner, handlers) in self._observers {
                for handler in handlers {
                    handler(oldValue: oldValue, newValue: value)
                }
            }
        }
    }

    // MARK: Initializers

    init(_ value: ValueType) {
        self.value = value
    }

    // MARK: Methods

    func addObserverForOwner(owner: AnyObject, triggerImmediately: Bool, handler: DidChangeHandler) {
        if let index = self._indexOfOwner(owner) {
            // since the owner exists, add the handler to the existing array
            self._observers[index].handlers.append(handler)
        }
        else {
            // since the owner does not already exist, add a new tuple with the
            // owner and an array with the handler
            self._observers.append(owner: owner, handlers: [handler])
        }

        if (triggerImmediately) {
            handler(oldValue: nil, newValue: self.value)
        }
    }

    func removeObserversForOwner(owner: AnyObject) {
        if let index = self._indexOfOwner(owner) {
            self._observers.removeAtIndex(index)
        }
    }

    // MARK: Private Properties

    private var _observers: [(owner: AnyObject, handlers: [DidChangeHandler])] = []

    // MARK: Private Methods

    private func _indexOfOwner(owner: AnyObject) -> Int? {
        var index: Int = 0
        for (possibleOwner, handlers) in self._observers {
            if possibleOwner === owner {
                return index
            }
            index++
        }
        return nil
    }
}

This allows me to create a variable and observer it as follows:

var observableString = Observable<String>("String")

observableString.addObserverForOwner(self, triggerImmediately: false) {
    (oldValue, newValue) in
    // ...
}

I think this is most useful to use for properties on classes.

However, there are a few things that I don't like:

Requires manually unobserving even if the owner is dealloced
triggerImmediately seems kind of verbose
oldValue has to be an optional because in the triggerImmediately case there is no old value

Mostly I am looking for any ideas on ways to improve the usability of this interface, but I am open to any ways to improve the code.

Update 08/02/2014

I actually figured out a way to not require manually unobserving if the owner is deallocated. It isn't perfect, but it does work. Basically I created a WeakWrapper class that makes a weak reference to a wrapped value. This allows Observable to keep an array of weak references instead of strong references to observers. The reference will be set to nil when the observer is dealloced so I can clean them up the next time the observable is triggered. The negative part of it is that the handlers are not removed until the next time the observable is triggered.

To see the new code, including changes suggested by @nhgrif below, you can visit my github repository.

You worry about triggerImmediately being too verbose (which can be fixed), yet you have addObserverForOwner as the primary function name, and for me, this is the part that is a bit too verbose.

Since what we're doing with this is so similar to the existing KVO in Objective-C (which can also be used in Swift), we'd do well to make this look and feel like KVO.

The KVO method looks like this: addObserver:forKeyPath:options:context:.  In KVO however, we don't get to pick our own handler.  We just have to implement observeValueForKeyPath:ofObject:change:context:.

Now, I'm not going to recommend a direct copy.  We can drop the forKeyPath: part of the first method.  The general consensus with KVO also seems to be that context: is relatively useless (it's a value you send when you start observing, and it is sent back to you in the call back method).  I also do like that you're suggesting the user can pick whatever method or function they want as the handler.

But, let's make some changes, shall we?

First of all, let's change our handler.  Instead of a method that takes an oldValue and a newValue, let's use a method that just takes a dictionary, shall we?

We're doing this, because we should change triggerImmediately to options, and provide an enum that lets users build a bitmask for their options.  In KVO, we have the following options:

NSKeyValueObservingOptionNew
NSKeyValueObservingOptionOld
NSKeyValueObservingOptionInitial
NSKeyValueObservingOptionPrior

The first two are self explanatory.

NSKeyValueObservingOptionInitial is a flag that say the handler block should be called before the addObserver method is finished--we send the current value of the observable to the handler block as a "new" value.

NSKeyValueObservingOptionPrior means the user wants to know about the values just before they're actually changed.  In the case of Swift, this means we'd fire in willSet with this option on (but we also still fire in didSet).

I'm not suggesting you use the NSKeyValueObservingOption enum.  I suggest you create your own enum.  As a minimum, include these options, but you may want to include others as well at some point in the future.  Maybe a StopObservingAfterChange option that automatically removes the observer after the first didSet for example.

But anyway, about our handler.  It should just take a dictionary argument as I stated earlier.  What values it contains will depend on the scenario and which bits are set in the options.

If the new bit is set, the dictionary will always hold the new value.  If the old bit is set, it will hold the old value in every situation it can (i.e. not when the initial bit is set and we fire before the addObserver method returns).

If the prior bit is set, when we call the handler from willSet, we include another key: "prior" which holds a boolean value of true.  I guess you could realistically include this key every time just set it to false in every other scenario.

Anyways, changing the add observer method to:

addObserver(observer:AnyObject, options:ObservableOptions, handler:DidChangeHandler);

Means that you can always add options in the future without screwing up any of the existing uses.  And changing the handler to taking a dictionary means we throw all the values we want to send to the handler into the dictionary, and not only is the handler flexible for different current use cases, it is also flexible for all future use cases.

As a final note, I might include constant values to use as the keys to the "change" dictionary.


Unit test an ExecutorService in a deamon To execute tasks sequentially with a timeout I use ExecutorService.submit() and Future.get(). As I don't want to block calling clients, I implement producer–consumer pattern with a queue into a Thread while(true) loop:

import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.LinkedBlockingQueue;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;

public class ExecutorWithQueue {
    private int timeout = 1;
    private TimeUnit timeoutUnit = TimeUnit.SECONDS;
    private LinkedBlockingQueue<Task> queue = new LinkedBlockingQueue<Task>();
    ExecutorService executor = Executors.newSingleThreadExecutor();
    private TaskProcessor taskProcessor = new TaskProcessor();

    public ExecutorWithQueue(int timeout, TimeUnit timeoutUnit)
    {
        this.timeout = timeout;
        this.timeoutUnit = timeoutUnit;

        taskProcessor.start();
    }

    public void push(Task task)
    {
        queue.add(task);
    }

    class TaskProcessor extends Thread
    {
        public void run() {
            while (true)
            {
                try
                {
                    final Task queueTask = queue.take();
                    Future<Boolean> future = executor.submit(queueTask);

                    try {
                        future.get(timeout, timeoutUnit);
                    } catch (TimeoutException e) {
                        System.out.println("TimeoutException");
                    }
                    catch (ExecutionException e)
                    {
                        System.out.println("ExecutionException");
                    }
                }
                catch (InterruptedException e)
                {
                    break;
                }
            }
            executor.shutdownNow();
        }
    }
}

I have two unit tests. The first to check if task is executed, the second to check that a task that takes two long doesn't block the execution flow:

import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static org.junit.Assert.assertEquals;
import org.junit.Test;

public class TestExecutorWithQueue {

    ExecutorWithQueue executor = new ExecutorWithQueue(9, MILLISECONDS);

    @Test public void fastTaskIsExecuted() throws Exception {
        assertExecuted(0, true);
    }

    @Test public void slowTaskIsNotExecuted() throws Exception {
        assertExecuted(50, false);
    }

    public void assertExecuted(int waiting, boolean expected) throws InterruptedException
    {
        WaitingTask task = new WaitingTask(waiting);

        executor.push(task);
        Thread.sleep(100);

        assertEquals(expected, task.executed);
    }

    class WaitingTask implements Task {
        boolean executed;
        int seconds_to_wait;

        public WaitingTask(int milliseconds_to_wait) {
            this.executed = false;
            this.milliseconds_to_wait = milliseconds_to_wait;
        }

        @Override
        public Boolean call() throws Exception {
            Thread.sleep(this.milliseconds_to_wait);
            this.executed=true;
            return executed;
        }
    }
}

What do you think of this code?

TaskProcessor

I see little reason why this should extend Thread, rather than simply implement Runnable
as a thread it is responsive to interruption, yet there is no way a client can interrupt this thread, through API nor directly. No, I'm mistaken, a client could push a task that interrupts its own thread, yet that is not an obvious way to shut it down, nor a clean one (as the ExecutorWithQueue will not know its executor has stopped working).
I would replace while (true) with while (!Thread.currentThread().isInterrupted())
I'm not happy with the way the ExecutionException is eaten. Clients have no way to know about the Exception that occurred in the Task.
As an extension of the previous point : you deny your clients the benefits of a Future to monitor their submitted tasks.
you accept only Tasks, yet tere is no reason you couldn't handle all Callables.

ExecutorWithQueue

taskProcessor does not need to be a field
The values supplied to timeout and timeoutUnit in their declaration are always overwritten in the constructor. If you omit these initialization, you can make them final.
the queue and executor fields can also be final.

Test

Fiddling with sleep() is error prone. The test can fail if the sleep is too short, and all the time that the test sleeps in excess of what is needed delays the swift completion of the unit test suite. (in projects with many thread safety tests this can really make the total suite of tests too slow). Timing combinations are often more reliably simulated using CountDownLatches, and these will never take longer than needed.
I find it good practice to add a timeout value to the @Test annotation for these kinds of tests. If at some point deadlock or so occurs, the test will fail gracefully, rather than hold up the test server.

Semantics

I'm completely puzzled by what this class is intended to do. All submitted tasks will simply be executed in sequence. But that could simply be achieved by submitting them to an ExecutorService with just one thread, which already uses a queue to store tasks it has yet to run. When the timeout passes and the task isn't finished, you submit the next task, which will simply be queued by the underlying ExecutorService until the timed out task has finished. I suspect you somehow think the timeout will cancel the task, but it won't. If that is your intention, you'll need to add cancelling logic to handle a timeout, if it isn't there doesn't seem to be much point in this class.

The second test seems to assert that a timed out task does not execute. Yet it only asserts this after having slept a certain amount of time. If you increase the sleep time enough, the assertion will fail. (this is another argument pro CountDownLatch as that will communicate the test's intention better).

Hope you found this useful.


How does chicken.js work? Recently Torbjörn Söderstedt shipped the chicken programing specification. The basic premise of chicken is this: Only the word chicken, spaces and line breaks may be used. It was inspired by this paper and this talk.

Along with the specification he also created a javascript function that implements the chicken VM. Sadly (or joyfully?) he wrote the function with little more than the word chicken and thus mad if very very hard to understand.

I was wondering, could anyone do a code review of chicken.js? Maybe add a bunch of in-line comments?

function chicken(CHICKEN, Chicken) {
    Chicken &&( chicken. chicken =[,
    CHICKEN, CHICKEN = Chicken = chicken.
    $Chicken =-( CHICKEN ==( chicken.
    Chicken = Chicken ))], chicken.
    chicken [Chicken++] = chicken. chicken, chicken.
    CHICKEN = ++Chicken, chicken (--Chicken), chicken.
    $Chicken = ++Chicken, chicken. CHICKEN++ );
    Chicken = chicken. Chicken [chicken.
    $Chicken++ ]; chicken. Chicken = CHICKEN? Chicken?
    '\012'== Chicken? chicken (++ CHICKEN, chicken.
    chicken [++ chicken. CHICKEN ]=
    CHICKEN - CHICKEN ): Chicken
    ==' '|'\015'== Chicken ||
    (Chicken   )== "c" &  chicken. Chicken [chicken.
    $Chicken++ ]== "h" &  chicken. Chicken [chicken.
    $Chicken++ ]== "i" &  chicken. Chicken [chicken.
    $Chicken++ ]== "c" &  chicken. Chicken [chicken.
    $Chicken++ ]== "k" &  chicken. Chicken [chicken.
    $Chicken++ ]== "e" &  chicken. Chicken [chicken.
    $Chicken++ ]== "n"&&++chicken. chicken [chicken.
    CHICKEN]? chicken (CHICKEN)
    :[ "Error on line "+CHICKEN+": expected 'chicken'",
       chicken. CHICKEN = CHICKEN ++- CHICKEN ]:
    chicken. chicken :( CHICKEN = chicken.
    Chicken[chicken.CHICKEN], Chicken? (Chicken =

    --Chicken? --Chicken? --Chicken? --Chicken? --Chicken?
    --Chicken? --Chicken? --Chicken? --Chicken?
    chicken. CHICKEN++ &&
    --Chicken :'&#'+CHICKEN+';': chicken.
    Chicken [chicken. Chicken [-- chicken. CHICKEN ]&&
    (chicken. $Chicken += CHICKEN), --chicken.
    CHICKEN ]: chicken. Chicken [chicken.
    Chicken [CHICKEN] = chicken. Chicken
    [-- chicken. CHICKEN ],-- chicken. CHICKEN ]:
    chicken. Chicken [chicken. Chicken [chicken.
    $Chicken++ ]] [CHICKEN]: CHICKEN == chicken.
    Chicken [-- chicken. CHICKEN ]:
    CHICKEN*chicken. Chicken [-- chicken.
    CHICKEN ]: chicken. Chicken [-- chicken.
    CHICKEN ]- CHICKEN: chicken. Chicken [-- chicken.
    CHICKEN ]+ CHICKEN: chicken.
    CHICKEN ++ && "chicken", chicken.
    Chicken [chicken. CHICKEN ]= Chicken, chicken
    ()): CHICKEN );

    return chicken.
    Chicken
}

Here's my first analysis. It's not complete, but the VM should be mostly understandable now. I've included some testcases from the homepage in the bottom.

var out,
    stack,
    input_ptr,
    stack_ptr;

function chicken(c, b)
{
    if(b)
    {
        // run once at the beginning of each interpretation
        // after this, chicken is only called recursively with one argument

        if(c == b)
        {
            out = b;
            c = b = input_ptr = 0;
            stack = [undefined, c, 0]
            stack[0] = stack;
            stack_ptr = 2;
            // parse the code
            chicken(1);
            // parsing is done, run the code
            input_ptr = 2;
            stack_ptr++;
        }
        else
        {
            // code is equal to input?!
            stack = [undefined, c, c = b = input_ptr = -( c == (out = b))];
            stack[b++] = stack;
            stack_ptr = ++b;
            chicken(--b);
            input_ptr = ++b;
            stack_ptr++;
        }
    }

    // either one character from the code
    // or a number of chickens to be interpreted as code
    b = out[input_ptr++];

    if(c)
    {
        // parsing
        if(b)
        {
            if(b == '\n')
            {
                // start counting on the top of the stack
                stack[++stack_ptr] = 0;
                return out = chicken(c + 1, 0);
            }
            else
            {
                // call chicken again, if:
                // 1. input it ' ' or '\r'
                // 2. input is 'chicken'
                // In the latter case, increment the top of the stack
                return out =
                    (b == ' ' | '\r' == b) ||
                    ((b == "c" &
                    out[input_ptr++] == "h" &
                    out[input_ptr++] == "i" &
                    out[input_ptr++] == "c" &
                    out[input_ptr++] == "k" &
                    out[input_ptr++] == "e" &
                    out[input_ptr++] == "n") &&
                    ++stack[stack_ptr]) ?
                        chicken(c)
                    :
                        // otherwise, error
                        ["Error on line " + c + ": expected 'chicken'", stack_ptr = c++ -c];
            }
        }
        else
        {
            // parsing is done, continue running the code
            return out = stack;
        }
    }
    else
    {
        // run the code
        // out is the stack at this point
        console.assert(out === stack);

        var top = out[stack_ptr];

        if(b)
        {
            (b = --b? --b? --b? --b? --b? --b? --b? --b? --b?

                /* n push */            stack_ptr++ && --b
                /* 9, BBQ */            : '&#' + top + ';'
                /* 8, fr */             : out[out[--stack_ptr] && (input_ptr += top), --stack_ptr]
                /* 7, peck */           : out[out[top] = out[--stack_ptr], --stack_ptr]
                /* 6, pick */           : out[out[input_ptr++]][top]
                /* 5, compare */        : out[--stack_ptr] == top
                /* 4, rooster (mult) */ : out[--stack_ptr] * top
                /* 3, fox (sub) */      : out[--stack_ptr] - top
                /* 2, add */            : out[--stack_ptr] + top
                /* 1, chicken */        : stack_ptr++ && "chicken");

            // push result on stack
            out[stack_ptr] = b;

            // next instruction
            return out = chicken();
        }
        else
        {
            /* 0, terminate */
            return top;
        }
    }
}

var read = require("fs").readFileSync;

console.assert(chicken("","chicken") === "chicken");
console.assert(chicken("test foo", read("echo.chicken") + "") == "test foo")
console.assert(chicken("", read("hello.chicken") + "") == "&#72;&#101;&#108;&#108;&#111;&#32;&#119;&#111;&#114;&#108;&#100;");


WinForm plugin architecture I have been developing this plugin architecture, mostly for fun and education, its a simple WinForm with some plugin logic and two plugins, i want to know if there is a better way of doing it.

Form1.cs

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Reflection;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace Plugin_architecture
{
    public partial class Form1 : Form
    {
    // -----------------------Plugin logic ----------------------------------

    private static IEnumerable<Type> getDerivedTypesFor(Type baseType)
    {
        var assembly = Assembly.GetExecutingAssembly();
        return assembly.GetTypes().Where(baseType.IsAssignableFrom).Where(t => baseType != t);
    }

    public void invokePlugin(object sender, EventArgs e, string pluginIdentifier) {
        Type type = Type.GetType(pluginIdentifier);
        object instance = Activator.CreateInstance(type);
        MethodInfo method = type.GetMethod("start");
        method.Invoke(instance, null);
    }

    public void registerPlugins() {
        // Register plugins at application startup
        IEnumerable<Type> plugins = getDerivedTypesFor(typeof(Plugin));

        foreach(Type plugin in plugins) {

            LinkLabel pluginLabel = new LinkLabel();
            pluginLabel.Text = plugin.ToString();
            pluginLabel.Click += delegate(object sender, EventArgs e) { invokePlugin(sender, e, plugin.ToString()); };
            pluginContainer.Controls.Add(pluginLabel);

        }
    }
    // --------------------------------------------------------------------------

    public Form1()
    {
        InitializeComponent();
        registerPlugins();
        }
    }
}

Plugin.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

abstract class Plugin
{
/*
 * Base class for plugins
 * Implement the start method and add any functionality you want. The start method will be
 * called each time the plugin is started. Use the stop method to do any nessesary cleanup.
 */

//public void init() {
//    // Initialize plugin
//}

//public void register() {
//    // Register plugin in application
//}

abstract public void start();
abstract public void stop();
}

ServicePlugin.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;

class ServicePlugin : Plugin
{
override public void start() {

    const string message = "Yeahh! Plugins!!";
    const string caption = "Plugins!";
    var result = MessageBox.Show(message, caption,
                                 MessageBoxButtons.OK,
                                 MessageBoxIcon.Information);
}
override public void stop() {

}
}

ClientPlugin.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using Plugin_architecture.Plugins.ClientPlugin;

class ClientPlugin : Plugin
{
override public void start() {

    Client frm = new Client();
    frm.Show();
}
override public void stop() {

    }
}

Client.cs

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace Plugin_architecture.Plugins.ClientPlugin
{
    public partial class Client : Form
    {
        public Client()
        {
            InitializeComponent();
        }
    }
}

The code works great, and should compile without problems.

You should use an interface instead of an abstract class:

public interface IPlugin {
    void Start();
    void Stop();
}

There are many reasons for this, as detailed in https://stackoverflow.com/questions/56867/interface-vs-base-class

I would also separate the plugin logic out of the form, and create a "PluginResolver" class to handle these concerns (This is where you hide the ugly reflection stuff you don't want to touch again).

In terms of the rest of the code, it's pretty basic, so not much to add.

In terms of creating a "plugin architecture" it's kind of a waste without using dependency injection (or a dependency resolver of some kind), because there's no obvious way for plugins to communicate with each other, unless they get coupled together (which defeats the whole point of the plugin architecture), instead by having a set of root services that can be injected into the particular plugins, you can have a disconnected way of giving them all access to say "the primary error logger".


Time lag on change function I have a script to load a range of number. This range of number is only 200 lengths. However, it has a change function. When the start number is change, the the end number will try to find its range again.

The whole of code is working fine. However, it gives me a time lag (sometime get crash) on each change.

Can anyone help me to clean this code, please?

function pad (str, max)
    {
        str = str.toString();

        return str.length < max ? pad("0" + str, max) : str;
    }

$("#game, #camp").on('change', function(){
    a = $("#game").val(),
    b = $("#camp").val(),
    j = pad(a,2);
    k = pad(b,2);
    c = j+k;

    $("#serial").empty();

    for (var i = 1; i<=200; i++)
         {
             l = pad(i,3)
             $("#manual").append("<option value='"+(c+l)+"'>"+(c+l)+"</option>");
         }

var $auto = $('#automatic'),
    $manual = $('#manual'),
    lastValue = +$manual.find('option').last().val();

    $manual.on('change', function () {

        var value = +$(this).val(),
        max = lastValue;

        //alert(max);

        $auto.empty();

        for (var i = value; i < max; i++)
            {
                $('<option>',
                     {
                        value:i+1,
                        text: i+1
                     }).appendTo($auto);
            }
    }).change()

});
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.8.3/jquery.min.js"></script>
<select id="game">
    <option value="01">01</option>
    <option value="02">02</option>
    <option value="03">03</option>
    <option value="04">04</option>
    <option value="05">05</option>
    <option value="06">06</option>
    <option value="07">07</option>
</select>

<select id="camp">
    <option value="11">11</option>
    <option value="22">22</option>
    <option value="33">33</option>
    <option value="44">44</option>
    <option value="55">55</option>
    <option value="66">66</option>
    <option value="77">77</option>
</select>

<select id="manual"></select>
<select id="automatic"></select>

Code hygiene

What is $("#serial")?  Did you mean $("#manual").empty() instead?

You are sloppy in localizing your variables.  a, b, j, k, c, l, lastValue, and max (the one in the $manual.on('change') handler) are all global.  All of them are horribly named.  Furthermore, I recommend eliminating unnecessary variables.

Take care to end your statements with a semicolon.  It's considered good practice even though the language doesn't require them.

Performance

There are two major problems.

The first issue is that it is much more efficient to append a giant string all at once than to manipulate the DOM an element at a time.

The second problem is that you are defining a change handler within a change handler.  That means the #manual element gets regenerated more and more frantically with each change in the first two columns.

function pad(str, max) {
    str = str.toString();
    return str.length < max ? pad("0" + str, max) : str;
}

$('#manual').on('change', function() {
    var autoOptions = '';
    var lastValue = +$(this).find('option').last().val();
    for (var i = 1 + +$(this).val(); i <= lastValue; i++) {
        // Should i be padded to 7 digits?
        autoOptions += '<option value="' + i + '">' + i + '</option>';
    }

    $('#automatic').html(autoOptions);
});

$("#game, #camp").on('change', function() {
    var gameCamp = pad($('#game').val(), 2) +
                   pad($('#camp').val(), 2);

    var manualOptions = '';
    for (var i = 1; i <= 200; i++) {
        var iii = pad(i, 3);
        manualOptions += '<option value="' + gameCamp + iii + '">' + gameCamp + iii + '</option>';
    }
    $('#manual').html(manualOptions)
                .change();
});
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.8.3/jquery.min.js"></script>
<select id="game">
    <option value="01">01</option>
    <option value="02">02</option>
    <option value="03">03</option>
    <option value="04">04</option>
    <option value="05">05</option>
    <option value="06">06</option>
    <option value="07">07</option>
</select>

<select id="camp">
    <option value="11">11</option>
    <option value="22">22</option>
    <option value="33">33</option>
    <option value="44">44</option>
    <option value="55">55</option>
    <option value="66">66</option>
    <option value="77">77</option>
</select>

<select id="manual"></select>
<select id="automatic"></select>


Date Comparison Function for `usort()` I am working through a PHP book, and date comparison was mentioned as one of the uses of usort(), so I wrote a date comparison function to use in line with usort(). Writing the function wasn't as easy as I expected (there was a pesky bug which took me a while to identify), so I thought I would submit my comparison code for review.

dates.php

<?
    function dateParse($date)
    {
        return (count(explode("-", $date)) == 3)?(explode("-", $date)):(explode("/", $date));   #Returns the date as an array.
    }

    function dateSort($date1, $date2, $format=1)
    {
        if ($format == 0)   #dd/mm/yyyy format.
        {
            list($day1, $month1, $year1) = dateParse($date1);
            list($day2, $month2, $year2) = dateParse($date2);
        }
        elseif ($format == 1)   #mm/dd/yyyy format.
        {
            list($month1, $day1, $year1) = dateParse($date1);
            list($month2, $day2, $year2) = dateParse($date2);
        }
        elseif ($format == 2)   #yyyy/mm/dd format.
        {
            list($year1, $month1, $day1) = dateParse($date1);
            list($year2, $month2, $day2) = dateParse($date2);
        }
        $day1 = str_pad($day1, 2, "0", STR_PAD_LEFT);   #Adds a leading "0" if the day is a single digit.
        $day2 = str_pad($day2, 2, "0", STR_PAD_LEFT);   #Adds a leading "0" if the day is a single digit.
        $month1 = str_pad($month1, 2, "0", STR_PAD_LEFT);   #Adds a leading "0" if the month is a single digit.
        $month2 = str_pad($month2, 2, "0", STR_PAD_LEFT);   #Adds a leading "0" if the month is a single digit.

        $x1 = $year1.$month1.$day1;
        $x2 = $year2.$month2.$day2;

        return ((int)$x1 <=> (int)$x2);
    }
?>

Magic numbers are a bad code smell and should be avoided. Use a constant instead.

In your definition of dateSort(), your third parameter is format=1. What does 1 mean? Without reviewing the code it isn't clear which makes maintaining the code more difficult. A common way to avoid this is to define constants with human readable names and use them in place of the magic numbers. Note that the names of these constants are arbitrary. Name them in such a way that makes sense to the reader what value they hold or reflect.

 defined('DATE_FORMAT_EU_SLASHES') || define('DATE_FORMAT_EU_SLASHES', 0);
 defined('DATE_FORMAT_US')         || define('DATE_FORMAT_US', 1);
 defined('DATE_FORMAT_YEAR_FIRST') || define('DATE_FORMAT_YEAR_FIRST', 2);

Using date_parse() is okay but working with DateTime() objects is preferred.

PHP offers a lot of ways of working with dates which can be a good or bad thing. In general using DateTime() objects are preferred as they were designed specifically for this purpose (and is the most powerful).

Your code can be simplified as DateTime() can parse most date formats natively and also handle not standard formats easily. date_parse() will fail once the format becomes non-standard or irregular.

Additionally, DateTime() objects are comparable so you don't need to output a string variable in a comparable format to do your comparison. Just compare the objects themselves.

$datetime1 = new DateTime($date1);
$datetime2 = new DateTime($date2);
return datetime1 <=> datetime2;

Simpler!

Use a switch statement instead of a potentially giant if/else

Because we can have situations where DateTime() can handle the format natively, there is no need to have an if/else statement for every date format. switch allows us to allow conditions to be handled by the same code block. It also offers us a default option which means we no longer need to define our default format in the function signature and can let the switch statement handle it for us.

defined('DATE_FORMAT_EU_SLASHES') || define('DATE_FORMAT_EU_SLASHES', 0);
defined('DATE_FORMAT_US')         || define('DATE_FORMAT_US', 1);
defined('DATE_FORMAT_YEAR_FIRST') || define('DATE_FORMAT_YEAR_FIRST', 2);

function dateSort($date1, $date2, $format = null)
    switch ($format) {
        case DATE_FORMAT_EU_SLASHES :
            // DateTime::createFromFormat() offers us flexibility
            $datetime1 = DateTime::createFromFormat('d/m/Y', $date1);
            $datetime2 = DateTime::createFromFormat('d/m/Y', $date2);
            break;
        case DATE_FORMAT_US :
        case DATE_FORMAT_YEAR_FIRST :
        default :
            // DateTime() can handle both formats natively so no need to handle them
            // individually. You can also ad formats like YYYY-MM-DD with only a couple
            // of lines of code
            $datetime1 = new DateTime($date1);
            $datetime2 = new DateTime($date2);
            break;
    }
    return datetime1 <=> datetime2;
}

If you want to take it a step further, you could simplify your code and only specify when the format is non-standard and handle them specifically. Otherwise, fall to the default date handler:

defined('DATE_FORMAT_EU_SLASHES') || define('DATE_FORMAT_EU_SLASHES', 0);

function dateSort($date1, $date2, $format = null)
    switch ($format) {
        case DATE_FORMAT_EU_SLASHES :
            $datetime1 = DateTime::createFromFormat('d/m/Y', $date1);
            $datetime2 = DateTime::createFromFormat('d/m/Y', $date2);
            break;
        default :
            $datetime1 = new DateTime($date1);
            $datetime2 = new DateTime($date2);
            break;
    }
    return datetime1 <=> datetime2;
}

*None of this code is tested -- I just wrote it off of the top of my head -- but hopefully it gives you ideas about how to improve this code


Euler's Totient Function in x86 assembly (MASM) My task was to implement Euler's totient function, and I'm looking for any and all criticism.

.386
.model flat, stdcall
.stack 4096

ExitProcess PROTO, dwExitCode:DWORD

.code
;------------------------------------------------------------------------------
; _phi@4 (int x)
;
; Calculates Euler's totient function using the product formula.
;
; param:
  x     TEXTEQU <[ebp+8]>       ;integer argument
;
; Exit: The result is stored in eax
;------------------------------------------------------------------------------
_phi@4:
        push    ebp             ; setup stack, save regs
        mov     ebp, esp
        push    ebx
        push    ecx
        push    edx

        mov     ecx, x          ; init result as x

; Consider the prime factors of x and subtract their multiples
; from the result
        mov     ebx, 2          ; initialize loop counter p
L3:     mov     eax, ebx
        mul     ebx
        cmp     DWORD PTR x, eax ; loop if( x >= p * p)
        jnge    endL3;

; Check if p is a prime factor
        cdq
        mov     eax, x
        div     ebx
        cmp     edx, 0          ; if(x % p == 0)
        jne     noprime

;if yes, update x and result
L2:     cdq
        mov     eax, x
        div     ebx
        cmp     edx, 0          ; if(x % p == 0)
        jne     endL2
        mov     x  , eax        ; x /= p
        jmp     L2
endL2:
        cdq
        mov     eax, ecx
        div     ebx
        sub     ecx, eax        ;result -= result / p
noprime:
        inc     ebx
        jmp     L3
endL3:

; If x has a prime factor greater than sqrt(x)
        cmp     DWORD PTR x, 1
        jle     finish
        cdq
        mov     eax, ecx
        div     DWORD PTR x
        sub     ecx, eax
finish:
        mov     eax, ecx

        pop     edx
        pop     ecx
        pop     ebx
        pop     ebp
        ret     4

main:
        push    100
        call    _phi@4
        INVOKE  ExitProcess, 0

        END     main

Is there any way I can make the code more efficient? - possibly with fewer instructions? Also, I'm looking for pointers on style. Is the code readable?

You're not using cdq properly.  cdq sign extends EAX into EDX usually in preparation for an idiv (signed division) instruction.  However, you're using div (unsigned division), so instead of using cdq (which can potentially fill EDX with 0xFFFFFFFF), you should zero out that register instead.  If you were doing an idiv, though, the cdq is in the wrong place since it needs to come after loading data into EAX.  And there are a couple of places where you already know that EDX will be 0 so you don't need the instruction at all.

In place of cmp edx,0, you can use test edx,edx.  It sets the zero and sign flags the same way but does so with a smaller instruction.

Your L2 loop can be shortened a bit.  If EDX is zero (no remainder after the division), you already know that EDX is zero, so you don't need to zero it out, and that x is already in EAX.  So you can loop back to the div instruction.


Your Code

Class method

You created the class method parse with this snippet:

class << self
  def parse str
    Grid.new str.split("\n").map { |row| row.split(" ").map { |val| val.to_i } }
  end
end

That works, but a more conventional definition is just:

def self.parse str
  Grid.new str.split("\n").map { |row| row.split(" ").map { |val| val.to_i } }
end

Further, since self equals Grid when this method is created, it is better to write:

def self.parse str
  new str.split("\n").map { |row| row.split(" ").map { |val| val.to_i } }
end

so that this code needn't be changed if you rename the class. (Note that new str.split... is the same as self.new str.split....)

However, you don't need a class method to create the instance. Instead, just write:

arr = str.split("\n").map { |row| row.split(" ").map { |val| val.to_i } }
grid = Grid.new(arr)

Consolidate all calculations in the class

You want to move:

def max_product lines
  lines.flat_map { |line| line.each_cons(4).to_a }.map { |cons| cons.inject {
   |prd, x| prd * x} }.max
end

and

[:horizontals, :verticals, :diagonals, :inv_diagonals].map { |method|
   max_product(grid.send(method)) }.max

into the class definition and make the latter a method:

def max_product_all_ways
  [:horizontals, :verticals, :diagonals, :inv_diagonals].map { |method|
    max_product(grid.send(method)) }.max
end

but I think it would be clearer to write:

def max_product_all_ways
  [max_product(horizontals),
   max_product(verticals),
   max_product(diagonals),
   max_product(inv_diagonals)].max
end

You would then invoke max_product_all_ways as follows:

arr = str.split("\n").map { |row| row.split(" ").map { |val| val.to_i } }
grid = Grid.new(arr)
grid.max_product_wall_ways

Diagonal detail

It's a small thing, but in diagonals you don't need to compute the diagonals of length less than four if only arrays of size four are to be considered. In fact, you should not, as it is conceivable that a diagonal of length less than four could have the greatest product!

Revised class definition

So this is what we now have (more or less):

class Grid

  def initialize arr
    @matrix = arr
    @dim = @matrix.size
  end

  def max_product_all_ways
    [max_product(horizontals),
     max_product(verticals),
     max_product(diagonals),
     # max_product(inv_diagonals)
     ].max
  end

  private

  def max_product lines
    lines.flat_map { |line| line.each_cons(4).to_a }.
          map { |cons| cons.inject { |prd, x| prd * x} }.max
  end

  def horizontals
    @matrix
  end

  def verticals
    @matrix.transpose
  end

  def diagonals
    (0..@dim-1).map { |i| diag(0, i) }.reverse +
    (1..@dim-1).map { |i| diag(i, 0) }
  end

  def inv_diagonals
    rev.diagonals
  end

  def diag x, y
    result = []
    while x < @dim && y < @dim
      result << @matrix[x][y]
      x += 1
      y += 1
    end
    result
  end

  def rev
    @matrix.map { |row| row.reverse }
  end
end

Well, this is still not quite right. The calculation of inv_diagonals is not correct, but it needs a fair bit of work to fix.  I've therefore commented it out in max_product_all_ways.  I also changed the name of the method reverse because it's easily confused with the built-in methods for reverse for strings and arrays.

I'll leave it to you to fix the rest and of course others may offer additional suggestions.

Alternative Solution

Here is one way you could write the program more compactly.

Code

class Grid

  def max_product(a, n)
    [array_max(a, n),
     array_max(a.transpose, n),
     array_max(diagonals(a, n), n),
     array_max(diagonals(rotate(a), n), n)
    ].max
  end

  def array_max(a,n)
    row_max(a.max_by { |r| row_max(r,n) }, n)
  end

  def row_max(r,n)
    r.each_cons(n).max_by { |e| e.reduce(:*) }.reduce(:*)
  end

  def diagonals(a, n)
    (0..a.size-n).each_with_object([]) do |i,b|
      b << (0...a.size-i).map { |j| a[i+j][j] }
      b << (0...a.size-i).map { |j| a[j][i+j] } if i > 0
    end
  end

  def rotate(a)
    a.transpose.reverse
  end
end

Examples

Given 20x20 array

a = str.lines.map { |l| l.split.map(&:to_i) }

g.max_product(a,4) #=>    51,267,216
g.max_product(a,3) #=>       776,776
g.max_product(a,5) #=> 3,143,734,308

3x3 array

a = [[1,2,3],
     [4,5,6],
     [7,8,9]]
g.max_product(a,1) #=>   9
g.max_product(a,2) #=>  72
g.max_product(a,3) #=> 504

a = [[1,2,3],
     [4,5,8],
     [7,6,9]]
g.max_product(a,2) #=> 72

a = [[1,7,3],
     [4,8,6],
     [5,2,9]]
g.max_product(a,2) #=> 72

a = [[1,2,3],
     [4,8,6],
     [9,5,7]]
g.max_product(a,2) #=> 72

In the last example, you can see the effects of rotate and diagonals:

b = rotate(a)
  #=> [[3, 6, 7],
  #    [2, 8, 5],
  #    [1, 4, 9]]

c = diagonals(b, 2)
  #=> [[3, 8, 9], [2, 4], [6, 5]]

array_max(c, 2)
  #=> [72, 8, 30].max => 72

Incidentally, I made the number of elements whose product is to be maximized a variable because it wasn't really any more difficult to do that and it facilitated debugging.


Simulator game in python I am making a simulator game "BEE HUNT SIMULATOR" and in that game we have bees and we have to collect pollen from different fields,this is fully command line game that means a typing game so i have made all things that should be there but the main thing is of collecting pollen out of specific storage for that i had tried this-:
import time

global aa
aa = 15 # POWER OF ALL BEES WHICH ADDED IN POLLEN PER FUNCTION
global b
b = 100 # MAX POLLEN STORAGE
global honey
honey = 0 # IGNORE THIS VARIABLE
global pollen
pollen = 0 # POLLEN WHICH ADDS ON EACH FUNCTION AND DID NOT EXCEND THE STORAGE

def add():

    global pollen
    if pollen < b:

        ss = aa + pollen
        if ss > b:

            pollen = b
        elif ss <= b:
            print("Collecting pollen....")
            time.sleep(0.5)
            pollen += aa

    else:
        print("Backpack Full convert it to honey or buy a new backapck.")
while True:

    a = input("Press enter or any key: ")

    if a == "s":
        print(a,b,honey,"",pollen)
        print(type(aa))
    else:
        add()
        print("Pollen Collected-: ",pollen,"/",b)
        time.sleep(1)

That code is fully working and its output is like this-:
Press enter or any key:
Collecting pollen....
Pollen Collected-:  15 / 100
Press enter or any key:
Collecting pollen....
Pollen Collected-:  30 / 100
Press enter or any key:

But i think that this can bored peoples and they can easliy collect pollen so i need more system like this which should not bored peoples and they take enough time to collect pollen.
I am using pyCharm
This code is fully working i only need ideas for collecting pollen , so that players like to do and they dont get bored and they take time to collect pollen.

As mentioned in comments above, your question is about gamedesign. But since you brought your code here already, here's a suggestion: use better variable names - aa and b are very non-descriptive and as the amount of code will grow, you'll have a hard time to understand what's going on there with variables like these.
So instead of b, use something like MAX_POLLEN. When you'll see it in the code you'll immediately know what it is. And since it's UPPERCASE, which is a Python's convention for constants, you'll also know it won't change its value over time.


Lightweight accordion script I'm using a very lightweight jQuery accordion with some tweaks and it's working nicely. Is there anything to improve?

JS

;(function($, doc, win) {
    "use strict";

    $.fn.accordionLite = function(){

        var sections = this.children(),
            trigger = sections.find('.accordion_trigger'),
            allContent = sections.find('.accordion_content');

        allContent.hide();
        $('.accordion_trigger.active').next('.accordion_content').show();

        trigger.click(function(){

            var activeContent = $(this).next('.accordion_content');

            if (activeContent.is(':hidden')) {
                allContent.slideUp('fast');
                activeContent.slideDown('fast');
                trigger.removeClass('active');
                $(this).addClass('active');
            } else {
                allContent.slideUp('fast');
                trigger.removeClass('active');
            }

        });

    }

})(jQuery, document, window);

Markup

        Trigger 1

Content Area 1

        Trigger 2

Content Area 2

        Trigger 3

Content Area 3

This is awesome code,

I would have added an s to var trigger since you are counting on more than 1 trigger.


Reverse double linked list in python 2.7 Problem is straightforward, reverse a double linkedlist, any advice on functional bug, performance in terms of algorithm time complexity and code style are highly appreciated.

Code written in Python 2.7.

class DoubleLinkedListNode:
    def __init__(self, value, next_node):
        self.value = value
        self.pre_node = None
        if next_node:
            next_node.pre_node = self
        self.next_node = next_node
    def reverse_list(self):
        new_list = self
        cur = new_list.next_node
        new_list.next_node = None
        new_list.pre_node = None
        while cur:
            node = cur.next_node
            new_list.pre_node = cur
            cur.next_node = new_list
            new_list = cur
            cur = node
        return new_list
    def print_list(self):
        cur = self
        result = []
        while cur:
            result.append(cur.value)
            cur = cur.next_node
        return result

if __name__ == "__main__":
    head = DoubleLinkedListNode(1, DoubleLinkedListNode(2, DoubleLinkedListNode(3, DoubleLinkedListNode(4, DoubleLinkedListNode(5, None)))))
    print head.print_list()
    new_head = head.reverse_list()
    print new_head.print_list()

Firstly, the line

new_list.pre_node = None

doesn't do anything and can be removed. Secondly, your code can't handle circular double-linked lists.

Circular lists

When called on a circular double-linked list:

Print_list goes into an endless loop.
Reverse_list terminates late, giving a partially reversed list.

Your code could be easily improved to handle these issues. Preventing an endless loop requires keeping track of the identity of each node. Below, I've done this by incrementing a global variable node_count, but there are more Pythonic ways to keep a count of instances of objects.

The unique id is used to track when the cursor id matches self.id, terminating the while loops in print_list and reverse_list at the appropriate time for circular lists.

Lastly, PEP8 specifies methods within a class should be separated by a single line.

See below:

class DoubleLinkedListNode:

    def __init__(self, value, next_node):
        global node_count                         # added unique node id
        self.id = node_count
        node_count += 1
        self.value = value
        self.pre_node = None                      # removed unnecessary line
        if next_node:
            next_node.pre_node = self
        self.next_node = next_node

    def reverse_list(self):
        new_list = self
        cur = new_list.next_node
        new_list.next_node = None
        while cur:
            node = cur.next_node
            new_list.pre_node = cur
                cur.next_node = new_list
            if cur.id == self.id and not node: break    # for circular lists
            new_list = cur
            cur = node
        return new_list

    def print_list(self):
        cur = self
        result = []
        while cur:
            result.append([cur.value])
            next_id = cur.next_node.id if cur.next_node else None
            if next_id == self.id: break               # for circular lists
            cur = cur.next_node
        return result

if __name__ == "__main__":
    node_count = 0
    head = DoubleLinkedListNode(1, DoubleLinkedListNode(2, DoubleLinkedListNode(3, DoubleLinkedListNode(4, DoubleLinkedListNode(5, None)))))
    print(head.print_list())
    new_head = head.reverse_list()
    print(new_head.print_list())

(Bonus: For simple testing, here's my method to make the lists circular. Add it to the DoubleLinkedListNode class.)

    def circularize(self):
        cur = self
        while cur.next_node:
            cur = cur.next_node
        cur.next_node = self

Complexity

Reversing lists, double-linked or not, scales according to \$\mathcal{O}(n)\$, where n is the length of the list.

The only way to speed it up significantly is to make each list an object, with a Boolean direction flag. Any process moving through the list first refers to the direction flag, which either calls next_node or pre_node depending on how it's set. All that's needed to reverse the list is to reverse the direction flag.

Update:

Below is an example of how \$\mathcal{O}(1)\$ reversal of double-linked lists might be implemented using a direction flag. Points relevant to OP's code are:

A dedicated reverse_list method is not really necessary.
An __iter__ method is more useful than a dedicated print_list method (thanks ChatterOne).
Circular lists are also reversed neatly this way (note that __iter__ cycles endlessly for circular lists).

See below:

class DoubleLinkedListNode:
    def __init__(self, value, plus_node, dir=None):
        self.value = value
        self.minus_node = None
        if plus_node:
            plus_node.minus_node = self
            if dir == None:
                self.dir = plus_node.dir
        if dir != None:
            self.dir = dir
        self.plus_node = plus_node

    def __iter__(self):
        self.cur = self
        return self

    def __next__(self):
        if self.cur:
            current = self.cur
            if self.cur.dir:
                if self.cur.dir[0] > 0:
                    self.cur = self.cur.plus_node
                elif self.cur.dir[0] < 0:
                    self.cur = self.cur.minus_node
            return current
        else:
            raise StopIteration

if __name__ == "__main__":
    tail = DoubleLinkedListNode(5, None, [1]) # [1] sets direction for all nodes in list.
    head = DoubleLinkedListNode(1, DoubleLinkedListNode(2, DoubleLinkedListNode(3, DoubleLinkedListNode(4, tail))))
    for i in head: print(i.value)  # Prints node values forwards from head.
    head.dir[0] = -1               # Reverses direction for all nodes in the list.
    for i in tail: print(i.value)  # Prints node values in reverse from tail.


Passing around Connection I have a myCon=getUnusedConnection() method that gets a connection from Connection pool. I also have a releaseConnection(myCon) method to release Connection to the pool after finishing using it.

When coding, I need to select data from the database many times.  I also want to reuse my code.  I want to have many methods for a single action.

Example:

public static List<String[]> getData(){
    Connection myCon=null;
    PreparedStatement preparedStmt=null;
    try{
        myCon=getUnusedConnection();
        String sql="select ........";
        preparedStmt=myCon.prepareStatement(sql);
        ResultSet results=preparedStmt.executeQuery();
        String str="";
        if(results.next()){
             str=results.getString(1);
        }
        if(!str.equals("")){
             List<String[]> list=getData2(myCon, preparedStmt, str);
        }
        return list;
    }
    catch (SQLException ex) {
        while (ex != null) {
            System.out.println ("SQL Exception:  " +
                              ex.getMessage ());
            ex = ex.getNextException ();
        }
    }catch (java.lang.Exception e) {
        System.out.println("***ERROR-->" + e.toString());
    }
    finally{
        releaseConnection (myCon);
        closeStatement (preparedStmt);
    }
    return null;
}

public static List<String[]> getData2(Connection myCon, PreparedStatement preparedStmt, String str){

     try{
         List<String[]> list=new ArrayList<String[]>();
         String sql="c.......";
         preparedStmt=myCon.prepareStatement(sql);
         ResultSet results=preparedStmt.executeQuery();
         while(results.next()){
             list.add(results.getString(1));
         }
         return list;
     }catch (SQLException ex) {
        while (ex != null) {
            System.out.println ("SQL Exception:  " +
                              ex.getMessage ());
            ex = ex.getNextException ();
        }
    }catch (java.lang.Exception e) {
        System.out.println("***ERROR-->" + e.toString());
    }
    finally {

        closeStatement(preparedStmt);
        releaseConnection(myCon);
    }
    return null;
}

Do I need to include try - catch - finally in getData2, since I am passing myCon and prepareStatement around?  I am not sure if this the right way to code.

Is the way I'm coding considered standard?  If not, do you have something better in mind?

There are a number of problems with your code. Let's go through the more important ones:

you should close the PreparedStatement before you return the connection to the pool.... should be:

finally{
    closeStatement (preparedStmt);
    releaseConnection (myCon);
}

you should be closing the ResultSet in addition to the Statement!
but, this is all somewhat pointless because you only use your prepared statement once.... A prepared statement is useful for two reasons only:

they separate issues in syntax from issues in data (you get syntax errors early) ... but, your SQL Query code will not have syntax errors... right?
they improve performance IF THEY ARE REUSED, but you are not reusing them.

If you are using Java 7 you should be using try-with-resources logic on your objects. This will remove the need for the finally block.

JDBC Connection pools are relatively complicated 'animals'. I strongly recommend you leverage the functionality that others have built. For example, Tomcat, WebSphere and I am sure other applications servers have good connection-pooling processes. Additionally, stand-alone applications can (re)use things like the Apache DBCP component (which also supports PreparedStatement connection pools)


Sending DTMF tones from dialpad I have the following code which works perfectly fine. I am using the dial pad to set the number and also to send the DTMF tones. But I find the code to bit more repitive. Is there any better way to optimize the code or the approach I have is good enough ?
Thank you in advance for your valuable suggestions. Cheers :)

public class MainActivity extends AppCompatActivity implements View.OnClickListener {

Button btnZero, btn1, btn2, btn3, btn4, btn5, btn6, btn7, btn8, btn9, btnAsh, btnHash;

@Override
protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);

    setContentView(R.layout.activity_main);

    // Buttons
    btn1 = findViewById(R.id.btnOne);
    btn2 = findViewById(R.id.btnTwo);
    btn3 = findViewById(R.id.btnThree);
    btn4 = findViewById(R.id.btnFour);
    btn5 = findViewById(R.id.btnFive);
    btn6 = findViewById(R.id.btnSix);
    btn7 = findViewById(R.id.btnSeven);
    btn8 = findViewById(R.id.btnEight);
    btn9 = findViewById(R.id.btnNine);
    btnZero = findViewById(R.id.btnZero);
    btnAsh = findViewById(R.id.btnAsterisk);
    btnHash = findViewById(R.id.btnHash);

    dialPadTone();
}

public dialPadTone() {
    btn1.setOnClickListener(this);
    btn2.setOnClickListener(this);
    btn3.setOnClickListener(this);
    btn4.setOnClickListener(this);
    btn5.setOnClickListener(this);
    btn6.setOnClickListener(this);
    btn7.setOnClickListener(this);
    btn8.setOnClickListener(this);
    btn9.setOnClickListener(this);
    btnZero.setOnClickListener(this);
    btnAsh.setOnClickListener(this);
    btnHash.setOnClickListener(this);
}

@Override
public void onClick(View view) {
    // Read current phone dialPadnumber
    String phoneNo = dialPadnumber.getText().toString();
    int value;
    if (btnZero.equals(view)) {
        value = 48;
        phoneNo += btnZero.getText();
    } else if (btn1.equals(view)) {
        value = 49;
        phoneNo += btn1.getText();
    } else if (btn2.equals(view)) {
        value = 50;
        phoneNo += btn2.getText();
    } else if (btn3.equals(view)) {
        value = 51;
        phoneNo += btn3.getText();
    } else if (btn4.equals(view)) {
        value = 52;
        phoneNo += btn4.getText();
    } else if (btn5.equals(view)) {
        value = 53;
        phoneNo += btn5.getText();
    } else if (btn6.equals(view)) {
        value = 54;
        phoneNo += btn6.getText();
    } else if (btn7.equals(view)) {
        value = 55;
        phoneNo += btn7.getText();
    } else if (btn8.equals(view)) {
        value = 56;
        phoneNo += btn8.getText();
    } else if (btn9.equals(view)) {
        value = 57;
        phoneNo += btn9.getText();
    } else if (btnAsh.equals(view)) {
        value = 42;
    } else if (btnHash.equals(view)) {
        value = 35;
    } else {
        throw new IllegalStateException("Unexpected value: " + view.getId());
    }

    setPhoneNum(phoneNo);
    Intent intent = new Intent("send_dtmf_tones");
    Message message = new DTMFtones(
            value
    );
    sendBroadcast(intent);
}
}

Use the android:onClick attribute to get rid of the boilerplate calls to setOnClickListener and findViewById.

The giant if-else statement is very repetitive. The actions taken in all branches are assigning a value to the value variable and appending a digit to the phone number. You could convert the code to data by mapping the button label to a value and a string.

final String label = ((Button) view).getText();
final int value = buttonLabelToValue.get(label);
phoneNo += buttonLabelToPhoneNumberDigit.get(label);

Handle the two special cases '#' and '*' by mapping the PhoneNumberDigit to an empty string.

I never write code to prepare for an UI component calling a wrong listener method. I define the UI and the listeners and if my UI calls the wrong method then it's a bug. My tests must be good enough to catch such scenarios. In this example, not checking the return value of buttonLabelToValue.get(...) for nulls results in NullPointerException instead of the IllegalStateException thrown in your version. Not much difference for the user as the app crashes in both cases.


Inheritance and forcing methods to be called inside another method I've written a text parser that extracts useful information from a given text. Texts are formatted different, so there are different specialized parsers. I've designed them so that they all must define the methods from the interface Parser.

class Parser:
    def parse(text):
        raise NotImplementedError

    def pre_validate(text):
        raise NotImplementedError

    def post_validate():
        raise NotImplementedError

How can I force all subclasses of Parser to call pre_validate before running parse and post_validate after calling parse?

I could solve it like this, but I don't think it's very elegant to wrap methods like this:

class Parser:
    def parse(text):
        self.pre_validate(text)
        self._parse()
        self.post_validate(text)

    def _parse(text):
        raise NotImplementedError

    def pre_validate(text):
        raise NotImplementedError

    def post_validate():
        raise NotImplementedError

Ideally I'd like to allow the subclasses to implement the method parse instead of _parse. What's the recommended way of solving this? Would it make sense to use a python decorator here?

You could achieve that like this:

class Parser:
    def __init__(self):
        self.parse = self._validated_parsing(self.parse)

    def parse(self, text):
        raise NotImplementedError

    def pre_validate(self, text):
        raise NotImplementedError

    def post_validate(self, text):
        raise NotImplementedError

    def _validated_parsing(self, parse_func):
        def parse_wrapper(text):
            self.pre_validate(text)
            parse_func(text)
            self.post_validate(text)
        return parse_wrapper

class ParserChild(Parser):
    def parse(self, text):
        print('parsing')

    def pre_validate(self, text):
        print('pre-validating')

    def post_validate(self, text):
        print('post-validating')

ParserChild().parse('spam')
# pre-validating
# parsing
# post-validating

I recommend doing it with an extra method that is called from parse though, like the _parse method you wrote.  It should not be called directly from outside, so it should be written with a leading underscore.  You might want to want to call it something more explicit, such as (in lack of a better name) _core_parse.

While we're at it, maybe pre_validate and post_validate should be preceded with an underscore too (depending on whether you want them to be called from outside the class).

Instead of raising NotImplementedError in the base class, you should use abc.ABCMeta for an abstract base class.  This is better for two reasons:

You can't create a subclass that doesn't implement all abstract methods.
It will work together properly with the super function which delegates calls to superclasses (base classes).

So, I think I would implement the class like this:

import abc

class Parser(object):
    __metaclass__ = abc.ABCMeta

    def parse(self, text):
        self._pre_validate(text)
        self._core_parse(text)
        self._post_validate(text)

    @abc.abstractmethod
    def _pre_validate(self, text):
        pass

    @abc.abstractmethod
    def _core_parse(self, text):
        pass

    @abc.abstractmethod
    def _post_validate(self, text):
        pass

class ParserChild(Parser):
    def _core_parse(self, text):
        print('parsing')

    def _pre_validate(self, text):
        print('pre-validating')

    def _post_validate(self, text):
        print('post-validating')

ParserChild().parse('spamspam')
# pre-validating
# parsing
# post-validating

(I'm assuming you use Python 3, but if you're using Python 2, you should let Parser inherit from object. In Python 3, this happens automatically.)


search a given word in word file by Aspose I implemented a function to check a MS Word document is contain a given word or not. The function will return true if filePath is contain word. Otherwise it will return false.

This is my solution based on Aspose framework. Is there any better solution?

public class FindContentOfWordDoc
{
    public bool FindContent(string filePath, string content)
    {
        var doc = new Document(filePath);

        var findReplaceOptions = new FindReplaceOptions
        {
            ReplacingCallback = new FindCallBack(),
            Direction = FindReplaceDirection.Backward
        };

        var regex = new Regex(content, RegexOptions.IgnoreCase);
        doc.Range.Replace(regex, "", findReplaceOptions);

        return (findReplaceOptions.ReplacingCallback as FindCallBack)?.IsMatch ?? false;
    }

    private class FindCallBack : IReplacingCallback
    {
        public bool IsMatch { get; private set; }
        ReplaceAction IReplacingCallback.Replacing(ReplacingArgs e)
        {
            IsMatch = true;
            return ReplaceAction.Stop;
        }
    }
}

Thanks!

Some quick remarks:

FindContentOfWordDoc is a bad class name, in the first place because it contains a verb. Please follow the Microsoft rules.

While this code only does a search, it is peppered with names that refer to "find and replace". This makes it hard to understand. I assume this must be due to Aspose, so perhaps this is inevitable, but then perhaps you could add some comments to explain why you implemented it this way.
Are you certain this is the best way to execute a search? I find it odd that you would need to "abuse" the "find and replace" functionality; it seems odd that Aspose would include that functionality yet not a "find" on its own.

You don't check content: is this function only intended for single word queries, or also for combinations of words? And why not check if the user actually entered a search term, in order to avoid invoking the callback etc. on a null or an empty string?


Simple servlet code to sum digits This servlet takes an input from the user and

Stores the number inside the bean but with spaces in between each digit then stores the bean in the request scope.
If the last digit is even, sums the numbers and stores it in the request attribute. It will be displayed by even.jsp. Otherwise, the servlet forwards to odd.jsp and just displays the numbers.

protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
    numberBean bean = new numberBean();
    int sum = 0;
    char[] inputNumbers = request.getParameter("input").toCharArray();
    StringBuilder inputs = new StringBuilder();
    for (char digit: inputNumbers) {
        inputs.append(digit).append(" ");
    }
    bean.setInput(inputs.toString());
    request.setAttribute("input", bean);
    if (inputNumbers[inputNumbers.length - 1] % 2 == 0) {
        for (char digit: inputNumbers) {
            sum += Character.getNumericValue(digit);
        }
        request.setAttribute("sum", sum);
        request.getRequestDispatcher("/WEB-INF/even.jsp").forward(request, response);
    } else {
        request.getRequestDispatcher("/WEB-INF/odd.jsp").forward(request, response);
    }
}

I can't help thinking that this code is too long and can be shorter and better. How can I improve this code?

I am not aware of how to improve the code so that it will be shorter. What I would suggest is that this code can be improved in terms of clarity of intent (in this case, the LOCs will increase). This can be achieved by using small functions and good names for the functions (for more information look and Robert C. Martins wonderful book: Clean Code or this for a little introduction).

I refactored your code by extracting some methods and giving more or less meaningful names:

protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {

    char[] inputNumbers = extractInputNumbersFromRequest(request);

    NumberBean bean = createNumberBeanForInputNumbers(inputNumbers);
    request.setAttribute("input", bean);

    forwardRequestToCorrectView(request, response, inputNumbers);
}

private char[] extractInputNumbersFromRequest(HttpServletRequest request) {
    return request.getParameter("input").toCharArray();
}

private NumberBean createNumberBeanForInputNumbers(char[] inputNumbers) {
    NumberBean bean = new NumberBean();
    String inputNumbersWithSpaces = combineInputNumbersWithSpaces(inputNumbers);
    bean.setInput(inputNumbersWithSpaces);
    return bean;
}

private String combineInputNumbersWithSpaces(char[] inputNumbers) {
    StringBuilder inputs = new StringBuilder();
    for (char digit: inputNumbers) {
        inputs.append(digit).append(" ");
    }
    return inputs.toString();
}

private void forwardRequestToCorrectView(HttpServletRequest request, HttpServletResponse response, char[] inputNumbers) throws ServletException, IOException {
    if (lastNumberIsOdd(inputNumbers)) {
        handleLastNumberOddCase(request, response, inputNumbers);
    } else {
        handleLastNumberEvenCase(request, response);
    }
}

private void handleLastNumberEvenCase(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
    request.getRequestDispatcher("/WEB-INF/odd.jsp").forward(request, response);
}

private void handleLastNumberOddCase(HttpServletRequest request, HttpServletResponse response, char[] inputNumbers) throws ServletException, IOException {
    request.setAttribute("sum", sumOfInputNumbers(inputNumbers));
    request.getRequestDispatcher("/WEB-INF/even.jsp").forward(request, response);
}

private int sumOfInputNumbers(char[] inputNumbers) {
    int sum = 0;
    for (char digit: inputNumbers) {
        sum += Character.getNumericValue(digit);
    }
    return sum;
}

private boolean lastNumberIsOdd(char[] inputNumbers) {
    return inputNumbers[inputNumbers.length - 1] % 2 == 0;
}

One could argue about the existence about some of the methods like handleLastNumberEvenCase, but I like the idea from Uncle Bob, that a method should not have different levels of abstractions.  As such, I've introduced these alias-like methods, which just give a one liner a name, that is more clear.

In addition, the class numberBean was renamed to NumberBean to use the standard conventions in the Java world. Furthermore, it should be renamed to a more meaningful name, because the word "Number" does not give you a hint on what this class does. The word "Bean" does not give you further information as well, so I would suggest something like WhitespaceNumberRequestor - which is obviously false, because I don't have any hints on what this class does.


A controller on a .net core API BACKGROUND

I am working on a .Net Core API to drive a lyrics application. Users can sign up, submit artists & lyrics, and earn kudos / XP points in the process. Basically a community-driven lyrics website.

CODE

I have the following ArtistsController.

[Route("api/artists")]
public class ArtistsController : Controller
{
    private readonly IArtistsService _artistsService;
    private readonly UserManager<BbUser> _userManager;

    public ArtistsController(IArtistsService artistsService, UserManager<BbUser> userManager)
    {
      _artistsService = artistsService ?? throw new ArgumentNullException(nameof(artistsService));
      _userManager = userManager;
    }

    [HttpGet]
    public async Task<IActionResult> GetArtists()
    {
      ArtistCardDtoCollection artistCardDtoCollection;
      if (User.Identity.IsAuthenticated)
      {
        var username = HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);
        var user = await _userManager.FindByNameAsync(username);
        var roles = await _userManager.GetRolesAsync(user);
        artistCardDtoCollection = _artistsService.GetAllArtists(user.Id, roles.Contains("Admin"));
      }
      else
      {
        artistCardDtoCollection = _artistsService.GetAllArtists("", false);
      }

      return Ok( new { artists = artistCardDtoCollection });
    }

    [HttpGet("{slug}")]
    [HttpGet("{slug}/lyrics", Name = "GetArtist")]
    public async Task<IActionResult> GetArtist(string slug)
    {
      if (!_artistsService.ArtistExists(slug)) return NotFound();
      ArtistOverviewDto artistOverviewDto;
      if (User.Identity.IsAuthenticated)
      {
        var username = HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);
        var user = await _userManager.FindByNameAsync(username);
        var roles = await _userManager.GetRolesAsync(user);
        artistOverviewDto = _artistsService.GetArtistBySlug(slug, user.Id, roles.Contains("Admin"));
      }
      else
      {
        artistOverviewDto = _artistsService.GetArtistBySlug(slug, "", false);
      }

      return Ok(artistOverviewDto);
    }

    [Authorize]
    [HttpPost]
    public async Task<IActionResult> CreateArtist([FromBody] ArtistCreateDto artistCreateDto)
    {
      if (artistCreateDto == null) return BadRequest();
      if (!ModelState.IsValid) return BadRequest(ModelState);
      var username = HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);
      var user = await _userManager.FindByNameAsync(username);
      var artistOverviewDto = await _artistsService.AddNewArtist(user.Id, artistCreateDto);
      if (artistOverviewDto == null) return NotFound();
      return CreatedAtRoute("GetArtist", new {artistOverviewDto.Slug}, artistOverviewDto);
    }
}

I have 3 methods. One to get a list of artists, one to get a single artist and finally, one to create a new artist (you need to be logged in for that one).

How would you improve the code above? Readability of code is very important to me, and right now I don't feel it particularly reads well.

Here I will discuss only about your API, I think something may also be changed in your domain model but I don't see enough code to suggest anything really meaningful.

You have some code duplication you can avoid, extract this:

private async (string UserId, IEnumerable<string> Roles) GetCurrentUserAndRole()
{
    if (!User.Identity.IsAuthenticated)
        return ("", Enumerable.Empty<string>());

    var username = HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);
    var user = await _userManager.FindByNameAsync(username);
    var roles = await _userManager.GetRolesAsync(user);

    return (user.id, roles);
}

Let's introduce an helper method to determine if user is an administrator:

private static bool IsAdministrator(IEnumerable<string> roles)
    => roles.Contains("Admin");

Now API methods might be simplified:

[HttpGet]
public async Task<IActionResult> GetArtists()
{
  (var userId, var roles) = await GetCurrentUserAndRole();
  var artists = _artistsService.GetAllArtists(userId, IsAdministrator(roles));

  return Ok(new { artists = artists });
}

Exactly same thing may be applied to all the other methods. Note that names can be simplified, the list of artists does not need to be artistCardDtoCollection when simply artists is enough.

Now we have a problem. I don't know how you setup your tests but I usually like to test different things in different layers, controllers are easy to test if I can simply create an instance of an object, call a method and assert about its result. In this way I can verify all the controller logic (that should be pretty small) in simple unit testing and then check for HTTP related stuff in integration tests (slower and often tedious to write). Can I do it here? No, because GetCurrentUserAndRole() needs HttpContext and User and they're not available when I manually created an instance of ArtistsController. It's also hard to try this logic because I need the whole authentication thing to try both code paths. Let's move this logic into a separate IPermissionsService (or similar name). You get an instance of this service from your favorite DI framework and you can mock it as you prefer:

[HttpGet]
public async Task<IActionResult> GetArtists()
{
  var permissions = await _permissionsService.GetPermissions(HttpContext);

  var artists = _artistsService.GetAllArtists(permissions.UserId,
      permissions.IsAdministrator);

  return Ok(new { artists = artists });
}

Now you can test it:

[TestMethod]
public async void IfUserIsAdministratorThenXyz()
{
    var artistsService = ...;
    var userManager = ...;
    var permissionsManager = new MockPermissionsManager(...);

    var controller = new ArtistsController(artistsService,
        userManager, permissionsManager);
    var response = await controller.GetArtists();

    // Check result and HTTP status code...
}

This is not the only (or the best) way to solve the problem but definitely something you should think about.

We're creating an anonymous object here, I strongly suggest to add a proper model object such as ArtistListModel. In .NET Core default naming conversion
 for serialized JSON objects changed, now you can name properties with PascalCase and they get serialized in camelCase.

When you create a new artist and AddNewArtist() fails you're returning 404 Not Found, it's not exactly the expected HTTP status code if something went wrong.

You're not catching any exception. I do not think that this code won't ever fail in any circumstance (and a generic 500 Server Error is somehow unprofessional if it can be avoided with a more meaningful error message).

To create an object you might consider to use PUT instead of POST. I say consider because it's an opinionated guideline which I won't discuss here. Also a redirect as result of an API call isn't completely clear to me, leave this design decision to Presentation. An API call should return 201 Created and caller has the responsibility to read the object content (if interested); this is presentation logic, not domain logic. If you want to inform the client about the location of the newly created object then you can manually add Location HTTP header in your response (even if it has been originally intended to be served only with 3xx responses, it is somehow common in API interface):

201 Created
Location: http://example.com/api/v1/artists/1234

As I said I won't discuss these issues here but someone suggests to return resource content in the body of the request when returning 201 Created (in this case you do not even need to add Location header) or an empty body when returning 202 Accepted. Think about it and stick to one convention everywhere.

For documentation purposes I'd also add [ProducesResponseType] attributes to your API methods. It will help you to understand the code later and it'll make automatically generated documentation (for example with Swagger) much more complete and useful.


Metalsmith plugin for stripping UTF8 BOM from files I've been developing a metalsmith static site and I came across an issue where Visual Studio was automatically adding a BOM to the pages.

I wrote the following plugin for metalsmith (it needs to be run as the first plugin). This feels a little messy. It was written mostly as a copy-paste and a lot of debugging.

var stripBom = require('strip-bom');
var front = require('front-matter');
var extend = require('extend');

// **snip**

.use(function __utf8BOM_workaround(files, metalsmith, done)
{
    setImmediate(done);
    Object.keys(files).forEach(function (file)
    {
        var data = files[file];
        var parsed = front(stripBom(data.contents.toString()));
        data = extend({}, data, parsed.attributes);
        data.contents = new Buffer(parsed.body);

        files[file] = data;
    });
})

As @konijn said in the comments, it looks pretty OK, though I do have some comments. I don't know anything about Metalsmith specifically, so it'll just be more general JS/Node comments.

Firstly, it looks like you're doing two different things here:

Stripping the BOM, of course, which is indeed what the function claims to do
Parsing some the file, and extracting some attributes.

I don't know the exact structure of the input for this function, but each "file" object seems to contain a contents buffer. Question is whether it also contains other attributes already, or if they only get added by this function.

If it's the latter, then the plugin is really two things, which doesn't smell quite right. I'd probably break it into two plugins: Strip the BOM somewhere, then parse the attributes somewhere else. I imagine you need to strip the BOM before anything else, but beyond that, those two operations sound independent to me.

I can see the appeal of keeping it in the same place though, since splitting it up requires and extra toString and buffer replacement per file. But the overhead is, I'd wager, completely negligible.

The alternative would be to call the plugin something like utf8SafeParsing (or something like that), just to indicate that its main function is to parse - compared to that, the BOM-stripping seems like a minor thing.

Besides that I can only think minor improvements.

Firstly, there are konijn's two observations: A comment about why this plugin is used (why the BOM is a problem, and/or where it comes from), and the call to setImmediate(done) before actually being done. The latter probably works just fine as there's nothing asynchronous going on in the function, but nevertheless seems odd to call it before doing anything.
I always lobby for using brace-on-same-line style in JavaScript, only because putting the brace after a linebreak can in some (uncommon) circumstances cause JS auto-insert a semicolon at the linebreak, causing all sorts of confusion. In other words, while whitespace shouldn't matter, it kinda sorta does, and the interpreter kinda sorta assumes that you're using brace-on-same-line, and will attempt to "fix" your code otherwise.
You can move your require calls into the function itself, to keep things neat and tidy (requires are cached, so there's no performance hit).
If you keep it as one function, you could add a little helper function or two for yourself. No difference in logic, just structure.
Super trivial thing; I'd name the function stripBOM rather than mix-case the acronym. If the named started with "BOM", then it should be lowercase, but acronyms in JS names are typically kept uppercase (e.g. innerHTML in browsers). But again, this nit-picking in extenso.

Again, though, the code seems quite fine as-is. It's readable, and if you say it works, then great.

Here are some refactoring attempts with really, really minor changes.

2 plugin solution:

// **snip**

.use(function stripUTF8BOM(files, metalsmith, done) {
    var stripBOM = require('strip-bom');

    Object.keys(files).forEach(function (file) {
        var data = files[file],
            parsed = stripBOM(data.contents.toString());

        files[file].contents = new Buffer(parsed.body);
    });

    setImmediate(done);  // or, presumably, just call `done();` directly
})
.use(function parseAttributes(files, metalsmith, done) {
    var front = require('front-matter'),
        extend = require('extend');

    Object.keys(files).forEach(function (file) {
        var data = files[file],
            parsed = front(data.contents.toString());

        files[file] = extend({}, parsed.attributes, {
          contents = new Buffer(parsed.body);
        });
    });

    setImmediate(done); // or, presumably, just call `done();` directly
})

Single plugin solution:

// **snip**

.use(function utf8SafeParsing(files, metalsmith, done) {
    var stripBOM = require('strip-bom'),
        front = require('front-matter'),
        extend = require('extend');

    function parse(buffer) {
      var sanitized = stripBOM(buffer.toString());
      return front(sanitized);
    }

    Object.keys(files).forEach(function (file) {
        var parsed = parse(files[file].contents);

        files[file] = extend({}, parsed.attributes, {
          contents = new Buffer(parsed.body);
        });
    });

    setImmediate(done);  // (see above)
})


Formatting the Lengthy Parameters in a better way Should I have made a line break convert_to_quick_mode( and from_exported_cdf,

        convert_to_quick_mode(
          from_exported_cdf,

code:

class QuickModeConverter(object):
    def run(self,
            from_exported_cdf,
            from_exported_msword,
            to_export_quick_mode_cdf,
            to_export_quick_mode_msword
            ):

        convert_to_quick_mode(
          from_exported_cdf,
          from_exported_msword,
          to_export_quick_mode_cdf,
          to_export_quick_mode_msword
        )
        pass

Best option would be to just use *args (or **kwargs if order does not matter), as follows:

class QuickModeConverter(object):
    def run(self, *args):
        convert_to_quick_mode(*args)