Summation and product functions using functional paradigm Below is the problem:

  Show that both summation and product are instances of a more general function, called accumulate, with the following signature:

def accumulate(combiner, start, n, term):
    """Return the result of combining the first n terms in a sequence."""
    "*** YOUR CODE HERE ***"

  Accumulate takes as arguments the same arguments term and n as summation and product, together with a combiner function (of two arguments) that specifies how the current term is to be combined with the accumulation of the preceding terms and a start value that specifies what base value to use to start the accumulation. Implement accumulate and show how summation and product can both be defined as simple calls to accumulate:

def summation_using_accumulate(n, term):
    """An implementation of summation using accumulate.

    >>> summation_using_accumulate(4, square)
    30
    """
    "*** YOUR CODE HERE ***"

def product_using_accumulate(n, term):
    """An implementation of product using accumulate.

    >>> product_using_accumulate(4, square)
    576
    """
    "*** YOUR CODE HERE ***"

For which, solution is mentioned below:

from operator import add
from operator import mul

def square(x):
    """ Retrun x squared"""
    return x * x

def accumulate(combiner, start, n, term):
    """Return the result of combining the first n terms in a sequence."""
    if n == 0:
        return start
    else:
        start = combiner(term(n), start)
        return accumulate(combiner, start, n-1, term)

def summation_using_accumulate(n, term):
    """An implementation of summation using accumulate.

    >>> summation_using_accumulate(4, square)
    30
    """
    assert n >= 0
    return accumulate(add, 0, n, term)

def product_using_accumulate(n, term):
    """An implementation of product using accumulate.

    >>> product_using_accumulate(4, square)
    576
    """
    assert n > 0
    return  accumulate(mul, 1, n, term)

result =  summation_using_accumulate(4, square)
print(result)
result = product_using_accumulate(3, square)
print(result)

Please let me know if this program can be improved further using functional paradigm and the DRY principle.

The start parameter of accumulate is a bit misleading.
My first thought was that start is the first element of the sequence,
but it's an accumulated value.
A more common name is acc or accum.

Reassigning function parameter is considered a bad practice,
in any language as far as I remember.
It's basically reusing an input variable for one additional purpose.
start is an input to your function,
but when you reassign it, it becomes something else:
instead of being just the input,
now it carries the result of a calculation.
When you look at your function body,
the variable start is sometimes the input,
sometimes the result of a local calculation.
This duality in purpose can be confusing and can lead to errors.
It's cleaner to assign the mutated value to a completely new local variable.

In this version, acc always has the same value,
there can be no confusion and misuse, it's perfectly clear:

def accumulate(combiner, acc, n, term):
    """Return the result of combining the first n terms in a sequence."""
    if n == 0:
        return acc
    else:
        next_acc = combiner(term(n), acc)
        return accumulate(combiner, next_acc, n-1, term)

The benefit may not be obvious in this simple function,
but I hope you see the truth in the principle of not reusing parameter variables.
In a longer, more complicated function the benefits would become more clear.

In languages that have out-parameters,
re-assignment of parameters forces me to double-check the signature.
Although Python doesn't have out-parameters,
I still review code like this with suspicion,
in case the author may have mistakenly believed that the re-assigned value will produce a side effect outside the function.
This may sound a bit paranoid, but I've seen this kind of thing happen more than once.
By not re-assigning parameters,
all suspicions are cleared, and the code is easier to review.

For the record, I was tempted to suggest to drop the else clause there,
to simplify to:

def accumulate(combiner, acc, n, term):
    """Return the result of combining the first n terms in a sequence."""
    if n == 0:
        return acc
    next_acc = combiner(term(n), acc)
    return accumulate(combiner, next_acc, n-1, term)

But then, most functional languages return the last evaluation,
and explicit return statements are not recommended,
which makes if-else necessary.
For example the Scala solution would look something like this:

def accumulate(combiner, acc, n, term): Int = {
    if (n == 0) {
        acc
    } else {
        val nextAcc = combiner(term(n), acc)
        accumulate(combiner, nextAcc, n-1, term)
    }
}

So using the explicit else clause is closer to the spirit of functional programming, so I'd leave that as you did.


Reduce or improve Linq query with nested from-where clauses I am new to linq to objects in C#. I have the following query which works adequately. Is there someway to reduce or rewrite this query using joins or something else?

var query =
        from container in containers
        let items = container.GetItems()
            from itemA in items
            let tag = CreateTag(itemA)
            where IsInteresting(tag)
                from itemB in items
                let derivedTag = CreateDerivedTag(tag)
                where CreateTag(itemB) == derivedTag
                select new { first = itemA, second = itemB };

To summarize,

there are a set of item sequences
for each item sequench

for each item A that is IsInteresting()

find an item B in the same sequence that matches based on the criteria derived from the item A
create a new item containing a reference to matching items A and B

return a collection of all such matching items

After a lot of setup, I arrived at this:

var extensionMethodQuery =
    from container in containers
    let items = container.GetItems()
    select items.GetItemDerivedTagItems
        (items.SelectInterestingItemTags()).FirstOrDefault();

Your original query looks pretty optimal, but it took a while to see why.  I broke it down into its component parts, and concluded that I'd concentrate on making it more reusable and readable.  Personally, I find lambda joins too verbose, so created two extension methods instead:

public static class Filters
{
    public static IEnumerable<ItemTag> SelectInterestingItemTags
        (this IEnumerable<Item> items)
    {
        return
            from item in items
            let tag = Tag.CreateTag(item)
            where Tag.IsInteresting(tag)
            select new ItemTag(item, tag);
    }

    public static IEnumerable<ItemDerivedTagItem>
        GetItemDerivedTagItems
        (this IEnumerable<Item> items, IEnumerable<ItemTag> itemTags)
    {
        return
            from itemTag in itemTags
            from item in items
            let derivedTag = Tag.CreateDerivedTag(itemTag.tag)
            where Tag.CreateTag(item) == derivedTag
            select new ItemDerivedTagItem(itemTag.item, item);

    }
}

You'll notice I needed to create two extra classes to enable this, ItemTag and ItemDerivedTagItem, that replace the anonymous classes.  Those names are probably awful, but not knowing the context, I did the best I could.  The IsInteresting and CreateDerivedTag methods were made static member methods of their classes, too.  The results were compared successfully via unit tests, against my dummy classes and dummy data, so hopefully you can verify that with yours.

As ever, the proof is in the profiling, and for me at least, the extension method query is 4 times faster.  I can supply the dummy classes and data I used if needed, but as mentioned in the comments, I'd much prefer those supplied next time please!


Remove duplicate chars from String I have retackled this problem using the help I received from here: Remove duplications from a Java String but this time using a LinkedHashSet since previosuly I was using a HashSet but the answer was out of order.

From this implementation my run time should be \$O(n)\$ correct?
Does anyone see any room where I can improve my code or some mistakes?

public static void main(String[] args){
    String test = "Banana";
    LinkedHashSet knownChars = new LinkedHashSet();
    StringBuilder noDups = new StringBuilder();

    for(Character c : test.toCharArray()){
        if(!knownChars.contains(c)){
            knownChars.add(c);
            noDups.append(c);
        }
    }
    System.out.println("No duplicate string is: " + noDups);
}

The below points are in no particular order:
Extraction into Methods

The utility and the test should be broken up into methods, i.e., you should have a function String removeDuplicates(String), containing the logic for removing duplicates.

Manual Boxing

Unnecessary manual boxing to Character, just use char and let javac take care of it on its own (JDK1.5+ supports autoboxing of primitives).

Why LinkedHashSet?

A HashSet can be used just fine here - the order of the resulting deduplicated String is determined by the order of insertion of characters into the StringBuilder, not the HashSet. (Believe me, I checked. Here you can also: http://ideone.com/K9Ku2p)

Note that the add method of Sets return a boolean, true if the set did not already contain the element and it has been added successfully, or false if the element was already present in the Set. Exploiting this makes the call to Set.contains(...) redundant, see the example code.

Generics

Use generics. Don't use raw collections - they can violate type safety. In your case, you might not realise the immediate benefit of doing so, but it is a good practice when scaling to larger programs. Here, using generics is as simple as changing LinkedHashSet knownChars = new LinkedHashSet(); to LinkedHashSet<Character> knownChars = new LinkedHashSet<>(); (JDK 1.7+ to get the diamond type inference, otherwise it has to be LinkedHashSet<Character> knownChars = new LinkedHashSet<Character>();, JDK 1.5+)

Space-time tradeoffs

To minimize the number of reallocations of the underlying buffers of StringBuilder or HashSet, initialize them with a default capacity of the largest possible size they could have, which is the length of the input String. Use the constructors which have an int capacity parameter. See the example code for details.

To avoid a gotcha involving the load factor (a parameter which decides how full a HashSet should be before it is resized) of the HashSet when initializing the HashSet with capacity in point 6 (the Hashset may be prematurely resized), also set the load factor to 1.0f, using the new HashSet(int capacity, float loadFactor) constructor overload.

Miscellaneous

Type to interfaces, e.g., use Set<Character> knownChars = new LinkedHashSet<>(); instead of LinkedHashSet<Character> knownChars = new LinkedHashSet<>();. This makes your code in general more resilient to refactoring, you can use a different Set implementation at any time by changing one word instead of 2.

Qualify your method parameters with final if you are not going to reassign them in any way - granted, String being immutable makes this redundant, in the sense that any reassignments done to input in removeDuplicates will not affect test in main, but it's a good practice anyway.

Better output messages - see the example code for an example.

Better variable naming - it's already quite good, but try to use full words. See the example code.

Example Code (Ideone):
import java.util.Set;
import java.util.HashSet;

// Store in a file `StringUtilities.java`
public class StringUtilities
{

    public static void main(String[] args)
    {
        String test = "Banana";

        System.out.println("Test string \"" + test + "\" with duplicates removed is: \"" + removeDuplicates(test) + "\"");
    }

    public static String removeDuplicates(final String input) {
        Set<Character> knownCharacters = new HashSet<>(input.length(), 1.0f);
        StringBuilder noDuplicates = new StringBuilder(input.length());

        for(char character : input.toCharArray()){
            if(knownCharacters.add(character)){
                noDuplicates.append(character);
            }
        }
        return noDuplicates.toString();
    }
}


Enabling/Disabling button on a certain condition I am using a Telerik Datetime control. On onBlur event of input box I am checking if the date is valid/in-valid, based upon that I am enabling or disabling the submit button (anchor tag actually).

var embedUrl = null;
function onBlur(sender) {
    var constants = {
        classDisabled: 'button-disabled',
        emptyHref: "javascript:void(0)"
    };
    if (sender) {
        var jqueryButton = window.jQuery('.button-submit');
        if (!embedUrl) {
            embedUrl = jqueryButton.href;
        }
        // Check if the string is valid date.
        // As it's being triggerd from Telerik so we use _invalid
        if (sender._invalid) {
            // Date is in-valid, add the class disabled if it does not exists.
            if (!jqueryButton.hasClass(constants.classDisabled)) {
                jqueryButton.attr('href', constants.emptyHref);
                jqueryButton.addClass(constants.classDisabled);
            }
        } else {
            // Date is valid, remove the class disabled if already exists.
            if (jqueryButton.hasClass(constants.classDisabled)) {
                jqueryButton.attr('href', embedUrl);
                jqueryButton.removeClass(constants.classDisabled);
            }
        }
    }
}

I have an anchor tag which has some href which I need to persist.

I want to know how I can write this code a little cleaner.

Some things that crossed my mind:

Your'e polluting the global namespace with embedUrl. Wrap your code in an IIFE, or make use of jQuery's data() method to pass around "global" variables.

IIFE example:

(function($, window, undefined){

    var embedUrl = null;

    // Do whatever you want in this scope without affecting window namespace
    // $ === jQuery, but you can still access the jQuery variable as well
    // window is passed for sligtly faster access
    // undefined is passed as an undefined argument to make sure it's
    // truly undefined (can be modified pre ES5)

})(jQuery)

// console.log(embedUrl); // undefined

jQuery's data example:

// Set data:
jQuery('.button-submit').data({ foo: 'bar' });

// Get data:
var foo = jQuery('.button-submit').data('foo');
console.log(foo); // "bar"

* I prefer Hungarian notation on jQuery objects to improove readbility i.e. change

var jqueryButton = window.jQuery('.button-submit');

to

var $button = jQuery('.button-submit');

In your example, jqueryButton is a jQuery object, so you can't do jqueryButton.href. If you want to access the href property, use the prop() method:

var href = $button.prop('href');

However, if you extract the selected DOM element from your jQuery object, your code would have worked:

var button = $button.get(0), // or $button[0]
    href = button.href;

Use prop() instead of attr(), unless you're using a really old jQuery version.
More info here.

You can use toggleClass() with a switch for your class toggling:

jqueryButton.toggleClass(constants.classDisabled, sender._invalid);

And set the href like this:

jqueryButton.prop('href', sender._invalid ? constants.emptyHref : embedUrl);

Just my two cents.


Implement search for JPA and Spring I'm trying to implement properly search functionality for database table. I tried this approach:

Controller:

    @GetMapping
    public Page<TransactionDTO> find(TransactionFilterDTO filter, Pageable page) {
        return searchRepository
                .findTransactionsByFilter(mapper.toFilter(filter), page)
                .map(mapper::toDTO);
    }

Filer DTO:

public class TransactionFilterDTO {

    private String name;

    private Integer id;

    @DateTimeFormat(iso = DateTimeFormat.ISO.DATE_TIME)
    private LocalDateTime from;

    @DateTimeFormat(iso = DateTimeFormat.ISO.DATE_TIME)
    private LocalDateTime to;
    ... // getters and setter
}

Search implementation:

@Repository
public class TransactionSearchRepositoryImpl implements TransactionSearchRepository {

    @Autowired
    private TransactionRepository transactionRepository;

    @Autowired
    private TransactionSpecification specification;

    @Override
    public Page<Transaction> findTransactionsByFilter(TransactionFilter filter, @Nullable Pageable page) {

        List<Transaction> transactions = transactionRepository
                .findAll(specification.getFilter(filter));

        int totalCount = transactions.size();

        if(page != null) {
           transactions = transactions
                   .stream()
                   .skip(page.getOffset())
                   .limit(page.getPageSize())
                   .collect(Collectors.toList());
        }

        return new PageImpl<>(transactions, page, totalCount);
    }
}

Repository:

public interface TransactionSearchRepository {

    Page<Transaction> findTransactionsByFilter(TransactionFilter filter, Pageable page);
}

Is there some better way to implement a search functionality? This solution is very ugly in my view.

In JPA you could use JpaSpecificationExecutor<T> in repository, with that you could utilize it's method.

TransactionRepository

public interface TransactionRepository extends JpaRepository<Transaction, Long>,
                                                JpaSpecificationExecutor<Transaction> {

}

If you look at the source of JpaSpecificationExecutor it has,

Page< T > findAll(@Nullable Specification<T> spec, Pageable pageable);

With that you just pass the specification and Pageable to return what you expected.

Specification

@AllArgsConstructor
class TransactionSpecification implements Specification<Transaction> {

       private TransactionFilter transactionFilter;

       @Override
        public Predicate toPredicate(Root<DataImport> root, CriteriaQuery<?> query, CriteriaBuilder criteriaBuilder) {
            // You could add multiple Predicates based on the transactionFilter
            return criteriaBuilder.equal(root.get("table_name"), "value");
        }
}

Controller

    @Autowired
    private TransactionRepository transactionRepository;

    @GetMapping
    public Page<TransactionDTO> find(TransactionFilterDTO filter, Pageable page) {
        TransactionFilter transactionFilter = mapper.toFilter(filter);
        return transactionRepository.findAll(new TransactionSpecification(transactionFilter), page);
    }

I am pretty late answering but you could give it a try


How to simply express a desire to run a function when two properties are true? require([
    'jquery',
    'underscore',
    'backbone'
], function ($, _, Backbone) {
    'use strict';

    //  TODO: Would like to access through define module, but not sure how..
    var player = chrome.extension.getBackgroundPage().YouTubePlayer;
    var user = chrome.extension.getBackgroundPage().User;

    //  If the foreground is opened before the background has had a chance to load, wait for the background.
    //  This is easier than having every control on the foreground guard against the background not existing.
    if (user.get('loaded')) {
        loadForeground();
    } else {
        user.on('change:loaded', function (model, loaded) {
            if (loaded) {
                loadForeground();
            } else {
                //  Show refreshing message?
            }
        });
    }

    function loadForeground() {

        if (player.get('ready')) {
            //  Load foreground when the background indicates it has loaded.
            require(['foreground']);
        } else {

            player.once('change:ready', function () {

                //  Load foreground when the background indicates it has loaded.
                require(['foreground']);
            });
        }

    }
});

My goal is to call require(['foreground']) once the user object is loaded and the player is ready. This code works fine, but seems really unclear / overly verbose.

Am I missing an easy simplification in trying to express this need?

jQuery.Deferred, or even just promises in general, would make your code simpler. In short, a promise can be unfulfilled, fulfilled, or failed. jQuery.when lets you compose multiple promises into one promise that's fulfilled when they're all fulfilled or failed as soon as one fails. This means that you can

create Deferreds for each event you want to monitor:

var userLoaded = new $.Deferred();
var playerReady = new $.Deferred();

Resolve the promises when whatever event they represent has completed, e.g.:

userLoaded.resolve();

If there's an error, you can signal that if you want as well:

userLoaded.reject();

Compose them together:

$.when(userLoaded, playerReady).then(function() {
    require(['foreground']);
});

If you pass a second argument to then, it will be called if either of the promises are rejected, simplifying error-handling, should you wish to do that.


Using Autofac to do DI in a static factory method I'm using Autofac to create one of my classes inside of a static factory method Create. This code is working and I haven't noticed any side effects yet. Admittedly, I've only used it in unit-tests so far but I'm wondering whether there could be any issues with it later when I take it into production code?

public static ICommandLineExecutor Create(ILogger logger)
{
    var containerBuilder = new ContainerBuilder();

    containerBuilder
        .RegisterType<CommandLineTokenizer>()
        .As<ICommandLineTokenizer>();

    containerBuilder
        .RegisterType<CommandLineParser>()
        .As<ICommandLineParser>();

    containerBuilder
        .RegisterType<CommandParameterFactory>()
        .WithParameter(new TypedParameter(typeof(TypeConverter), CommandParameterFactory.DefaultConverter))
        .As<ICommandParameterFactory>();

    containerBuilder
        .RegisterType<CommandLineExecutor>()
        .WithParameter(new TypedParameter(typeof(ILogger), logger))
        .As<ICommandLineExecutor>();

    using (var container = containerBuilder.Build())
    using (var scope = container.BeginLifetimeScope())
    {
        return scope.Resolve<ICommandLineExecutor>();
    }
}

What are you registering CommandLineTokenizer etc for? this whole code block is equivalent to

return new CommandLineExecutor(logger);

If all these types are somehow used after all (inside CommandLineExecutor .ctor?), remember that everything created  Per LifeTime scope will be disposed at exit out of using and everything created  Per Dependency scope will become your responsibility to Dispose.


Grid Overlay For Browser Using HTML5 Canvas I wanted to create a grid overlay for my browser that I can use when building websites. I decided on HTML5 canvas because it allows me to create lots of lines without adding loads of elements to the DOM.

The code is below and you can demo it on GitHub pages here. I'd appreciate your guys feedback on the following areas:

How can I make the code more efficient/faster/lighter?
Do you think this grid is accurate, or will it be of any use?

These are the outstanding issues I currently want to address:

Need to add a max and min value (don't want grid squares of 1 pixel)
Need a "clear grid" button (still deciding the best way to implement it, does the "show grid" button change text when grid is visible?
Prevent non-numeric input

CODE:

window.onload = init;

function init() {

// grid controls
var controls = getById('gridControls');
controls.style.position = 'absolute';
controls.style.right = "30px";
controls.style.padding = "10px";
controls.style.backgroundColor = "#ffffff";
controls.style.borderLeft = "1px solid #aaa";
controls.style.borderRight = "1px solid #aaa";
controls.style.borderBottom = "1px solid #aaa";
controls.style.zIndex = '100001';

// get our canvas
var grid = getById('grid');
hide(grid);

// get the button to show grid
var button = getById('showGrid');
button.onclick = drawCanvas;
}

function drawCanvas() {

var horizontalSpace = 0;
var verticalSpace = 0;

// input for setting the grid squares
var input = getById('widthHeight');
var gridSize = parseInt(input.value);

show(grid);

var context = grid.getContext('2d');

// set the canvas style to positions absolute
// and set z-index high to keep it on top
grid.style.position = 'absolute';
grid.style.zIndex = '100000';

// get window width and height
var w = window.innerWidth;
var h = window.innerHeight;

// set the canvas width and height to be the window w/h
grid.width = w;
grid.height = h;

// create the horizontal lines
for(var horizontal = 0; horizontal < w; horizontal++) {
    context.beginPath();
    context.moveTo(0, horizontalSpace);
    context.lineTo(w, horizontalSpace);
    context.lineWidth = 1;
    context.strokeStyle = "lightblue";
    context.stroke();
    horizontalSpace += gridSize;
}

// create the vertical lines
for(var vertical = 0; vertical < h; vertical++) {
    context.beginPath();
    context.moveTo(verticalSpace, 0);
    context.lineTo(verticalSpace, h);
    context.lineWidth = 1;
    context.strokeStyle = "lightblue";
    context.stroke();
    verticalSpace += gridSize;
}

}

function hide(element) {
    element.style.display = 'none';
}

function show(element) {
    element.style.display = 'block';
}

function getById(id) {
    return document.getElementById(id);
}

You can use cssText to update the style properties all in one go.

This avoids the element being repainted after every style prop change.

i.e. var myStyleString = 'right: 30px'; // etc

getById('gridControls').style.cssText = myStyleString;

This way you also don't need the controls var.

you can read more about style here.


A method that calls multiple async tasks with error handling, done the right way? I've got a method, CheckForValue(), that uses named pipes to send a message to another process running locally, and then receive a message from that process, returning a bool indicating if it is equivalent with some value.
I'm concerned that I am not handling CancellationTokens and errors correctly. If there any errors (or timeouts), I just want this method to return false.
Again, I am not concerned with different methods of IPC, or using a single duplex named pipe, etc. My concern is the error handling and the usage of CancellationTokens here.
CheckForValue
public async Task<bool> CheckForValue()
{
    int timeout = 300; //300ms should be plenty of time
    try
    {
        using (var getValueCancellationTokenSource = new CancellationTokenSource())
        {
            var valueTask = GetValueUsingNamedPipe(getValueCancellationTokenSource);
            using (var timeoutCancellationTokenSource = new CancellationTokenSource())
            {
                var completedTask = await Task.WhenAny(valueTask, Task.Delay(timeout, timeoutCancellationTokenSource.Token));
                if (completedTask == valueTask)
                {
                    if (timeoutCancellationTokenSource.Token.CanBeCanceled)
                    {
                        timeoutCancellationTokenSource.Cancel();
                    }

                    var result = valueTask.Result;
                    return (result == "WhatIWant");
                }

                if (getValueCancellationTokenSource.Token.CanBeCanceled)
                {
                    getValueCancellationTokenSource.Cancel ();
                }

                return false;
            }
        }

    }
    catch (Exception)
    {
        return false;
    }
}

GetValueUsingNamedPipe
public async Task<string> GetValueUsingNamedPipe(CancellationTokenSource ct)
{
    var response = "";
    try
    {
        Task sendMsg = SendMessage ("MyMessage", ct);
        await sendMsg;
        response = await Listen(ct);
    }
    catch (Exception)
    {
        return "";
    }
    return response;
}

SendMessage
public async Task SendMessage(string message, CancellationTokenSource ct)
{
    try
    {
        using (var _pipeClientStream = new NamedPipeClientStream(".", "MyPipe", PipeDirection.Out, PipeOptions.Asynchronous))
        {
            await _pipeClientStream.ConnectAsync (1000, ct.Token);

            var writer = new StreamWriter (_pipeClientStream) { AutoFlush = true };
            await writer.WriteLineAsync (message);
            await writer.WriteLineAsync (MessageFooter);
        }
    }
    catch (Exception)
    {
        await Task.FromCanceled(ct.Token);
    }
}

Listen
public async Task<string> Listen(CancellationTokenSource ct)
{
    try
    {
        if (ct.Token.IsCancellationRequested)
        {
            ct.Token.ThrowIfCancellationRequested ();
        }
        using (var _pipeClientStream = new NamedPipeClientStream(".", "MyListenPipe", PipeDirection.In, PipeOptions.Asynchronous, TokenImpersonationLevel.Impersonation))
        {
            await _pipeClientStream.ConnectAsync (ct.Token);
            if (!ct.IsCancellationRequested)
            {
                var sb = new StringBuilder ();
                var reader = new StreamReader (_pipeClientStream);
                do
                {
                    string line = await reader.ReadLineAsync ();

                    if (line == MessageFooter || line == null)
                    {
                        break;
                    }
                    sb.AppendLine (line);

                } while (true);

                return sb.ToString ();
            }
            return "";
        }
    }
    catch (Exception e)
    {
        return "";
    }
}

According to my understanding your piece of software can finish in one of the following states:

Succeeded
Failed
Timed out

You are not exposing the ability to cancel it on the top level. So, it is not cancelable.
Your explicit cancellation mechanism is not needed because:

If timeout occurs at the top level then this fact will be available for all async methods, which received the CancellationToken. The built-in BCL functions will check the token's validity so you don't have to worry about it.
SendMessage and Listen are called sequentially. So if former fails then latter won't be called.
After connectAsync your IsCancellationRequested is pointless. If connectAsync succeeded then this property won't be true. If it was not succeeded then you would not reach this line because your Listen function will be aborted.

On the other hand there is one place where it could make sense to check the cancellationToken. That's inside the do-while loop because the ReadLineAsync is not cancellable.
So let's put all this together with these in mind. I've converted your code into C# 8 if you don't mind.
Listen
public async Task<string> Listen(CancellationToken ct)
{
    try
    {
        await using var _pipeClientStream = new NamedPipeClientStream(".", "MyListenPipe", PipeDirection.In, PipeOptions.Asynchronous, TokenImpersonationLevel.Impersonation);
        await _pipeClientStream.ConnectAsync(ct);

        var sb = new StringBuilder();
        using var reader = new StreamReader(_pipeClientStream);
        do
        {
            ct.ThrowIfCancellationRequested();
            string line = await reader.ReadLineAsync();

            if (line == MessageFooter || line == null)
            {
                break;
            }
            sb.AppendLine(line);

        } while (true);

        return sb.ToString();
    }
    catch
    {
        return "";
    }
}

The NamedPipeClientStream is IAsyncDisposable that's why I used await using.
The StreamReader is IDisposable that's why I used using.
I've checked the CancellationToken before each ReadLineAsync because it is not cancellable.

SendMessage
public async Task SendMessage(string message, CancellationToken ct)
{
    await using var _pipeClientStream = new NamedPipeClientStream(".", "MyPipe", PipeDirection.Out, PipeOptions.Asynchronous);
    await _pipeClientStream.ConnectAsync(1000, ct);

    await using var writer = new StreamWriter(_pipeClientStream) { AutoFlush = true };
    await writer.WriteLineAsync(message.AsMemory(), ct);
    await writer.WriteLineAsync(MessageFooter.AsMemory(), ct);
}

WriteLineAsync can accept CancellationToken only if it is called with a ReadOnlyMemory or with a StringBuilder. That's why I used AsMemory.
StreamWriter is IAsyncDisposable that's why I used await using.

GetValueUsingNamedPipe
public async Task<string> GetValueUsingNamedPipe(CancellationToken ct)
{
    await SendMessage("MyMessage", ct);
    return await Listen(ct);
}

The sendMsg variable is pointless because right after the assignment it is awaited then never used.
I've removed the try-catch block because in that way the TaskCancelledException would be caught here.
So basically if you wish you can inline this method into the CheckForValue

CheckForValue
public async Task<bool> CheckForValue()
{
    const int timeout = 300;
    try
    {
        using var timeoutTokenSource = new CancellationTokenSource(timeout);
        var result = await GetValueUsingNamedPipe(timeoutTokenSource.Token);
        return (result == "WhatIWant");
    }
    catch (TaskCanceledException) //The actual type will be TimedOutTaskCanceledException
    {
        return false;
    }
    catch (Exception) // GetValueUsingNamedPipe failed for some reason
    {
        return false;
    }
}

All the explicit cancellation calls are gone, because they are not needed. The cancellation fact will be propagated all the related parties and it will manifest in a TaskCancelledException.
So it is much cleaner now. :D

I hope it helped you a bit.


Is this a correct implementation/interpretation of the "Birthday Problem"? I recently wanted to determine the probability of generating a colliding string, given a character pool size, string length and number of strings to generate.  I was led to the Birthday Problem, and came up with this code in PHP:

function collision_probability($length, $count = 1) {

    $chars = '1234567890qwertyuiopasdfghjklzxcvbnm';

    $probability = 1.0;

    // Maximum number of combinations
    $combinations = pow(strlen($chars), $length);

    echo "$combinations combinations at $length characters long for $count tries\n";

    foreach(range(1,$count) as $i) {
        $probability = $probability * (1 - ($i/$combinations));
    }

    $probability = 1 - $probability;
    echo "A total probability their being collisions of ".($probability*100). "%\n";

    return $probability;

}

echo collision_probability(9, 3000000);

Given the echos, and the values you can see, am I interpreting this correctly?  Or am I missing something?

Nitpicks

$chars = '1234567890qwertyuiopasdfghjklzxcvbnm';

I would find this easier to read if it were in alphabetical order.

$chars = '1234567890abcdefghijklmnopqrstuvwxyz';

That way I can see that are no letters missing.

$alphanumeric_alphabet = '0123456789abcdefghijklmnopqrstuvwxyz';

I think that makes it clearer what the string is.  Also, I prefer to name scalars (numbers, strings, etc.) with singular names.  I'd name $combinations $combination_count for the latter reason.

$probability = 1.0;

This is the probability that you can select the first combination without a collision.

foreach(range(1,$count) as $i) {

This is neat, but PHP doesn't handle it well.  It will create the whole array and then iterate over the array.

for ( $i = 1; $i <= $count; $i++ ) {

This is more efficient in PHP.

Mathematics

However,

for ( $i = 1; $i < $count; $i++ ) {

is probably more correct.  Remember that the probability of finding a combination without a collision is 1.0.  So the first iteration through the loop is the 2nd, and the $count iteration is the $count + 1 combination.

Now, if instead of the birthday problem, you are looking for the chance of a match to an already existing combination, then this would be closer.  However, that problem would normally not care about collisions in the match tries.  So it seems you should either change the upper limit in the for loop or swap out the loop for

$probability = pow(1 - 1/$combinations, $count);

That's the chance of not matching a preexisting selection with $count tries.  Note that in that case, the tries can match each other.  If you want the probability of a collision:

$probability = 1 - pow(1 - 1/$combinations, $count);

More Nitpicks

Assuming the birthday problem is the right model,

    $probability = $probability * (1 - ($i/$combinations));

You could write this as

    $probability *= 1 - ($i/$combinations);

Which seems simpler.  Note that the parentheses around the division are unnecessary as division is higher precedence than subtraction.  Not normally a big deal either way.  They do slow down the loop slightly though, so if you have performance issues, you could try removing them.  With high enough iterations, it could shave off noticeable time.

echo "A total probability their being collisions of ".($probability*100). "%\n";

You misspelled "there":

echo "Total probability there being collisions of ".($probability*100). "%\n";


Create two arrays duplicate code I wrote two functions that creates two 2 dimensional arrays, but there is a lot of duplicate code and I don't know how to split it in one function. Is it even possible? I know the rule don't repeat yourself but I don't know how to use it because I learn javascript and programming. Also do you recommended books to learn how to write clean code.

First function

function createMatrixA() {

    var i, j;
    var htmlElements = "";
    var matrixWidth = parseInt(document.getElementById("matrix-width-a").value, 10);
    var matrixHeight = parseInt(document.getElementById("matrix-height-a").value, 10);

    if (matrixWidth <= 7 && matrixHeight <= 7) {
        matrixA = new Array(matrixHeight);
        for (i = 0; i < matrixA.length; i++) {
            matrixA[i] = new Array(matrixWidth);
        }
        for (i = 0; i < matrixA.length; i++) {
            htmlElements += "
";
            for (j = 0; j < matrixA[i].length; j++) {
                matrixA[i][j] = "
<input type=\"number\" class=\"form-control\" id=\"a" + (i + 1) + (j + 1) + "\" placeholder=\"a" + (i + 1) + (j + 1) + "\" >
";
                htmlElements += matrixA[i][j];
            }
            htmlElements += "
";
        }
        document.getElementById("matrix-a").innerHTML = htmlElements;
    } else {
        document.getElementById("matrix-a").innerHTML = "
Maximum matrix size is 7x7.
";
    }
}

Second function

function createMatrixB() {
    var i, j;
    var htmlElements = "";
    var matrixWidth = parseInt(document.getElementById("matrix-width-b").value, 10);
    var matrixHeight = parseInt(document.getElementById("matrix-height-b").value, 10);

    if (matrixWidth <= 7 && matrixHeight <= 7) {
        matrixB = new Array(matrixHeight);
        for (i = 0; i < matrixB.length; i++) {
            matrixB[i] = new Array(matrixWidth);
        }
        for (i = 0; i < matrixB.length; i++) {
            htmlElements += "
";
            for (j = 0; j < matrixB[i].length; j++) {
                matrixB[i][j] = "
<input type=\"number\" class=\"form-control\" id=\"b" + (i + 1) + (j + 1) + "\" placeholder=\"b" + (i + 1) + (j + 1) + "\" >
";
                htmlElements += matrixB[i][j];
            }
            htmlElements += "
";
        }
        document.getElementById("matrix-b").innerHTML = htmlElements;
    } else {
        document.getElementById("matrix-b").innerHTML = "
Maximum matrix size is 7x7.
";
    }
}

compare the difference and extract the differences into parameters.
you can look at this.

function createMatrix(domId) {
    var i, j;
    var htmlElements = "";
    var matrixWidth = parseInt(document.getElementById(domId).value, 10);
    var matrixHeight = parseInt(document.getElementById(domId).value, 10);

    if (matrixWidth <= 7 && matrixHeight <= 7) {
        matrix = new Array(matrixHeight);
        for (i = 0; i < matrix.length; i++) {
            matrix[i] = new Array(matrixWidth);
        }
        for (i = 0; i < matrix.length; i++) {
            htmlElements += "
";
            for (j = 0; j < matrix[i].length; j++) {
                matrix[i][j] = "
<input type=\"number\" class=\"form-control\" id=\"b" + (i + 1) + (j + 1) + "\" placeholder=\"b" + (i + 1) + (j + 1) + "\" >
";
                htmlElements += matrix[i][j];
            }
            htmlElements += "
";
        }
        document.getElementById(domId).innerHTML = htmlElements;
    } else {
        document.getElementById(domId).innerHTML = "
Maximum matrix size is 7x7.
";
    }
}

createMatrix('matrix-width-a');
createMatrix('matrix-width-b');


Looping through a string finding nested brackets So I've been trying to check through a string, looping for, [ (start) and ] (end).

In this, I also have to check for nested brackets.

My expected output, given input [depth1[depth2]] is as follows:

depth2
depth1[depth2]

To achieve this, I wrote this little snippet of code, which uses a Dictionary<int, int> to store the initial bracket's position at a given depth. If it finds another opening bracket, the depth will increase (and the position will be recorded). It will look for the next ending bracket (with respect to any other opening brackets), and get the initial position of the bracket for that depth. (Thus we get both the beginning and ending of that nested string)

    var depthItems = new Dictionary<int, int>();
    int depth = 0;

    string input = "[depth1[depth2]]";

    for (int i = 0; i < input.Length; i++) {
        if (input[i] == '[') {
            depth++;
            depthItems.Add(depth, i);
        } else if (input[i] == ']') {
            int item = depthItems[depth];
            depthItems.Remove(depth);
            depth--;
            Console.Write($"Found item at ({item}, {i}) -> ");
            Console.WriteLine(input.Substring(item + 1, i - item - 1));
        }
    }

I'm not sure how viable this would be when looking up against large strings with lots of nested brackets - I think it's a start, at least.

I will note that I was first surprised that higher "depths" were returned first, but then realised that in order for the lower ones to complete, the higher ones should have been returned first. Not a bad thing, but curious to note.

So how could this be improved, especially in terms of memory (i.e. large strings)?

I'm not sure how viable this would be when looking up against large strings with lots of nested brackets [...]

"how viable" is a very vague question.
Two common performance metrics are CPU load and memory load,
perhaps that's what you were trying to get at.
It's good to clarify what exactly you want to know.

In terms of CPU, how will this program behave as the input grows?

The algorithm does a single pass over all characters of the input.
When the input doubles, the number of operations doubles, roughly.

=> In other words, it's linear.

Can it be any better than linear?

=> No, because we cannot know the positions of the brackets without looking through each character once.

In terms of memory, how will this program behave as the input grows?

Aside from the memory used by the input string,
what consumes memory in this program?

=> The dictionary storing the starting positions of brackets.

How do brackets on the same nesting level impact the memory consumption?
That is, in a string like [][][][]?

=> Since the storage for a [ is removed when the corresponding ] is found, brackets at the same nesting level use a single entry in the dictionary.

How do deeply nested brackets impact the memory consumption?

=> The dictionary has as many entries as the deepest nesting level. As closing brackets are found, entries get deleted. The maximum memory consumed by the dictionary throughout the program is proportional to the maximum depth of nesting encountered.

  So how could this be improved, especially in terms of memory (i.e. large strings)?

The current performance of the program is on the expected order of complexity, so it's fine.
However, a dictionary is not the most natural choice for this purpose,
and its actually overkill.
A simpler data structure, a stack, would have been enough to solve this.
Instead of storing (depth, index) pairs in a dictionary,
you could store just index in a stack:

When you see a [, push the index onto the stack.
When you see a ], pop the last pushed index from the stack.

The result will be equivalent, the depth variable becomes unnecessary.


Finding number of times each string in an Array of strings occurs in a large stream of characters (i.e large string) Interview problem:

  Given a stream of characters (e.g. acacabcatghhellomvnsdb) and a list of words (e.g. ["aca","cat","hello","world"] ) find and display count of each and every word once the stream ends.(Like : "aca" : 2 , "cat" : 1 , "hello" : 1 , "world" : 0 ).

This solution works functionally.  Please let me know if there is a faster more optimized algorithm or if there are some corner scenarios I did not take care of.

#define VALUE_KEY 1
#define COUNT_KEY 2

NSMutableArray *stringsToSearchFor = [@[[@{@VALUE_KEY: @"cat", @COUNT_KEY: @0} mutableCopy],[@{@VALUE_KEY: @"aca", @COUNT_KEY: @0} mutableCopy], [@{@VALUE_KEY: @"hello", @COUNT_KEY: @0} mutableCopy],[@{@VALUE_KEY: @"world", @COUNT_KEY: @0} mutableCopy]] mutableCopy];

NSString *stringStream = @"acacabcatghhellomvnsdb" ;

    for(NSMutableDictionary *nextDict in stringsToSearchFor)
    {
        NSUInteger currRangeStart = 0 ;
        NSUInteger currRangeLength = [nextDict[@VALUE_KEY] length] ;
         NSUInteger currentStringNumFound = 0 ;

        while(currRangeStart <= [stringStream length]-currRangeLength)
        {
            NSRange foundRange = [stringStream rangeOfString:nextDict[@VALUE_KEY]
                                                     options:0
                                                       range:NSMakeRange(currRangeStart, [stringStream length]-currRangeStart)] ;
            if(foundRange.location != NSNotFound)
            {
                 currRangeStart = foundRange.location+1 ;
                 currentStringNumFound++;
            }
            else
            {
                break ;
            }
        }

        if(currentStringNumFound)
        {
            nextDict[@COUNT_KEY] = @(currentStringNumFound) ;
        }
    }

    NSLog(@"Num occurrences dict is %@",stringsToSearchFor) ;

Given that you stated this was for an interview, I'm going to focus not on how to make this code as fast as possible, but instead, the types of things I'd be looking for in an interview candidate.

First, and most concerning, there is not function or method definition here.  Presumably, you just typed this code straight into the main function.  That'd be the first worrying thing for me if I'm in this interview.

Upon hearing the acceptance criteria, I'd immediately expect a function signature to be written out:

NSDictionary<NSString *, NSNumber *> * _Nonnull findStringsInString(
    NSArray<NSString *> * _Nonnull,
    NSString * _Nonnull
);

You're already getting bonus points from me if you type this out.

You've demonstrated that you have some understanding of what is expected.  At a minimum, you're completely clear on what sort of inputs and outputs make sense.  And the function name will give some indication that you know what's going on.
You've demonstrated above knowledge in the form of a function.  The next step of the interview could be writing some unit tests around the function.  Even if we don't get that far, even if you're not familiar with unit testing, if you can write your code in a testable manner, that's a plus.
You've taken into account Swift annotations so this method will show up nicely in Swift.  Even if it's not used from Swift, these annotations help self-document the function and lessen the need for comments.

I'd also be okay if you wrote this as an extension method on NSString.

- (NSDictionary<NSString *, NSNumber *> * _Nonnull)occurencesOfStrings:(NSArray<NSString *> * _Nonnull)strings;

Second problem (get ready to notice a theme here), you haven't broken your problem down into logical sub-problems.  How can we count the occurrences of an array of potential substrings if we can't yet even count the occurrences in a single potential substring?

The body of your findStringsInString function needs to really look simply like this:

NSDictionary<NSString *, NSNumber *> * _Nonnull findStringsInString(
    NSArray<NSString *> * _Nonnull substrings,
    NSString * _Nonnull string
) {
    NSDictionary * results = [NSMutableDictionary dictionary];
    for (NSString * substring in substrings) {
        results[substring] = countOccurrencesOfSubstringInString(substring, string);
    }
    return results;
}

Why should it be this simple?  Because the countOccurrencesOfSubstringInString also needs to be a very simple function that is easy to write tests around.  Finding the count for multiple strings is just a simple expansion over the problem of finding the count for a single string.

For the problem of finding the count of a substring in a string, I'll just point you toward this Stack Overflow question which has multiple good answers.

As for actually writing the unit tests for either of these functions, I wouldn't necessarily expect it during an interview, but I'd be delighted to hear the candidate ask about it.  Whether or not we did it would depend on whether the candidate asked about it, and completed the first part relatively quickly.  But the combination of writing easily testable functions and asking me about testing itself would be enough information about the candidate's attitude toward testing (which I find important).

You notice how I still haven't really addressed any performance issues?  As an interviewer, I simply have never cared.  When candidates ask me if I want to fastest possible solution to a programming challenge, I always respond with something like this:

  I want the solution that works with the least amount of your time invested.  After we have a working solution, we can talk about optimizations.

We'll usually have specific criteria on the minimum acceptable performance of our code.  That criteria is never phrased as "as fast as possible".  It comes with a benchmark.  It must perform in less than some measurable amount of time.  It's either below that and good enough or above that and not good enough.  For almost all things you do, there is never the requirement that a solution work as fast as possible.  (There is usually the requirement that it not be slow, and slow is a concrete value.)

So, let's talk about style, and the readability of your solution.

#define VALUE_KEY 1
#define COUNT_KEY 2

These #defines are weird and unnecessary.  As you'll read later, a better data structure makes these completely obsolete, but if we must use them, I much prefer solutions that aren't preprocessor find & replace.

I'd prefer

NSNumber * const kKeyValue = @1;
NSNumber * const kKeyCount = @2;

NSMutableArray *stringsToSearchFor = [@[[@{@VALUE_KEY: @"cat", @COUNT_KEY: @0} mutableCopy],[@{@VALUE_KEY: @"aca", @COUNT_KEY: @0} mutableCopy], [@{@VALUE_KEY: @"hello", @COUNT_KEY: @0} mutableCopy],[@{@VALUE_KEY: @"world", @COUNT_KEY: @0} mutableCopy]] mutableCopy];

This is an absolute mess on so many levels.

First of all, a little bit of whitespace breaks cleans this up a lot.

NSMutableArray *stringsToSearchFor = [@[
    [@{kKeyValue: @"cat", kKeyCount: @0} mutableCopy],
    [@{kKeyValue: @"aca", kKeyCount: @0} mutableCopy],
    [@{kKeyValue: @"hello", kKeyCount: @0} mutableCopy],
    [@{kKeyValue: @"world", kKeycount: @0} mutableCopy]
] mutableCopy]

But this is still far from ideal.  Read the problem statement again.

  Given a stream of characters (e.g. acacabcatghhellomvnsdb) and a list of words (e.g. ["aca","cat","hello","world"] )

We're not going to get this mutable array of mutable dictionaries as our input.  You just assumed this would be the input.  At this point, as an interviewer, I'd be a little irritated.  Your code is going to either take the time to build out this overly complicated data structure, or worse, you're going to expect input in this overly complicated data structure.  I say the latter is worse because it demonstrates no technical empathy (or indicates you misread the requirements, which is also very bad).  Be clear on the requirements, and if you're unsure, ask.  It won't make you look bad during an interview--it will make you look good to get clarification on anything that's unclear.  The 30 seconds it takes to ask a question can save days of work in the real world.

And the worst part is that as my earlier code snippet demonstrates, there's a very easy solution here for what our data structure looks like.  And you should never be creating dictionaries whose keys roughly translate into @"key", @"value".

Any time you have a dictionary with two key value pairs that looks something roughly like this:

@{@"key": someKey, @"value": someValue}

You've messed up.  Especially because you probably have an array of these dictionaries.  And it's extraordinarily hard to extract values out of an array of dictionaries that look like this (believe me, the current project has to deal with an API that provides some user settings to me in this format).

The dictionary should instead simply look like this:

@{someKey: someValue}

And now you don't need an array of these dictionaries.  You can just have a dictionary:

@{
    someKey: someValue,
    anotherKey: anotherValue
}

On most lines (but not all), you have placed a space before the semicolon.  These seems very strange to me.  It is generally preferred not to have this space.

else
{
    break ;
}

Given that there's nothing else in the while loop, these four lines of code are entirely unnecessary.

if(currentStringNumFound)
{
    nextDict[@COUNT_KEY] = @(currentStringNumFound) ;
}

I know this technically works, but I've got a couple issues with it.  First of all, this is hard to follow.  We're relying on the fact that in C, non-zero equates to true, and then we're immediately inserting that same variable into a dictionary as a count.  I'd much prefer it if we just wrote:

if (currentStringNumFound > 0)

This more clearly mirrors the actual logic you're applying here.  You only want to insert the value if it was found at least once.

So that can make the readability and intent more clear.  But I'm a little concerned here that this is perhaps not the intended behavior.  If I passed in a string and it wasn't found in the dictionary, how can I be confident that it was even checked at all?

In fairness, the result would give me nil for whatever key I was looking for, and that'd really translate equivalently to 0.  But it doesn't translate equivalently in the case that I wanted to call some prettify NSNumber extension I wrote on it.

So, for example, if I had an NSNumber extension something like this:

- (NSString * _Nonnull)prettifiedCount;

I might expect the result of calling this on an NSNumber representing zero to be @"zero", but if I called it on what I got out of your result for a string that wasn't found, I'd get nil.

And the thing is, it won't be nil in your dictionary because of that very strange data structure you set up in the beginning, but that data structure definitely needs to change, so keep this part in mind when you're changing your data structure.

Finally, logging.

I really don't want my utility functions spitting out logs.  Yeah, you probably need that log to demonstrate to the interviewer that it's working, but to be clear, if you rewrite this per the guidelines I've set forth throughout this answer, you'll have all of your logic wrapped up in these functions.  There shouldn't be any logging within those functions.  If you want to log the results after the fact, sure, go ahead... but not within the functions themselves.


C# Code Optimization -- Finding nearest key in a SortedList Just wondering if anyone can see a way to optimize this piece of code.  It is a key piece of my program and needs to run as quickly as possible. The part of the code I am unsure of is the while loop for finding the nearest key -- but any assistance with optimizing the code would be appreciated.

// TODO: Move to snippets lib or create a new collection type that supports this feature
private string _getTrait(SortedList<decimal, string> thisList, decimal thisValue)
{
    // Check to see if we need to search the list.
    if (thisList == null || thisList.Count <= 0) { return null; }
    if (thisList.Count == 1) { return thisList.Values[0]; }

    // Setup the variables needed to find the closest index
    int lower = 0;
    int upper = thisList.Count - 1;
    int index = (lower + upper) / 2;

    // Find the closest index (rounded down)
    bool searching = true;
    while (searching)
    {
        int comparisonResult = Decimal.Compare(thisValue, thisList.Keys[index]);
        if (comparisonResult == 0) { return thisList.Values[index]; }
        else if (comparisonResult < 0) { upper = index - 1; }
        else { lower = index + 1; }

        index = (lower + upper) / 2;
        if (lower > upper) { searching = false; }
    }

    // Check to see if we are under or over the max values.
    if (index >= thisList.Count - 1) { return thisList.Values[thisList.Count - 1]; }
    if (index < 0) { return thisList.Values[0]; }

    // Check to see if we should have rounded up instead
    if (thisList.Keys[index + 1] - thisValue < thisValue - (thisList.Keys[index])) { index++; }

    // Return the correct/closest string
    return thisList.Values[index];
}

I am using C#, .net4.0 -- I need to use a Generic SortedList ( http://msdn.microsoft.com/en-US/library/ms132319(v=vs.100).aspx )

Your loop is pretty well written. The only thing that sticks out to me is your use of a constant in the while loop and manually breaking.  I also prefer to use a slightly different version of CompareTo, but it doesn't make much difference:

while ( (lower<=upper))
{
    int comparisonResult = thisValue.CompareTo( thisList.Keys [index]);
    if (comparisonResult == 0) { return thisList.Values[index]; }

    if (comparisonResult < 0)
    {
        upper = index - 1;
    }
    else
    {
        lower = index + 1;
    }
    index = (lower + upper) / 2;
}

You might also be looking for a solution that's just easier to read or shorter. Are you familiar with LINQ? This is one possibility for how your function might look.

    private static string GetTraitRefactor (SortedList<decimal, string> thisList, decimal thisValue)
    {
        var keys = thisList.Keys;
        var nearest = thisValue -
            keys.Where(k => k <= thisValue)
            .Min(k => thisValue - k);
        return thisList[nearest];
    }


Iterative version of a permutations algorithm For learning purposes, I've written an iterative version of a recursive algorithm for generating the set of permutations for a given set of unique integers.

Can I make any improvements to increase readability and performance?

I've also read in various sources that when transforming a recursive algorithm into an iterative one, the stack data-structure is often used to 'substitute' the call stack used in the recursive case. I'm not using a std::stack here but could I have used one to simplify my implementation?

using std::vector;

vector<vector<int>> permutations(vector<int> integers) {
    vector<vector<int>> curr_perms;

    if (integers.size() <= 1) {
        curr_perms.push_back(integers);
        return curr_perms;
    }

    size_t sz = integers.size();

    vector<vector<int>> next_perms;

    // initialise with the one permutation of the last single element
    vector<int> t = { integers[sz - 1] };
    curr_perms.emplace_back(t);

    // backwards iterating a sequence of at least two elements
    for (int i = sz - 2; i >= 0; --i) {
        auto first = integers[i];

        for (const auto &s : curr_perms) {
            for (auto j = 0U; j < s.size() + 1; ++j) {
                vector<int> p(s.begin(), s.end());  // make a copy
                p.insert(p.begin() + j, first);     // shuffle in 'first' element
                next_perms.push_back(std::move(p)); // and add this as a new permutation
            }
        }

        // permutations found in this iteration are the input for the next
        std::swap(next_perms, curr_perms); // <-- is this needlessly copying all the elements?
        next_perms.clear();
    }

    return curr_perms;
}

As often, the recursive algorithm is readable and beautiful, but not very efficient: stack consumption can be overwhelming, memory allocations are plentiful, and you're forced to compute the whole set of permutations at once.

When you mechanically translate the recursive algorithm into its iterative form, you often lose most of its beauty and readability, without gaining much on the front of performance: you just emulate with a data structure what the compiler would have done. It might be useful sometimes but you can generally assume that the compiler will do a better job than yourself.

Hence I would recommend looking into two alternative approaches:

the first is to do it the standard library way, and provide a std::next_permutation function which return the next permutation in the lexicographical order (see for instance my answer to this question). It can be improved upon though, with the SJT algorithm, and that would be an exercise of choice;
the second is to keep the general, incremental reasoning of the recursive algorithm, but use the additional knowledge you get in the iterative form: indeed, on the contrary to the recursive form, you know upfront how many permutations you'll return in the end, and thus can avoid all but one allocation.

For instance:

#include <vector>

std::vector<std::vector<int>> permutations_set(const std::vector<int>& set) {

    if (set.empty()) return {};

    auto current = std::begin(set);

    // the unique memory allocation
    std::vector<std::vector<int>> permutations(factorial(set.size()), {*current});

    for (++current; current != std::end(set); ++current) {
        std::size_t offset = 0; // offset plays the central role here; you could say it replaces the stack
        for (auto& permutation : permutations) {
            const auto initial_size = permutation.size();
            permutation.insert(std::next(std::begin(permutation), offset), *current);
            if (offset++ == initial_size) offset = 0;
        }
    }

    return permutations;
}


Python Morse Code Translator I've been teaching myself Python over the last few weeks, I have an interest in cryptography, codes, etc. so I figured starting a Morse code translator  would be a good project. I know my variable names could be different, it's not really encrypting, decrypting, etc. I'm mostly looking for advice on how I can make the code cleaner and where I can be more efficient.
I think my biggest problem is not really knowing how to handle the inputs in a while loop like I usually would. The problem I had was that I couldn't check if the input was 'e' or 'd' so it just got really wonky.
Areas I know I could improve:

Add input loop
The if, elif, else for the action
Make 'sound' an actual boolean value
Find the actual sound time for the dit and dah but that's not really a code issue

# Started: 08/17/2020
# Finished: 08/17/2020
# Takes an input message and outputs the message in morse code
# Keys taken from 'https://en.wikipedia.org/wiki/Morse_code'

from playsound import playsound
import time

# Dictionary that holds each letter and it's corresponding value
dict = {'a': '.-', 'b': '-...', 'c': '-.-.', 'd': '-..', 'e': '.', 'f': '..-.', 'g': '--.', 'h': '....', 'i': '..', 'j': '.---', 'k': '-.-', 'l': '.-..', 'm': '--',
        'n': '-.', 'o': '---', 'p': '.--.', 'q': '--.-', 'r': '.-.', 's': '...', 't': '-', 'u': '..-', 'v': '...-', 'w': '.--', 'x': '-..-', 'y': '-.--', 'z': '--..',
        '1': '.----', '2': '..---', '3': '...--', '4': '....-', '5': '.....', '6': '-....', '7': '--...', '8': '---..', '9': '----.', '0': '-----',
        ' ': '/', '.': '.-.-.-', ',': '.-.-', '?': '..--..', "'": '.----.', '!': '-.-.--', '/': '-..-.', '(': '-.--.', ')': '-.--.-',
        ':': '---...', ';': '-.-.-.', '=': '-...-', '+': '.-.-.', '-': '-....-', '_': '..--.-', '"': '.-..-.', '$': '...-..-', '@': '.--.-.'}

outputMessage = ""               # Holds our output message

# Sounds
sound = 'False'
dit = 'dit.wav'
dah = 'dah.wav'

def Encrypt(message):

    output = ''

    for char in message:
        if char in dict:
            output = output + dict[char]
            output = output + ' '

    return output

def Get_Key(val):
    for key, value in dict.items():
        if val == value:
            return key

def Decrypt(message):

    output = ''

    letters = message.split(' ')

    for letter in letters:
        temp = Get_Key(letter)
        output = output + temp

    return output

def Get_Inputs():
    # Get Inputs
    inputString = input('Enter a message to start.\n')
    action = input('(E)ncrypt or (D)ecrypt?\n')

    # Format Inputs
    message = inputString.lower().strip()
    action = action.lower().strip()

    return message, action

def Play_Sound(message):

    for char in message:
        if char == '.':
            playsound(dit)
        elif char == '-':
            playsound(dah)
        elif char == ' ':
            time.sleep(0.15)
        elif char == '/':
            time.sleep(0.30)

message, action = Get_Inputs()

if action == 'e' or action == 'encrypt':
    outputMessage = Encrypt(message)
elif action == 'd' or action == 'decrypt':
    outputMessage = Decrypt(message)
else:
    print('Error!')

print(outputMessage)

print('')
sound = input('Play sound? (T)rue / (F)alse\n')
if sound.lower().strip() == 't' or sound.lower().strip() == 'true':
    Play_Sound(outputMessage)

General Style
Your translation dict uses a keyword and lower case letters.
Consider writing constants with upper-case letters and giving them expressive names like MORSE_CODES = {...}.
According to PEP 8, functions should be named using snake_case. CamelCase is reserved for classes:
outputMessage → output_message, def Encrypt(...) → def encrypt(...), etc.
Performance
Using the Get_Key function is not very performant, since it performs a linear search of the dict. Just reverse the translation dict once and then use it:
MORSE_ENCODING = {
    'a': '.-',
    'b': '-...',
    ...
}
MORSE_DECODING = {value: key for key, value in MORSE_ENCODING.items()}

...

        temp = MORSE_DECODING[letter]

Handling errors
Currently the Encrypt function silently skips all non-translatable characters. Consider Throwing a ValueError() instead to indicate, that invalid input was provided:
def encode(message):
    """Encodes a string into morse code."""

    code = ''

    for index, char in enumerate(message):
        try:
            code += MORSE_ENCODING[char.lower()]
        except KeyError:
            raise ValueError(f'Char "{char}" at {index} cannot be encoded.')

        code += ' '

    return code[:-1]  # Remove trailing space.

def decode(morse_code):
    """Decodes morse code."""

    message = ''

    for index, sequence in enumerate(morse_code.split()):
        try:
            message += MORSE_DECODING[sequence]
        except KeyError:
            raise ValueError(f'Cannot decode code "{sequence}" at {index}.')

    return message

Correctness
Your Encrypt function currently always returns a trailing space. You can avoid that by returning output[:-1].
Terminology
Converting from morse code to text back and forth is not really an encryption in its sense. You might want to rephrase {en,de}crypt with {en,de}code.
Globals
Using global variables like outputMessage can have nasty side-effects when the program is used as a library. All the code below the def Play_Sound function should go into a def main() function that you can invoke via
if __name__ == '__main__':
    main()

At the bottom of the unit.


Pass the password back This code simply receives a very basic 'password' (numbers, letters, nothing fancy). After you press enter, it will display it back. The only special key I'm handling is backspace (code 8 in ASCII).

uses crt;

var
    pass: string;
    key: char;
    len: integer;

begin

clrscr;

key := chr(0);
pass := chr(0);

write('Write the password:');

while key<>chr(13) do
begin
    key := readkey;
    len := length(pass);
    case ord(key) of
        8: begin
                if len > 0 then
                begin
                    delete(pass, len, 1);
                end;
            end;
        else
            pass := concat(pass, key);
    end;
end;

writeln('Password: ', pass);

readln; //pauses the code

end.

You will probably notice that the program block is missing. It was on purpose, since Free Pascal Compiler has issues with it.

The code compiles / runs fine, to the best of my knowledge. Remember that this is simply and purely made for ASCII 7-bit (I think the extended set will work).

In terms of clarity, structure and indentation, what else can I improve?

Indentation:

From the Coding Style recommendation, I am using 2-space indentation, which works for me. The guide suggests that you should avoid tabs as well:

  Do not use TAB characters (ASCII HT, 0x09). There is no standard default TAB setting, so the look of source files using TAB characters will depend on client settings. This may result in a chaotic view of source files. Align by space characters (also see Indentation).

However, the same coding style says that you should not use space around operators, symbols...:

  Don't use spaces around operators, colons, parentheses etc. e.g. write p:=p+i; instead of p := p + i ;.

Since I am not using colored symbols, I've chosen to follow the "Guideline of the Lazarus 1.4.0 color preview":

As you can see, the color preview of the IDE uses spaces around symbols (:=, +, -, *, and so on). Whoever wrote this preview is a point out of curve like me.

Code:

Repeat is a posttest loop, and your code needs to run at least once, so why not a repeat..until loop.. In another words, you know the first key will always be different from #13, you hardcoded it. You can save a begin..end block, as well.
You do not need begin..end blocks for one line statements inside case conditions.

Structure:

For code reuse and exploration I would find it more practical to create a function that would only wait for and then return the password:

program HandlePassword;

uses
  crt;

function WaitForPassword: string;
var
  key: char;
  len: integer;
begin
  Result := Chr(0);
  key := Chr(0);
  repeat
    key := ReadKey;
    len := Length(Result);
    case Ord(key) of
      8: if len > 0 then Delete(Result, len, 1);
      else Result := Result + key;
    end;
  until key = Chr(13);
end;

begin
  ClrScr; // clear screen
  Write('Write the password:');
  WriteLn(WaitForPassword);
  ReadLn; // pauses the code
end.


How effective is it to default to user input in C++ function parameters? This defaults to user input if the arguments are not passed at compile time.. How effective is this technique and where can it be applied?

#define _CRT_SECURE_NO_WARNINGS
#include <iostream>

void foo(unsigned number = []() {unsigned num = 0; std::cin >> num; return num; } ()) {
    std::cout << number << "\n";
}

int main() {
    foo(1); //prints 1
    foo();  //defaults to user input
    return 0;
}

I haven't seen this pattern in use, and I wouldn't use it personally, for two reasons:

1) Unintuitive - Changing the function's behavior in this significant of a way is not something you'd expect from the default value of a parameter.

2) Mixing Concerns - It's almost always better to separate your I/O from your algorithm.

Considering that this is a compile-time difference anyway, it would be more clear to have another method to read input.  For example:

foo(1);
foo(foo_input());


Multiple returns in a method or one return I have a method which returns an Array. This array can contain values or can be empty. Here is the code:

function getCustomerIds() {
    //this declaration is at the top to inform about returning value of the method; I don't declare variables at the top
    var customerIds = [];
    var linkedContact = getLinkedContacts(this);
    if (linkedContact === null) {
        return [];
    }
    var linkedResource = getLinkedResources(linkedContact);
    if (linkedResource === null) {
        return [];
    }
    var linkedCustomersIds = getLinkedCustomers(linkedResource);
    return customerIds.concat(linkedCustomersIds);
}

I'm not sure if it should be return [] or return customerIds. The first one seems more explicit that the empty array is returned, while the second option seems more logical. What would be the best practice here?  Do you have some other suggestions re method structure?

It's debatable. Or perhaps I'd say: it's very dependent on context.

If you return early (as you do here), I think it's probably good to return []. When you read the code, you see the conditional and its effect: an empty array is returned, period. To me early returns feel a bit like throwing exceptions - not in the sense that there's an error, but just "this is a special case; do something special and return control to the caller (i.e. exit the function)"

The other way to go about it is, of course, to return at the end of the function, and treat the steps before that as optional (or "conditional" to be exact). They may do something to the customerIds array, or they may not. Regardless, we return the array at the end, whether it's empty or not.

(note: I'm using all these words, like "normal" or "special", very lightly. Don't read too much into them)

Either way is perfectly acceptable, so it's pretty much up to you. You've got two dependent conditionals, so returning early is a good way to avoid a ton of indentation and nesting.

However, if the return value "changes" when the function returns early, and you don't otherwise declare variables at the top, I'd skip declaring customerIds there too. If the idea is to declare the thing you'll return, but then you don't return that specific object anyway, it's misdirection. Though again, this is really on the margins of "does it matter?" because you're always returning an array, so it's not like the var declaration is totally false.

But, but, in your case, the simplest solution might be to not have that declaration at all. If you expect the code to be read, and people to see the return [];, you can also expect them to read to the end of the function anyway, and see what it would otherwise return. No need for the return-type pseudo-declaration at all. Especially as it's not needed at all (the concat call is pointless). (see update below the code)

function getCustomerIds() {
    var linkedContact = getLinkedContacts(this);
    if (linkedContact === null) {
        return [];
    }
    var linkedResource = getLinkedResources(linkedContact);
    if (linkedResource === null) {
        return [];
    }
    return getLinkedCustomers(linkedResource);
}

Update: As 200_success points out in the comments, the semantics do change when returning without using concat. However, I'd probably still prefer a more direct route than declaring an empty array (that I might not use for anything), and using concat. I'd probably do this

return getLinkedCustomers(linkedResource).slice(0);

to get a copy of the array returned by getLinkedCustomers(). End of update

No declaration necessary. Your function's name (which is plural) should already be a strong enough hint that an array will get returned. Or, you know, add a comment... that's a pretty good way to deal with this too, now that I think about it.

So basically, I think it's your custom of "kinda' sorta'" declaring the return type that's interfering here. Since such a declaration is not exactly binding or enforced by JS (at all), the early returns muddy the picture.

Or (for fun) if you just want to confuse people for a moment :)

function getCustomerIds() {
    var linkedContact = getLinkedContacts(this),
        linkedResource = !linkedContact || getLinkedResources(linkedContact);
    return linkedResource ? getLinkedCustomers(linkedResource) : [];
}

Update: If I'm correct in assuming that all the getLinked...() functions return similar types, then we can deduce that they all return either an array or null.

If that's the case, then both my suggestion (not the joking one above, but the actual one higher up), and your code may give some unexpected results.

The original customerIds.concat(linkedCustomersIds) will result in [null] - not an empty array.

My code will either return the null directly, or - if you use the slice call - it'll just fail, as null doesn't have a slice function.

So there should perhaps be a 3rd conditional for null-checking the return of getLinkedCustomers(linkedResource) and deciding what to return.

But again, this is me making some straight-up guesses at how the rest of the code works. No idea if it's actually an issue.

However, it's confusing that your variables are singular, but hold the return values of functions that are plural. Given the plural function names, I'd assume the return values are (nominally) arrays, but given the singular variables, I'd assume something else (anything else, really).


Knockout selectlist binding I've done some tinkering about with Knockout in search for a better solution on how to bind select lists (dropdowns). I came up with a extender in combination with a binding.

Extender:

ko.extenders.selectList = function (target, params) {
                    var result = ko.computed({
                        read: target,
                        write: function (newValue) {
                            if (newValue) {
                                target(newValue);
                            } else {
                                target(undefined);
                            }
                        }
                    });

                //add some sub-observables
                result.options = $.map(params.options, function (v, k) { return { val: k, txt: v }; });
                result.caption = params.caption;

                function getInitialEnableValue(){
                    if(params.hasOwnProperty('enable')){
                        if($.isFunction(params.enable)){
                            return params.enable();
                        }
                        return params.enable;
                    }
                    return true;
                }

                var isEnabled = ko.observable(getInitialEnableValue());
                result.enable = ko.computed({
                    read: function(){
                        return isEnabled();
                    },
                    write: function(newValue){
                        if(!newValue) result(undefined);
                        isEnabled(newValue);
                    }
                });

                if(isEnabled() && result.options.length === 1){
                    result(result.options[0].val);
                }

                //return the new observable
                return result;
            };

Binding:

var selectListHTML = "<select data-bind=\"value: {0}, options: {0}.options, optionsCaption: {0}.caption, optionsText: 'txt', optionsValue: 'val', enable: {0}.enable\"></select>";
                    ko.bindingHandlers.selectList = {
                        init: function(element, vA, aBA,vm,bc){
                            $(element).html(selectListHTML.replace(/\{0\}/g,vA()));
                        }
                    };

Example fiddle

Are there any pitfalls I'm missing? Does this look like good code? Any comments welcome, thanks in advance!

From a once over:

Your indentation in ko.extenders.selectList = function (target, params) { should be 4 spaces to make it far more readable
write could use a ternary approach:

write: function (newValue) {
  target( newValue || undefined );
}

This code could use some more/better comment
JSHint could find nothing wrong with it
I am not too excited by seeing all those options as part of the HTML, it just feels wrong. None of the tools work for you in this case ( linting, beautifying, even colored syntax ). I guess that's more a reflection on knockout than on your code.


recursive function : filtering large multidimensional array by key element to HTML list I have a multidimensional array as a result from a webservice called from PHP. I cannot edit the webservice, but the result is as

array (size=5)
  'id' => int 0
  'name' =>
    array (size=7)
      'it' => string 'Tutti' (length=5)
      'ca' => string 'Tots' (length=4)
      'es' => string 'Todos' (length=5)
      'de' => string 'Alle' (length=4)
      'fr' => string 'Tous' (length=4)
      'nl' => string 'Alle' (length=4)
      'en' => string 'All' (length=3)
  'tree' =>
    array (size=0)
      empty
  'languages' =>
    array (size=7)
  'subTypes' =>
    array (size=4)
      0 =>
        array (size=5)
          'id' => int 1
          'name' =>
            array (size=29)
              ...
          'tree' =>
            array (size=1)
              ...
          'languages' =>
            array (size=29)
              ...
          'subTypes' =>
            array (size=3)
              ...
      1 =>
        array (size=5)
          'id' => int 2
          'name' =>
            array (size=29)
              ...
          'tree' =>
            array (size=1)
              ...
          'languages' =>
            array (size=29)
             ...
          'subTypes' =>
            array (size=6)
              ...

As you can see, you can interpret it as a multidimensional array of objects. Each objects can have child objects of same type.

My goal is to get only the elements on a specific language, fe en (and thus rest is not taken). I thought to use a recursive function.

// filter array
function getByKey($array, $search, $level, $valuesUsed) {
    echo '
';
    foreach($array as $key => $value) {
        if(is_array($array[$key])) {
            $valuesUsed = getByKey($array[$key], $search, $level + 1, $valuesUsed);
        } else if ($key === $search) {
            if(!in_array($value, $valuesUsed)) {
                echo "
 $key = $value
";
                $valuesUsed[] = $value;
            }
        }
    }
    echo '
';
    return $valuesUsed;
}

Then I call the above function from my HTML template by

    <?php
        getByKey($routeTypes, 'en', 0, array());
    ?>

$routeTypes is the array, obtained from the webservice.
It works as expected, it results as the list here below, which I can wrap with a js library.

this is level1

this is level1

this is level 2

this is level 2

               ... level++

Now, I'm questioning myself if this can be improved ? It doesn't take long to have a list from a large multidimensional array. (a simple print_r command resulted in 45000+ characters).

Especially checking the $valuesUsed array, to prevent duplicates (yes the webservice has objects which contains child keys which is similar to level - 2 parent (or on other level ...... ))

Each time I am running through the lower levels, I am looping through the $valuesUsed array. Is there not a better way to loop through the sublevels ? Maybe an array_walk() or array_walk_recursive() with a callback function ?

You can choose a (key => value) pair recursively with this function:

function array_value_recursive($key, array $arr){
    $val = array();
    array_walk_recursive($arr, function($v, $k) use($key, &$val){
        if($k == $key) array_push($val, $v);
    });
    return count($val) > 1 ? $val : array_pop($val);
}

This function makes use of array_walk_recursive() to push into a placeholder array whatever value is pointed to by whichever key matches our needle key.

Proof code:

$array = array(
    'id' => 0,
    'name' => array(
        'it' => 'Tutti',
        'ca' => 'Tots',
        'es' => 'Todos',
        'de' => 'Alle' ,
        'fr' => 'Tous',
        'nl' => 'Alle',
        'en' => 'All'
    ),
    'languages' => array(
        "d" => 33
    ),
    'subTypes' => array(
        0 => array(
            'id' => 1,
            'name' => array(
                'it' => 'Tutti',
                'ca' => 'Tots',
                'es' => 'Todos',
                'de' => 'Alle' ,
                'fr' => 'Tous',
                'nl' => 'Alle',
                'en' => 'All'
            )
        ),
        1 => array(
            'id' => 1,
            'name' => array(
                'it' => 'Tutti',
                'ca' => 'Tots',
                'es' => 'Todos',
                'de' => 'Alle' ,
                'fr' => 'Tous',
                'nl' => 'Alle',
                'en' => 'All'
            )
        ),
        2 => array(
            'id' => 1,
            'name' => array(
                'it' => 'Tutti',
                'ca' => 'Tots',
                'es' => 'Todos',
                'de' => 'Alle' ,
                'fr' => 'Tous',
                'nl' => 'Alle',
                'en' => 'All'
            )
        )
    )
);

print_r(array_value_recursive("de", $array));

The above test code, which seeks all "de" keys, will output:

Array (
    [0] => Alle
    [1] => Alle
    [2] => Alle
    [3] => Alle
)


Validate IP4 address Validate IP Address
I got this problem during an interview. And would like to get some code review. I also wrote several tests with the expected output, and they all passed as expected.

Validate an IP address (IPv4). An address is valid if and only if it
is in the form "X.X.X.X", where each X is a number from 0 to 255.
For example, "12.34.5.6", "0.23.25.0", and "255.255.255.255" are valid
IP addresses, while "12.34.56.oops", "1.2.3.4.5", and
"123.235.153.425" are invalid IP addresses.

Examples:

"""

ip = '192.168.0.1'
output: true

ip = '0.0.0.0'
output: true

ip = '123.24.59.99'
output: true

ip = '192.168.123.456'
output: false
"""

def validateIP(ip):
  #split them by '.' , and store them in an array
  #check the array if the length is 4 length
  arr = ip.split('.')
  if len(arr) != 4:
    return False
  #0 check for special edge cases when non-digit
  #1. check if they are digit,
  #2. check if check the integer is between 0 and 255

  for part in arr:
    if len(part) > 1:
      if part[0] == '0':
        return False
    if not part.isdigit():
      return False
    digit = int(part)
    if digit < 0 or digit > 255:
      return False
  return True

#case#0

ip0="08.0.0.0" # False
test0= validateIP(ip0)
print(test0)

#case#1
ip1 = "192.168.0.1"
test1 = validateIP(ip1)
print(test1)

#case#2
ip2 = '0.0.0.0'
test2 = validateIP(ip2)
print(test2)

#case#3
ip3 = '123.24.59.99'
test3 = validateIP(ip3)
print(test3)

#case#4
ip4 = '192.168.123.456'
test4 = validateIP(ip4)
print(test4)

#case5
ip5 = "255.255.255.255"
test5 = validateIP(ip5)
print(test5)

def validateIP(ip):

I would expect a name starting is (a useful hint that it returns a Boolean rather than some more complex validation data structure) and explicitly mentioning IP v4 (since the current name is misleading). E.g. is_valid_IPv4_address.

  #split them by '.' , and store them in an array
  #check the array if the length is 4 length
  arr = ip.split('.')
  if len(arr) != 4:
    return False

The comments don't tell me anything which the code doesn't already. In general, good comments explain why, not what.

  #0 check for special edge cases when non-digit
  #1. check if they are digit,
  #2. check if check the integer is between 0 and 255

  for part in arr:
    .. various conditions which return False
  return True

IMO it would be more Pythonic to use all: I would boil the whole function down to

    parts = ip.split('.')
    return len(parts) == 4 and all(is_valid_IPv4_address_part(part) for part in parts)

    if len(part) > 1:
      if part[0] == '0':
        return False

This isn't in the spec. It's a reasonable constraint, but you should check with the person who gave you the spec before writing the code, or at least put in a comment saying that you're making an assumption about the true intentions of the specifier.

    if not part.isdigit():
      return False

This is buggy. (Before testing I thought there was an issue which should be bounced back to the specifier. Upon testing, I found that some of my test cases caused validateIP to throw an exception).

What is the expected output for these test cases?

¹.¹.¹.¹
١.١.١.١
...
①.①.①.①


Static method to create intents I had the idea to have a static method on an activity which creates intents that it can use. For example if I have an activity with a webview that can load html from a resource or a URL I would have 2 methods in the activity like so.

public static Intent createURLIntent(Context context, int titleId, String URL) {
    Intent intent = new Intent(context, WebViewActivity.class);
    intent.putExtra(TITLE_ID, titleId);
    intent.putExtra(URL, URL);
    return intent;
}

public static Intent createFileIntent(Context context, int titleId, String fileName) {
    Intent intent = new Intent(context, WebViewActivity.class);
    intent.putExtra(TITLE_ID, titleId);
    intent.putExtra(FILE, fileName);
    return intent;
}

The on create would look like this..

@Override
protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.webview_activity);
    int titleId = getIntent().getIntExtra(TITLE_ID, 0);
    setTitle(getString(titleId));
    if (getIntent().hasExtra(FILE)) {
        webview.loadUrl(getIntent().getStringExtra(FILE));
    } else {
        webview.loadUrl(getIntent().getStringExtra(URL));
    }
}

Then a class that wants to open that activity would use it in this way:

    Intent intent = WebViewActivity.createURLIntent(
            context, R.string.about_title, "www.google.com");
    startActivity(intent);

Is there any problems with using this approach?

Judging by this bit:

if (getIntent().hasExtra(FILE)) {
    webview.loadUrl(getIntent().getStringExtra(FILE));
} else {
    webview.loadUrl(getIntent().getStringExtra(URL));
}

It looks like the loadUrl method doesn't care whether the parameter you pass to it is a file path out a web address. Actually the term "url" has a general meaning that can encompass both file paths and web addresses. So I don't see the point of distinguishing these two in the intents: just use "URL", it doesn't matter if it's a path out web address. Again, this suggestion is based on the posted code.

If you really want to have two separate static method for some reason, then extract the common logic in these methods to a helper method.

private static Intent createIntent(Context context, int titleId, String key, String url) {
    Intent intent = new Intent(context, WebViewActivity.class);
    intent.putExtra(TITLE_ID, titleId);
    intent.putExtra(key, url);
    return intent;
}

public static Intent createURLIntent(Context context, int titleId, String url) {
    return createIntent(context, titleId, URL, url);
}

public static Intent createFileIntent(Context context, int titleId, String path {
    return createIntent(context, titleId, FILE, path);
}


Hiding element based on content I have a page that is part of a CMS and I cannot alter any existing HTML or CSS, but I can add additional custom JavaScript and CSS. There is some text on this page I want to hide and I found a solution but it feels kludgey. I can't be sure other pages won't have similar content on them, which is why I'm being so specific in my check.

This doesn't seem like an optimal way to do this, so I'm just curious how it could be improved.

The HTML:

Welcome!

The office is closed!

And the JavaScript I'm using to hide just "The office is closed":

function removeWarning() {
  var els = document.getElementsByClassName("hero-action");
  var el = els[0]

  if (el.textContent.indexOf("office" > -1) && el.textContent.indexOf("is" > -1) && el.textContent.indexOf("closed" > -1)) {
    el.style.display = "none";
   }
}

removeWarning();

Like I said, it works it just feels dumb. I know I could make an anonymous self calling function, but I'm more interested in how to improve the if check if there is a way.

Note: I'm not just checking for the entire string because the CMS is generating the
...
 block with some weird amount of whitespace and I was struggling to match spaces, newlines, etc.

if (el.textContent.indexOf("office is closed" > -1)) {
    el.style.display = "none";
}

Does this help?

Or if you know that hero-action is talking about Office, as in you are confirmed, then you can simply check for the word close rather than checking the whole string.


Java Map value access I am more or less a Java noob and wanted some feedback on a method I wrote. I wanted to avoid the pattern of checking if a key exists before accessing the value and throw an exception if the key does not exist.

Instead of doing this:

if (map.containsKey(key)) {
    return map.get(key)
}
else {
    throw new IllegalArgumentException();
}

everywhere in the code, I came up with the following solution:

class Utils {
    public static <K, V> V safeGet(final K key, Map<K,V> map) {
        if (map.containsKey(key)) {
            return map.get(key);
        } else {
            throw new IllegalArgumentException();
        }
    }
}

Is there anything wrong with this method? Do you see any problems with this approach?

As some other stated I am also convinced that there is less or no beneficial usage for enriching a simple associative map to throw an exception if an element is not found.

I suggest to go with a use case specific exception handling as the context is important where a not available value is an exceptional case.


Multithreaded telnet chat server in Java I have writed a multithreaded telnet chat application in Java. In main method I run telnet server. In server class at run method I bind in specified port (8189) and I spawn listener threads for every client. In every client listener (MessageHandler class) I print welcome message, prompt user for nickname and I listen for messages in loop. Server.sendAll method propagates message in created telnet chat room at bound port. If user enter "BYE" message, then it parts chat room.

App.java:

package pl.hubot.dev.telnet_chat;

public class App {
    public static void main(String[] args) {
        Server.run();
    }
}

Server.java:

package pl.hubot.dev.telnet_chat;

import java.io.IOException;
//import java.net.InetSocketAddress;
import java.net.ServerSocket;
import java.net.Socket;
import java.util.ArrayList;
import java.util.List;

class Server {
    static void run() {
        try {
            int i = 1;
            ServerSocket serverSocket = new ServerSocket(8189);
            //serverSocket.bind(new InetSocketAddress("192.168.1.2", 8189));

            while (true) {
                Socket incoming = serverSocket.accept();
                System.out.println("Spawning " + i);
                new MessageHandler(incoming);
                i++;
            }
        } catch (IOException ex) {
            ex.printStackTrace();
        }
    }

    static void sendAll(String message){
        for (MessageHandler handler : handlers) {
            handler.getOut().println(message);
        }
    }

    static List<MessageHandler> getHandlers() {
        return handlers;
    }

    private static List<MessageHandler> handlers = new ArrayList<MessageHandler>();
}

MessageHandler.java:

package pl.hubot.dev.telnet_chat;

import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.io.PrintWriter;
import java.net.Socket;
import java.util.Scanner;

public class MessageHandler extends Thread {
    MessageHandler(Socket incoming) {
        this.incoming = incoming;
        this.start();
    }

    @Override
    public void run() {
        try {
            try {
                Server.getHandlers().add(this);

                InputStream inputStream = incoming.getInputStream();
                OutputStream outputStream = incoming.getOutputStream();

                Scanner in = new Scanner(inputStream);
                out = new PrintWriter(outputStream, true /* autoFlush */);

                out.println("dev::hubot.pl - Telnet Chat Demo");

                out.println("Choose your nick:");
                String nick = "";
                nick = in.nextLine();

                out.println("Hello! Enter BYE to exit.");

                boolean done = false;
                while (!done && in.hasNextLine()) {
                    String line = in.nextLine();
                    Server.sendAll(nick + ": " + line);
                    if (line.trim().equals("BYE"))
                        done = true;
                }
//          } catch (InterruptedException ex) {

            } finally {
                incoming.close();
            }
        } catch (IOException ex) {
            ex.printStackTrace();
        }
    }

    PrintWriter getOut() {
        return out;
    }

    private Socket incoming;
    private PrintWriter out;
}

I scanned the code of Server 3 times, looking for where something is added to handlers. Because that's where I expected it.
I thought the code might be broken or that sendAll is an unused method, because handlers seems always empty.
Finally I found that it's the MessageHandler instances that use Server.getHandlers(), and append themselves.

Why is MessageHandler aware of the handlers of the server?
MessageHandler doesn't really need to be aware of the collection of handlers in the server, does it?
It doesn't, and therefore shouldn't.
In fact, it would be better if the message handlers were cleanly isolated from each other.

By the same line of reasoning,
why is MessageHandler aware of the Server at all?
It would be better if it wasn't.
If you want to let message handlers interact with the server in some way,
it would be better to create a dedicated interface for that,
and pass an instance of that interface to message handlers.

It's not recommended to start threads from constructors.
It would be better to let Server call .run() on the message handlers right after construction.

Do not nest try blocks within try blocks.
Break up the large try-catch to smaller blocks with minimal code within any try. The nesting will naturally disappear,
and the code will become easier to understand and to debug.

In Java it's recommended to declare class fields at the top, followed by constructors, followed by methods.