New password generation public static GeneratePassword(minPassLength) {
    let small = "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".split(' ');
    let big = "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".split(' ');
    let numbers = "0 1 2 3 4 5 6 7 8 9".split(' ');
    let special = "! \" # $ % & ( ) * + - . : ; < = > ? @ [ \ ] _ { | }".split(' ');

    var pass = "";
    for (let i = 0; i < minPassLength; i++) {
      pass += small[this.randomIntFromInterval(0, small.length - 1)];
    }
    for (let i = 0; i < minPassLength / 4; i++) {
      pass += big[this.randomIntFromInterval(0, big.length - 1)];
    }
    for (let i = 0; i < minPassLength / 4; i++) {
      pass += numbers[this.randomIntFromInterval(0, numbers.length - 1)];
    }
    for (let i = 0; i < minPassLength / 4; i++) {
      pass += special[this.randomIntFromInterval(0, special.length - 1)];
    }
    pass = pass.split('').sort(function () { return 0.5 - Math.random() }).join('');
    return pass;
 }

  private static randomIntFromInterval(min, max) // min and max included
  {
    return Math.floor(Math.random() * (max - min + 1) + min);
  }

A simple function to generate a new password with at least X (this is function parameter) chars including 1 special, 1 number and 1 uppercase.

Please a code review :-)

You've four for-loops that are doing the same with different input. Make a function for this logic and call it with the different arrays.

I don't know how typescript handles it and how often you call this function, but it could be better to define your characters outside of this because generating them every time you call this function is not necessary.

Besides, you don't need to define your alphabet twice. You can use toUpperCase/toLowerCase.

And last: From an outsiders perspective, I have no idea what this code is for:

 return 0.5 - Math.random()

You should make a function for it or at the very least, write a comment for it.


Refactoring legacy code for unit testing I'm trying to refactor some code for unit testing and was hoping you could critique it.

This is the original method:

public class MyNonRefactoredClass
{
   public List<MyClass> DoSomething()
  {
      List<MyClass> data = GetData();

      if (data.Count == 0)
        return new List<MyClass>();

      data = GetFormattedData(data);

      if (data.Count == 0)
        return new List<MyClass>();

      //Do some more stuff

      return data;
   }

  public List<MyClass> GetData()
  {
      //Do db query and return data
  }

  public List<MyClass> GetFormattedData(List<MyClass> Data)
  {
      //Do some logic on Data variable. Might return empty if data is not what I like
  }

}

Here is my new approach. I have done it like this as I believe I need an entry point to try and access methods that I want to test.

public class MyRefactoredClass
{
  public List<MyClass> DoSomething()
  {
    //Dependency Injection needs to work somehow here
    MySeparateClass newClass = new MySeperateClass(new DataProvider());
    List<MyClass> dbData = newClass.GetData();
    if (db.Data.Count == 0)
      return new List<MyClass>();
    dbData = newClass.GetFormattedData();
    if (db.Data.Count == 0)
      return new List<MyClass>();
    //Continue method doing other things...
    }
}

public class MySeparateClass
{
  private IDataProvider dbProvider;

  public MySeparateClass(IDataProvider DataProvider)
  {
    dbProvider = DataProvider;
  }

  public List<MyClass> GetData()
  {
    //Do DB stuff
    return dbProvider.GetData();
  }

  public List<MyClass> GetFormattedData(List<MyClass> Data)
  {
    //Format
  }
}

[Test]
public DoSomething_DBReturnsNoData_ReturnEmptyList()
{
  //Setup mock of IDataProvider
  //Tell mock to not return data from GetData
  //Assert that result.count == 0
}

[Test]
public DoSomething_FormattedReturnsNoData_ReturnsEmptyList()
{
  //Setup mock of IDataProvider
  //Tell mock to return some data from GetData
  //Setup mock of MySeparateClass
  //Tell mock to return no data from GetFormattedData
  //Assert that result.Count == 0
}

Looking at your code I see three responsibilities:

the Data Provider (component that hits the db)
the Data Formatter
the business service (your class) that uses the other two components to execute a business flow.

According to the SRP, one class should have one responsibility, so we should end up having three classes.

Therefore, I'd separate out the data provider and formatter interfaces, like so:

public interface IDataProvider
{
    List<MyClass> GetData();
}

public interface IDataFormatter
{
    List<MyClass> GetFormattedData(List<MyClass> data);
}

and refactor our business class to use these dependencies:

public class RefactoredClass
{
    private readonly IDataProvider _dataProvider;
    private readonly IDataFormatter _dataFormatter;

    public RefactoredClass(IDataProvider dataProvider, IDataFormatter dataFormatter)
    {
        _dataProvider = dataProvider;
        _dataFormatter = dataFormatter;
    }

    public List<MyClass> DoSomething()
    {
        var data = _dataProvider.GetData();

        if (data.Count == 0)
            return new List<MyClass>();

        data = _dataFormatter.GetFormattedData(data);

        if (data.Count == 0)
            return new List<MyClass>();

        //Do some more stuff

        return data;
    }
}

Now you can write all relevant unit tests, for each system component in isolation:

you can test RefactoredClass providing your mocks/stubs for the two dependencies
you can test the DataFormatter(s) in separate unit test(s)
you can test the concrete DataProvider using integration tests (since this component touches the database)

Regarding Depenedency Injection, this is how you should compose this component somewhere in your application root:

var dataProvider = new SqlDataProvider();
var dataFormatter = new MySlickDataFormatter();

var businessComponent = new RefactoredClass(
    dataProvider,
    dataFormatter);

This technique of manually instantiating the needed component and its dependencies is called "Poor man's DI". If things get any more complicated, you should consider using an IoC container for this.

Update:

Your tests make sense, but there are more tests to be added. For example, the formatting logic is not tested.

Also, regarding your second test - you have no mocks in there. The MySeparateClass instance is the system under test and the IDataProvider instance is a stub. Your test does make sense; that's just a small naming issue.

A mock is basically a fake instance that you assert against.

If you google for "mocks and stubs" you'll find plenty of articles on this subject that you can read. I also recommend the book "The Art of Unit Testing" by Roy Osherove - a great resource on this subject.


CRAP index (56) in Web Scraper Engine I am working on a Web Scraper for the first time using Test Driven Development, however I have caught myself into a huge CRAP (Change Risk Anti-Patterns) index (56) and I can not seem to find a solution about this.

<?php

/**
 * Code Snippet MIT licensed taken from proprietary source.
 */

namespace WebScraper;
use WebScraper\Contracts\Scraper;
use WebScraper\Collections\BaseCollection;
use WebScraper\Exceptions\EngineIgnitionException;

class Engine
{
    protected $targetAdAge;

    protected $currentAdAge = 1;

    protected $targetPage  = 999;

    protected $currentPage = 1;

    public function __construct(Scraper $scraper, BaseCollection $collection)
    {
        $this->scraper    = $scraper;
        $this->collection = $collection;
    }

    public function start($targetAdAge = null)
    {
        if ($this->collection->count() >= 1)
        {
            throw new EngineIgnitionException("Engine can not be ran twice in the same instance.");
        }

        if (array_key_exists('adage', $requestData = $scraper->queryBuilder()->buildArray()))
        {
            $this->targetAdAge = $requestData['adage'];
        }

        if (! $this->targetAdAge)
        {
            throw new EngineIgnitionException("targetAdAge value is a core part of the Engine and it is missing.");
        }

        for ($this->currentAdAge = 1; $this->currentAdAge <= $this->targetAdAge; ++$this->currentAdAge)
        {
            for ($this->currentPage = 1; $this->currentPage <= $this->targetPage; ++$this->currentPage)
            {
                $this->scraper->queryBuilder()->adage($this->currentAdAge)->page($this->currentPage);
                $parser = $this->scraper->extract()->parse();
                $items = $parser->items();
                $this->targetPage = $parser->maxPages();
                foreach ($items as &$item)
                {
                    $item->age = (new DateTime('now'))->modify("-{$this->currentAdAge} day");
                    $this->collection->push($item);
                }

                unset($item);
            }
        }

        return $this->collection;
    }
}

The main issue is in the start() function.

Basically, what this class does is building an Engine to web scrape a website that uses pagination to show their data, and because of so I am forced to use iterators and scrape every single page to gather all the data. Line 48 is the function that does the dirty job and scrapes the website.

The thing is, this function is likely to take even minutes to complete if there's a lot of data to extract or if scraping filters are very broad. This is the only function where I really do not know how to refactor it in a better way. The good thing, however, is that every other single class has a low CRAP index (< 5) and Code Coverage is above 85%.

How do I refactor the iterator of page and adAge?

The code looks mostly fine to me. If you feel that the code isn't as clean as it should be, or just want it to pass the metric, what you could do is split it into functions:

            $this->scraper->queryBuilder()->adage($this->currentAdAge)->page($this->currentPage);
            $parser = $this->scraper->extract()->parse();
            $items = $parser->items();
            $this->targetPage = $parser->maxPages();

That goes into a private function which sets targetPage and returns $items... you could call it parsePageToItems

And this

            foreach ($items as &$item)
            {
                $item->age = (new DateTime('now'))->modify("-{$this->currentAdAge} day");
                $this->collection->push($item);
            }

            unset($item);

Could go into a separate function mapItems.

You could then write a test for mapItems in particular, increasing coverage and further reducing your CRAP score.


Hexadecimal to decimal converter in C (htoi.c) Is there any way I can make this code more efficient? I'm new to C, any suggestion is welcome!

#include <stdio.h>
#include <string.h>
#include <math.h>

#define LIMIT 1024

unsigned long htoi(char s[], int len);

int main(void){
    char line[LIMIT+1];

    int i=0;
    for(int c=0; i<LIMIT-1 && (c = getchar()) != '\n' && c != EOF; ++i){
        line[i] = c;
    }
    line[i] = '\0';

    printf("%lu\n", htoi(line, i));
}

unsigned long htoi(char s[], int len){

    unsigned long total = 0;
    unsigned short currdigit = 0;
    int power = 0;

    for(int i=len; i>-1; --i){

        if(s[i] == 'X' || s[i] == 'x' || s[i] == 0){
            continue;
        }

        if(s[i] >= '0' && s[i] <= '9'){ // if it is a numerical char
            currdigit = s[i]-'0';
        }
        else{
            if(s[i] >= 'a' && s[i] <= 'z'){ // if it's a lower case char
                currdigit = s[i] - 'a';
            }
            else{ // if not, then we assume it is an upper case char (should I?)
                currdigit = s[i] - 'A';
            }

            currdigit+=10;
        }

        total += (currdigit * pow(16, power));

        ++power;
    }

    return total;
}

I have a few questions as well:

The book I am using to learn C didn't reach the point of teaching pointers yet; however, I do know at least the basics. Is there any way pointers could be used in this program?
Should I make change the parameter char s[] to const char s[] since I am not changing any of its elements' values? Is that common practice?
Should I use short instead of int when I know the number won't surpass great quantities? As an instance, when developing a game, if I know my player's health is from 0 to 100, should I be using a short to store that value? And should it be unsigned as well if I know it won't be negative?

#define LIMIT 1024: magic constants should be named well. What is the limit for? You are also storing the result in an unsigned long and contain much less than 1021 hex digits (the last 3 are for 0x and the null terminator).

Operators should be spaced for better readability: int i = 0; and for (int

I'm a bit rusty with C, but I think fgets can be used instead of rolling your own getc loop

Inside htoi, a single tolower or toupper removes the need to check lowercase and capitals

if(s[i] >= 'a' && s[i] <= 'z'): hex digits only contain 0-9 and a-f (not a-z)

pow(16, power): in this case, pow(16, power) = 1 << (4 * power) (see bitshifts). I would split the 4 * power so that it is more clear what it represents (digit position)

Some changes to the algorithm are necessary: input of 1z00x000 gives 53477376 and kibe gives 86718. I would check if there is a leading 0x and if there is, to skip it (if not, return an error code)

if not, then we assume it is an upper case char and And should it be unsigned as well if I know it won't be negative: with your code, I can get currdigit to underflow by choosing (for example) a plus sign as input.


Navigation bar built from fetched JSON data I have finally been able to create my little navigation plugin in an object-oriented way that reads the li from a JSON file.

Now, is my code efficient? Especially, the click function within getRegion function?

Is this the correct way to handle the click events in OO?

Here's is working version: PLNKR

'use strict';
if (typeof Object.create !== 'function') {

    Object.create = function (obj) {
        function F () {
        }
        F.prototype = obj;
        return new F ();
    };
}

(function ($, window, document, undefined) {

    var Navigation = {
        init: function (options, elem) {
            var self = this;

            self.elem = elem;
            self.$elem = $ (elem);

            self.url = 'data.json';

            self.navigation = ( typeof options === 'string' )
                ? options
                : options.navigation;

            self.options = $.extend ({}, $.fn.drawNavigation.options, options);

            self.cycle ();
        },

        cycle: function () {
            var self = this;
            self.fetch ().done (function (results) {
                self.buildFrag (results);
                self.display ();
                self.getRegion ();
            });
        },

        buildFrag: function (results) {
            var self = this;
            var navigationRegion = self.options.navigation;
            self.list = $.map (results[navigationRegion].navigation, function (obj, i) {
                return $ (self.options.wrapEachWith).append (obj);
            });
        },

        fetch: function () {
            return $.ajax ({
                url: this.url
            });
        },

        display: function () {
            this.$elem.hide().html (this.list).fadeIn(800);
        },

        activate: function (item) {
            $ (item).siblings ().removeClass ('flag-active');
            $ (item).addClass ('flag-active');
        },

        getRegion: function () {
            var self = this;
            $ ('#flags').find ('div').on ('click', function () {
                //activate flag
                self.activate (this);
                self.options.navigation = $ (this).attr ('id').split ('_')[0];
                self.fetch ().done (function (results) {
                    self.buildFrag (results);
                    self.display ();
                });
            });

        }
    };

    $.fn.drawNavigation = function (options) {
        return this.each (function () {
            var navigationBar = Object.create (Navigation);
            navigationBar.init (options, this);
        });
    };

    //default
    $.fn.drawNavigation.options = {
        navigation: 'uk',
        wrapEachWith: '

'
    }

}) (jQuery, window, document);

$ (function () {
    $('#navigation').find('ul').drawNavigation({});
});

Now, is my code efficient?

We don't care. There's not much going on here, there are not thousands of elements in the DOM, and there are no tight data processing loops. If the UI feels sluggish, something must have been gone horribly wrong.

However, there is one inefficiency in your code that matters: The JSON file is fetched on really every single click. I'm pretty sure this is not desired.

It would be OK if you expect it to change really often (that is, < 15min); or if the response would be dynamically created and you did transmit the currently selected option as a parameter.

  Especially, the click function within getRegion function? Is this the correct way to handle the click events in OO?

Yes, that's fine.

However, a few oddities I found in your code:

You use Object.create and an init function. That's OK if you want to do it that way, and good to understand prototypical inheritance. Yet, I think using a constructor function and new is simpler, as it requires only a single call for instantiation.
Using self in your init function is unnecessary. Just use this, as you do in fetch or display.
Why assign .elem? You never need it.
The url should definitely not be a constant, but go into the options. 'data.json' may be the default value.
navigation is not the most descriptive property name. From the usage, I'd expect an adjective like current or selected in the name.
You use options.navigation to create self.navigation before assigning it a default value.
cycle is not an optimal name either, it sounds as if it cycles through the options periodically. Name it after what the method actually does - or just inline it into the initialisation code.
I'm not entirely sure what getRegion does and whether it needs to go into the fetch callback. If you move it outside, you might avoid the code duplication with the fetch in the click handler below.
You shouldn't use options.navigation for storing the state, but the this.navigation property (which you created in init but never use it) - both in buildFrag and the click handler.
Why does buildFrag assign to that list property? Instead, return your result from the method! display should then take this as a parameter (or you inline it right away).
getRegion is misnamed as well. The only code that is "getting the region" is $ (this).attr ('id').split ('_')[0]. What your whole method does is something like setupRegionSelectHandlers or so.
What is $ ('#flags').find ('div')? This shouldn't be static, make the selector an option. Also, it would get clearer if those selection-sensitive divs were stored in a property similar to .$elem - I assume they're not descendants of each other?
As said above, the clicks shouldn't fetch the file again. Instead, store the promise for the result on your instance, and reuse that for every display.

I'd do something like this:

(function ($, window, document, undefined) {

    function Navigation(options, elem) {
        if (typeof options === 'string')
            options = {navigation: options};
        this.options = $.extend ({}, $.fn.drawNavigation.options, options);

        this.$elem = $(elem);
        this.$selects = $(this.options.select)

        this.navigatedRegion = this.options.navigation;

        this.fetched = $.ajax({
            url: this.options.url
        });

        this.display();
        this.setupSelectionEvents();
    }

    Navigation.prototype.display = function () {
        var self = this;
        return this.fetched.then(function(results) {
            self.$elem.hide().html(self.buildFrag (results)).fadeIn(800);
        });
    };

    Navigation.prototype.buildFrag = function(results) {
        var self = this;
        return $.map(results[this.navigatedRegion].navigation, function(obj, i) {
            return $(self.options.wrapEachWith).append(obj);
        });
    };

    function activate(item) {
        $(item).siblings().removeClass('flag-active');
        $(item).addClass('flag-active');
    }

    function getRegion(item) {
        return $(item).attr('id').split('_')[0];
    }

    Navigation.prototype.setupSelectionEvents = function() {
        var self = this;
        this.$selects.on('click', function () {
            activate(this);
            self.navigatedRegion = getRegion(this);
            self.display();
        });
    };

    $.fn.drawNavigation = function(options) {
        return this.each(function() {
            new Navigation(options, this);
        });
    };

    //default
    $.fn.drawNavigation.options = {
        navigation: 'uk',
        wrapEachWith: '

',
        select: '#flags div',
        url: 'data.json'
    };

}(jQuery, window, document));


Find files by pattern and copy to target location This is my solution to the Chapter 9 exercise in Automate the Boring Stuff:

  Selective Copy

  Write a program that walks through a folder tree and searches for files with a certain file extension (such as .pdf or .jpg). Copy these files from whatever location they are in to a new folder.

#!/usr/local/bin/python3
#selectiveCopy.py - Walks through a folder tree with the extension .PDF and copies them to a new folder.

import re
import os
import shutil

filePdf = re.compile(r"""^(.*?)          #text before extension
          [.][p][d][f]                   #extension .pdf
          """, re.VERBOSE)

def selectiveCopy(fileExtension):
    for pdfFiles in os.listdir("/Users//Desktop/newPDFs"):
        locatedFiles = filePdf.search(pdfFiles)
        if locatedFiles != None:
            print(locatedFiles)
            files_to_copy = os.path.join("/Users/my_user_name/Desktop/newPDFs", pdfFiles)   `
            shutil.copy(files_to_copy, "/Users/my_user_name/Desktop/oldPDFs")
            print("The following file has been copied " + files_to_copy)
selectiveCopy(filePdf)

I appreciate for probably all of you on here that it's not a very difficult exercise, but I've been working through this book slowly and have just decided it's probably a good idea to start putting them on a Github page.  I wanted to know if I could improve the code to make it more efficient or if there are some things I've included that aren't necessarily considered "standard practice".

If you want to recurse also into sub-folders, you should use os.walk:

import os
import shutil

def get_files_recursively(start_directory, filter_extension=None):
    for root, _, files in os.walk(start_directory):
        for file in files:
            if filter_extension is None or file.lower().endswith(filter_extension):
                yield os.path.join(root, file)

def selective_copy(source, target, file_extension=None):
    for file in get_files_recursively(source, file_extension):
        print(file)
        shutil.copy(file, target)
        print("The following file has been copied", file)

if __name__ == "__main__":
    selective_copy("/Users/my_user_name/Desktop/newPDFs",
                   "/Users/my_user_name/Desktop/oldPDFs",
                   ".pdf")

As stated in the documentation, this is actually faster than os.listdir since Python 3.5:

  This function now calls os.scandir() instead of os.listdir(), making
  it faster by reducing the number of calls to os.stat().

I also

added a if __name__ == "__main__": guard to allow importing this module from another script without running the function
pulled the constants out into the calling code to make it re-usable
used is instead of != to compare to None
used str.endswith instead of a regular expression to avoid some overhead
changed the names to adhere to Python's official style-guide, PEP8.


Response to an answer: Solution to maxMirror problem Garrett Openshaw had an okay answer to Codingbat maxMirror challenge

But there were a couple of issues with the code he gave, so I decided to review it and make it a little bit better.

I don't have access to Java here at work so I instead changed the code in C# which wasn't too much trouble.

I added a List<int[]> to keep track of all the test cases and added an extra loop to the outside of the the rest of the code.

I moved some variables as I mentioned in my comment to Garret's answer, and also moved some variables to the correct scope.  It seems like there should be more I can do to simplify this answer.

There was indentation and bracketing issues that made it difficult to read that I also changed.

List<int[]> sequences = new List<int[]> {
     new int[] {1, 2, 1, 2, 1},
     new int[] {1, 2, 3, 8, 9, 3, 2, 1},
     new int[] {1, 2, 1, 4},
     new int[] {7, 1, 2, 9, 7, 2, 1}
 };

foreach (var sequence in sequences)
{
    int maxLength = 1;
    int endIndex = sequence.Length - 1;
    for (int frontIndex = 0; frontIndex < endIndex; frontIndex++)
    {
        for (int i = endIndex; i > frontIndex; i--)
        {
           if (sequence[frontIndex] == sequence[i])
            {
                var currentLength = 0;
                while (frontIndex + currentLength < sequence.Length && i - currentLength >= 0)
                {
                    if (sequence[frontIndex + currentLength] == sequence[i - currentLength])
                    {
                        currentLength++;
                    }
                    else break;
                }
                if (currentLength > maxLength)
                {
                    maxLength = currentLength;
                }
            }
        }
    }
    Console.WriteLine("Max length is " + maxLength);
    Console.ReadLine();
}

This is looking very good.

First thing I would do is split out a method, GetMaxMirror, and rename maxLength to maxMirror. Then I would put braces around the break, maybe invert the condition if (sequence[frontIndex] == sequence[i]) to reduce nesting, and call it a day.

Ah, whoops, this fails on an empty array by returning the answer 1. Fix, add a unit test, and now we're really done.

Until... I notice that the complexity looks like it's \$O(n^3)\$. Let's increment a counter in the inner while:

while (frontIndex + currentLength < sequence.Length && i - currentLength >= 0)
{
    steps++;
    ...

And run it on our test cases:

GetMaxMirror took 15 steps.
Max length is 5
GetMaxMirror took 9 steps.
Max length is 3
GetMaxMirror took 3 steps.
Max length is 3
GetMaxMirror took 7 steps.
Max length is 2

Not bad!

And after a bit of playing around, we find that it runs badly on this input

private static int[] GetBadInput(int length)
{
    var result = new int[length];
    result[length / 2 - 1] = 1;
    return result;
}

Now let's run it again:

sequences.Add(GetBadInput(1000));
sequences.Add(GetBadInput(2000));
sequences.Add(GetBadInput(3000));

GetMaxMirror took 104664252 steps.
Max length is 999
GetMaxMirror took 835328502 steps.
Max length is 1999
GetMaxMirror took -1477974544 steps.
Max length is 2999

Ahhhh... integer overflow in our counter? Not good.

So what can we do? Let's look at the problem statement again.

  We'll say that a "mirror" section in an array is a group of contiguous
  elements such that somewhere in the array, the same group appears in
  reverse order. For example, the largest mirror section in {1, 2, 3, 8, 9, 3, 2, 1} is length 3 (the {1, 2, 3} part). Return the size of the
  largest mirror section found in the given array.

So we're looking for the longest contiguous sequence that appears in the input, and its reversal. This sounds like another problem, longest common substring (LCS).

In particular,

$$
maxMirror(S) = LCS(S, T)
$$

where \$T\$ is \$S\$ reversed. If we can solve LCS, then we have a solution to maxMirror. Using dynamic programming, we can solve LCS in \$O(n^2)\$ time, with \$O(n)\$ space.

The pseudocode in that article is pretty good, so let's translate that to C#:

private static int GetMaxMirrorLcs(int[] sequence)
{
    var length = sequence.Length;
    var prevRow = new int[length];

    var maxMirror = 0;
    for (var i = 0; i < length; i++)
    {
        var row = new int[length];
        for (var j = 0; j < length; j++)
        {
            if (sequence[i] != sequence[length - j - 1])
            {
                continue;
            }

            row[j] = (i == 0 || j == 0)
                ? 1
                : prevRow[j - 1] + 1;
            maxMirror = Math.Max(maxMirror, row[j]);
        }

        prevRow = row;
    }

    return maxMirror;
}

Since we split out our method before, it's easy to test that these give the same results:

foreach (var sequence in sequences)
{
    int maxMirror = GetMaxMirror(sequence);
    int maxMirrorLcs = GetMaxMirrorLcs(sequence);
    if (maxMirror != maxMirrorLcs)
    {
        Console.WriteLine("Answers don't match!");
        Console.WriteLine("Input: [{0}].", string.Join(", ", sequence));
        Console.WriteLine("GetMaxMirror: {0}. GetMaxMirrorLcs: {1}.", maxMirror, maxMirrorLcs);
    }
}


Correct use of Option in scala? I'm somewhat new to Scala and am not sure if I'm doing things in a very Scala-esque way.

In particular, I'm not sure if I'm using Option correctly, since by calling .isDefined, I'm basically just doing ye olde Java null check. Also, the getMethodAndAnnotationFromInfaces method could probably be done better.

Please point out ways in which I could better leverage language features in the below code:

for (val method <- clazz.getMethods) {
        val methodAndAnnotation = method.getAnnotation(annotationType) match {
            case annotation: MessageHandler => Some((method, annotation))
            case _ => getMethodAndAnnotationFromInterfaces(clazz.getInterfaces, method, annotationType)
        }

        if (methodAndAnnotation.isDefined){
            // do something
        }
    }

getMethodAndAnnotationFromInterfaces function:

def getMethodAndAnnotationFromInterfaces(interfaces: Array[java.lang.Class[_]], method: Method, annotationType: Class[_ <: java.lang.annotation.Annotation])
    : Option[(Method, java.lang.annotation.Annotation)] = {

    @tailrec
    def getMethodAndAnnotationFromInterfacesInner(interfaces: Array[java.lang.Class[_]], acc: Option[(Method, java.lang.annotation.Annotation)])
        : Option[(Method, java.lang.annotation.Annotation)] = {
        interfaces.length match {
            case 0 => acc
            case _ =>
                try {
                    interfaces.head.getMethod(method.getName, method.getParameterTypes: _*) match {
                        case newMethod: Method =>
                            val newAnnotation = newMethod.getAnnotation(annotationType)
                            if (newAnnotation != null && acc.isDefined) {
                                throw new RuntimeException("The annotation is applied to the method in more than one interface of the class.")
                            }
                            getMethodAndAnnotationFromInterfacesInner(interfaces.tail, Some((newMethod, newAnnotation)))
                        case _ => getMethodAndAnnotationFromInterfacesInner(interfaces.tail, acc)
                    }
                } catch {
                    case e: NoSuchMethodException => getMethodAndAnnotationFromInterfacesInner(interfaces.tail, acc)
                }
        }
    }

    getMethodAndAnnotationFromInterfacesInner(interfaces, None)
}

You could write it like this:

for (val method <- clazz.getMethods) {
  val methodAndAnnotation = method.getAnnotation(annotationType) match {
    case annotation: MessageHandler => Some((method, annotation))
    case _                          => getMethodAndAnnotationFromInterfaces(clazz.getInterfaces, method, annotationType)
  }

  methodAndAnnotation match {
    case Some((method, annotation)) => // do something
    case None => //whatever is relevant
  }
}

Or shorter:

for (val method <- clazz.getMethods) {
  method.getAnnotation(annotationType) match {
    case annotation: MessageHandler =>  // do something with method and annotation
    case _                          => {
      val v = getMethodAndAnnotationFromInterfaces(clazz.getInterfaces, method, annotationType)
      //whatever is relevant
    }
  }
}

But it depends on what you want to do / if you have something to do when getMethodAndAnnotationFromInterfaces returns None.


Your code is too imperative while groovy favours Functional Programming:

def largest_product =
  (0 .. number.size() - digits)
    .map {number[i..<(i + digits)].inject(1) {acc, it -> acc * (it as long)}}
    .max

println largest_product

As you can see, the code is now without all those confusing variables, and the algorithm is more clearly expressed.


Old-way of asynchronous programming in ASP.NET MVC 3 On ASP.NET MVC, I try to write an async Controller action with the old asynchronous programming model (actually, it is the current one, new one is still a CTP).

Here, I am trying to run 4 operations in parallel and it worked great. Here is the complete code:

public class SampleController : AsyncController {

    public void IndexAsync() {

        AsyncManager.OutstandingOperations.Increment(4);

        var task1 = Task<string>.Factory.StartNew(() => {

            return GetReponse1();
        });
        var task2 = Task<string>.Factory.StartNew(() => {

            return GetResponse2();
        });
        var task3 = Task<string>.Factory.StartNew(() => {

            return GetResponse3();
        });
        var task4 = Task<string>.Factory.StartNew(() => {

            return GetResponse4();
        });

        task1.ContinueWith(t => {

            AsyncManager.Parameters["headers1"] = t.Result;
            AsyncManager.OutstandingOperations.Decrement();
        });

        task2.ContinueWith(t => {

            AsyncManager.Parameters["headers2"] = t.Result;
            AsyncManager.OutstandingOperations.Decrement();
        });

        task3.ContinueWith(t => {

            AsyncManager.Parameters["headers3"] = t.Result;
            AsyncManager.OutstandingOperations.Decrement();
        });

        task4.ContinueWith(t => {

            AsyncManager.Parameters["headers4"] = t.Result;
            AsyncManager.OutstandingOperations.Decrement();
        });

        task3.ContinueWith(t => {

            AsyncManager.OutstandingOperations.Decrement();

        }, TaskContinuationOptions.OnlyOnFaulted);
    }

    public ActionResult IndexCompleted(string headers1, string headers2, string headers3, string headers4) {

        ViewBag.Headers = string.Join("

", headers1, headers2, headers3, headers4);

        return View();
    }

    public ActionResult Index2() {

        ViewBag.Headers = string.Join("

", GetReponse1(), GetResponse2(), GetResponse3(), GetResponse4());

        return View();
    }

    #region helpers

    string GetReponse1() {

        var req = (HttpWebRequest)WebRequest.Create("http://www.twitter.com");
        req.Method = "HEAD";

        var resp = (HttpWebResponse)req.GetResponse();

        return FormatHeaders(resp.Headers);
    }

    string GetResponse2() {

        var req2 = (HttpWebRequest)WebRequest.Create("http://stackoverflow.com");
        req2.Method = "HEAD";

        var resp2 = (HttpWebResponse)req2.GetResponse();

        return FormatHeaders(resp2.Headers);
    }

    string GetResponse3() {

        var req = (HttpWebRequest)WebRequest.Create("http://google.com");
        req.Method = "HEAD";

        var resp = (HttpWebResponse)req.GetResponse();

        return FormatHeaders(resp.Headers);
    }

    string GetResponse4() {

        var req = (HttpWebRequest)WebRequest.Create("http://github.com");
        req.Method = "HEAD";

        var resp = (HttpWebResponse)req.GetResponse();

        return FormatHeaders(resp.Headers);
    }

    private static string FormatHeaders(WebHeaderCollection headers) {

        var headerStrings = from header in headers.Keys.Cast<string>()
                            select string.Format("{0}: {1}", header, headers[header]);

        return string.Join("
", headerStrings.ToArray());
    }

    #endregion

}

I have also Index2 method here which is synchronous and does the same thing.

I compare two operation execution times and there is major difference (approx. 2 seconds)

But I think I am missing lots of things here (exception handling, timeouts, etc). I only implement the exception handling on task3 but I don't think it is the right way of doing that.

What is your opinion on this code? How can it be improved?

There is a lot of code repetition. I would create a custom class that inherits from Task and make it generic enough so you can have an array of your custom class and just loop through that array to do what you need to do.

class TaskController<T> : Task<T>
{
    public TaskController(AsyncManager asyncManager, string asyncManagerParameter, Func<T> callback) : base(callback)
    {
        this.ContinueWith(t =>
        {
            //just for demonstration
            Console.WriteLine(t.Result);

            // Do some work with your asyncManager
            asyncManager.Parameters[asyncManagerParameter] = t.Result.ToString();
        });
    }
}

I would also have GetResponse take a string uri as parameter instead of making a new GetResponse for each url that you want to use.

public static string GetResponse(string uri)
{
    HttpWebRequest req = (HttpWebRequest)WebRequest.Create(uri);
    req.Method = "HEAD";

    HttpWebResponse resp = (HttpWebResponse)req.GetResponse();

    return resp.Headers.ToString(); // just changed this for demonstration purposes
}

and finally I would use it like this

AsyncManager asyncManager = new AsyncManager();

TaskController<string>[] taskControllers = {
    new TaskController<string>(asyncManager, "header1", () => GetResponse("http://google.com")),
    new TaskController<string>(asyncManager, "header3", () => GetResponse("http://www.twitter.com"))
};

foreach (TaskController<string> tc in taskControllers)
{
    tc.Start();
}

It is much less code, you can reuse it later, you can very easily add a new TaskController if you want to work with another URL, and it's a lot more readable.

To use this you will need to just edit this.ContinueWith inside TaskController and have it do whatever you want (decrement, etc)


Completed Hangman game This works for the most part. I am very new to programming so be aware.

import java.util.*;
public class HangManP5
{
private static Scanner k;
public static void main(String[] args)
{
int attempts = 10;
int wordLength;
boolean solved;
k = new Scanner(System.in);

System.out.println("Hey, what's your name?");
String name = k.nextLine();
System.out.println(name+ ", hey! This is a hangman game! (Some answers have two words, usually the longer ones\n");
String word = getRandomWord();
int len = word.length();
char[] temp = new char[len];
for(int i = 0; i < temp.length; i++)
{
    temp[i] = '*';
}
System.out.print("\n");
System.out.print("Word to date: "+len+" letters.");
while (attempts <= 10 && attempts > 0)
{
    System.out.println("\nAttempts left: " + attempts);
    System.out.print("Enter letter: ");
    String test = k.next();
    if(test.length() != 1)
    {
        System.out.println("Please enter 1 character");
        continue;
    }
    char testChar = test.charAt(0);
    int foundPos = -2;
    int foundCount = 0;
    while((foundPos = word.indexOf(testChar, foundPos + 1)) != -1)
    {
        temp[foundPos] = testChar;
        foundCount++;
        len--;
    }
    if(foundCount == 0)
    {
        System.out.println("Sorry, didn't find any matches for " + test);
    }
    else
    {
        System.out.println("Found " + foundCount + " matches for " + test);
    }
    for(int i = 0; i < temp.length; i++)
    {
        System.out.print(temp[i]);
    }
    System.out.println();
    if(len == 0)
    {
        break; //Solved!
    }
    attempts--;
}
if(len == 0)
{
    System.out.println("\n---------------------------");
    System.out.println("You did iiiiiiit!");
}
else
{
    System.out.println("\n---------------------------");
    System.out.println("Better luck next time, you didn't get the word!");
    System.out.println("It was \"" + word + "\"");
}
}
public static String getRandomWord()
{
    switch(new Random().nextInt(5))
    {
        case 1:
            return "peace";
        case 2:
            return "nuts";
        case 3:
            return "dankmemes";
        case 4:
            return "fizz";
        case 5:
            return "awesome";
        default:
            throw new IllegalStateException("Something went wrong!");
    }
}
}

This program is not bad, for a beginner.  You split the getRandomWord() routine into its own function, which is good.  You should do much more of that, though, as your main() is too long to follow at a glance.

The getRandomWord() function has a serious bug arising from your misuse of Random.nextInt(int n).  Possible outputs from that call are 0, 1, 2, 3, 4.  Therefore, it will never choose "awesome", and it may throw an IllegalStateException.  I don't recommend using a switch statement for that purpose, though, because the vocabulary list should be treated as data, not as code.  It would be better to pick a random element from an array of strings.

You also have a logic bug that lets the user "win" by guessing the same correct letter again and again.  For example, if the word is "fizz", and the user guesses 'z', then you record the fact that there are two blanks remaining.  If the user guesses 'z' again, you report a win.  (You really ought to handle duplicate guesses better, but at the least you should prevent such erroneous wins.)

A block of variable declarations at the top of your main() function is unfriendly — nobody likes reading those.  Instead, declare your variables as close to the point of use as possible.  In fact, within your block of declarations are two unused variables wordLength and solved that you yourself lost track of.

Pick variable names that make sense — not just to you but to any programmer who might come along.  Instead of k (short for "keyboard"?), pick input (and initialize it immediately).  Instead of temp (which isn't temporary, so it must mean "template"?), try wordDisplay.

Suggested implementation

This is mostly just a result of

refactoring your main() code into functions
renaming variables for clarity
writing some loops a bit more eloquently

import java.util.*;

public class HangMan {
    private static final String[] VOCABULARY = new String[] {
        "peace", "nuts", "dankmemes", "fizz", "awesome"
    };

    private static Scanner input = new Scanner(System.in);

    private static void intro() {
        System.out.println("Hey, what's your name?");
        String name = input.nextLine();
        System.out.println(name+ ", hey! This is a hangman game! (Some answers have two words, usually the longer ones)\n");
    }

    /**
     * Picks a word at random from the vocabulary.
     */
    public static String getRandomWord() {
        return VOCABULARY[new Random().nextInt(VOCABULARY.length)];
    }

    /**
     * Makes an array of placeholder characters the same length as the word.
     */
    private static char[] initWordDisplay(String word) {
        char[] wordDisplay = new char[word.length()];
        Arrays.fill(wordDisplay, '*');
        return wordDisplay;
    }

    /**
     * Prompts the user to enter a single-character guess (repeatedly
     * if necessary in case of errors).
     */
    private static char promptGuess() {
        do {
            System.out.print("Enter letter: ");
            String s = input.next();
            if (s.length() == 1)  {
                return s.charAt(0);
            }
            System.out.println("Please enter 1 character");
        } while (true);
    }

    /**
     * Fills wordDisplay with the guess, returning the
     * number of blanks correctly filled as a result.
     */
    private static int fillGuess(String word, char[] wordDisplay, char guess) {
        int foundCount = 0;
        for (int pos = 0; (pos = word.indexOf(guess, pos)) != -1; pos++) {
            if (wordDisplay[pos] != guess) {   // Prevent repeat guess
                wordDisplay[pos] = guess;
                foundCount++;
            }
        }
        return foundCount;
    }

    public static void main(String[] args) {
        intro();

        String word = getRandomWord();
        char[] wordDisplay = initWordDisplay(word);
        int blanksLeft = wordDisplay.length;

        System.out.print("\nWord to date: " + blanksLeft + " letters.");

        for (int attemptsLeft = 10; blanksLeft > 0 && attemptsLeft > 0; attemptsLeft--) {
            System.out.println("\nAttempts left: " + attemptsLeft);
            char guess = promptGuess();
            int foundCount = fillGuess(word, wordDisplay, guess);
            blanksLeft -= foundCount;

            if (foundCount == 0) {
                System.out.println("Sorry, didn't find any matches for " + guess);
            } else {
                System.out.println("Found " + foundCount + " matches for " + guess);
            }
            System.out.println(new String(wordDisplay));
        }

        if (blanksLeft == 0) {
            System.out.println("\n---------------------------");
            System.out.println("You did iiiiiiit!");
        } else {
            System.out.println("\n---------------------------");
            System.out.println("Better luck next time, you didn't get the word!");
            System.out.println("It was \"" + word + "\"");
        }
    }
}


Red implementation of Rock, Scissors, Paper I have implemented the Rosetta Code Rock Paper Scissors game in Red, but I'm sure that there are better ways to do every thing that I did. I would appreciate any feedback on this code, which does work according to spec:

  Implement the classic children's game Rock-Paper-Scissors, as well as
  a simple predictive AI (artificial intelligence) player.

  Rock Paper Scissors is a two player game.

  Each player chooses one of rock, paper or scissors, without knowing
  the other player's choice.

  The winner is decided by a set of rules:

  Rock beats scissors
  Scissors beat paper
  Paper beats rock

  If both players choose the same thing, there is no winner for that
  round.

  For this task, the computer will be one of the players.

  The operator will select Rock, Paper or Scissors and the computer will
  keep a record of the choice frequency, and use that information to
  make a weighted random choice in an attempt to defeat its opponent.

Red [
    Problem: %http://www.rosettacode.org/wiki/Rock-paper-scissors
    Code: %https://github.com/metaperl/red-rosetta/blob/master/rock-paper-scissors.red
]

help1: %https://stackoverflow.com/questions/54272942/how-to-find-the-first-element-of-a-block-of-strings-whose-first-character-matche
help2: %https://stackoverflow.com/questions/54272956/how-to-increment-element-of-block-after-found-element
help3: %https://stackoverflow.com/questions/54273057/two-dimensional-dispatch-table-with-templated-response
help4: %https://stackoverflow.com/questions/54273161/in-red-how-do-i-search-through-a-block-for-a-string-matching-a-pattern/54275072#54275072

games-played: 0

weapons: ["rock" "scissors" "paper"]
matching-weapon: func [abbrev][
    foreach weapon weapons [
        if (first weapon) = first abbrev [
            return weapon
        ]
    ]
]
player-choices: ["rock" 0 "scissors" 0 "paper" 0 ]
player-choice-tally: func [choice][player-choices/(choice): player-choices/(choice) + 1]

player-choice: "x"
valid-choice: func [c][find "rpsq" c]

player-wins: [
    ["rock" "scissors"] "breaks"
    ["paper" "rock"] "covers"
    ["scissors" "paper"] "cut"
]

player-wins?: function [player1 player2] [
    game: reduce [player1 player2]
    winning: player-wins/(game)
]

report-win: func [player1 player2][rejoin [player1 " " (player-wins? player1 player2) " " player2]]

draw: func [player computer][player = computer]

update-stats: func [player-choice][
    player-choice-tally player-choice
    games-played: games-played + 1
]

make-computer-choice: func [][
    either games-played >= 3 [
        tmp: random games-played
        tally: select "rock" player-choices
        either tmp <= tally [return "rock"][
            tally: tally + select "scissors" player-choices
            either tmp <= tally [return "scissors"][
                return "paper"
            ]
        ]
    ][random/only weapons]
]

while [not player-choice = "q"][
    player-choice: ask "(r)ock, (s)cissors, (p)aper or (q)uit? "
    either (player-choice = "q") [][
        if (valid-choice player-choice) [
            computer-choice: random/only weapons
            player-choice: matching-weapon player-choice
            update-stats player-choice
            print rejoin ["Player choice: " player-choice "tally" player-choices "Computer choice:" computer-choice]
            either draw player-choice computer-choice [print "Draw"][
                tmp: player-wins? player-choice computer-choice
                print either tmp [rejoin ["Player wins: " report-win player-choice computer-choice]]
                    [rejoin ["Computer wins: " report-win computer-choice player-choice]]
            ]
        ]
    ]

]

Looks an interesting first project! Some thoughts:

The URL! type does not need a % prefix (they would be interpreted as FILE! and would behave incorrectly were you to operate upon them).
In Red, you can use the WORD! type in place of strings. You can use a LIT-WORD! to get a word without evaluating it. Takes a little getting used to, but is more efficient and expressive:

weapons: [rock paper scissors]
player-choices: [rock 0 scissors 0 paper 0]

player-wins: [
    [rock scissors] "breaks"
    [paper rock] "covers"
    [scissors paper] "cut"
]

select player-choices 'rock

Instead of VALID-CHOICE, you could use SWITCH to drive things:

until [
    switch player-choice: ask "(r)ock, (s)cissors, (p)aper or (q)uit? " [
        "q" [true]
        "r" "s" "p" [
            play-round player-choice
            false
        ]
    ]
]

You can use CASE in place of nested EITHER statements—breaks the choices out a bit:

make-computer-choice: func [
    /local tmp tally
][
    case [
        games-played < 3 [
            random/only weapons
        ]

        (
            tmp: random games-played
            tmp <= tally: select player-choices 'rock
        ) [
            'rock
        ]

        tmp <= tally: tally + select player-choices 'scissors [
            'scissors
        ]

        true [ ; or else
            'paper
        ]
    ]
]

If you want to somewhat keep the game logic separate from the interface, I'd recommend a play-round function called in the same fashion as above. play-round could return false (continues) or true (ends loop when someone wins a certain amount of games).
I notice you define the function make-computer-choice but don't invoke it.


A recursive program that attempts to solve an extremely large number This is my recursive program for the following function:

\$F(x) = G(x) - W(x)\$
\$G(x) = [G(x-1)+G(x-2)]^2\$
\$W(x) = [W(x-1)]^2 + [W(x-2)]^2\$
If x == 0 in either function G or function W, then return 0
If x == 1 in either function G or function W, then return 1

import java.math.BigInteger;
import java.io.PrintWriter;
import java.io.IOException;

public class RecursiveProgram
{
    BigInteger test = BigInteger.valueOf(30);
    BigInteger zero = new BigInteger(Integer.toString(0));
    BigInteger one = new BigInteger(Integer.toString(1));
    BigInteger two = new BigInteger(Integer.toString(2));
    public BigInteger G(BigInteger x)
    {
        if(x.equals(zero))
            return zero;
        if(x.equals(one))
            return one;
        return (G(x.subtract(one)).add(G(x.subtract(two)))).multiply((G(x.subtract(one)).add(G(x.subtract(two)))));
    }

    public BigInteger W(BigInteger x)
    {
        if(x.equals(zero))
            return zero;
        if(x.equals(one))
            return one;
        return ((W(x.subtract(one))).multiply(W(x.subtract(one)))).add((W(x.subtract(two))).multiply(W(x.subtract(two))));
    }

    public void solveForF() throws IOException
    {
        PrintWriter writer = new PrintWriter("C:/Users/my computer/Desktop/f(30).txt");
        writer.println(G(test).subtract(W(test)));
        writer.close();
    }
}

It writes the calculated number to the .txt file on my desktop. I want to improve the recursive program to allow it to calculate F(30). However, I did some math calculations as to how long it would take for the program to complete using the amount of time it took to calculateF(20) and F(21), approximating to 12.35 days. Obviously this is inefficient and I haven't learned a better method to make it more efficient. I've heard of dynamic programming but I'm not sure as how I would use it here.

Your function is extremely slow because it does a lot of redundant calculations.

$$
\begin{align}
G(5) =& (G(4) + G(3))^{2} \\
     =& (G(4) + G(3)) \cdot (G(4) + G(3)) \\
     =& ((G(3) + G(2))^{2} + (G(2) + G(1))^{2})) \cdot ((G(3) + G(2))^{2} + (G(2) + G(1))^{2})) \\
     =& ((G(3) + G(2)) \cdot (G(3) + G(2)) + (G(2) + G(1)) \cdot (G(2) + G(1)))) \cdot ((G(3) + G(2)) \cdot (G(3) + G(2)) + (G(2) + G(1)) \cdot (G(2) + G(1))))) \\
     =& \ldots
\end{align}
$$

The complexity is \$O(4^x)\$: to calculate \$G(x)\$, you're breaking it into four problems of approximately the same size.

To start with, it would be smarter to square a number without having to compute the base again.

$$
\begin{align}
G(5) =& (G(4) + G(3))^{2} \\
     =& ((G(3) + G(2))^{2} + (G(2) + G(1))^{2}))^{2} \\
     =& \ldots
\end{align}
$$

Then you would be down to "just" \$O(2^x)\$, which is still horrible.

The trick is to notice that you're going to be computing \$G(2)\$, \$G(3)\$, etc. many many times.  What you want to do is memoization — caching previously computed results.  With memoization, you should be able to compute \$G(x)\$ in \$O(x)\$ time.

Once you solve the efficiency issue above, you will still run into a problem with stack overflow for large \$x\$.  To avoid stack overflow, you'll want to reimplement the functions using an iterative loop rather than recursion.


Make palindrome finder-builder function faster I built a small JavaScript function to see if a series of characters are a palindrome or not. The functions works well and does everything it has to do, but I'm wondering if something can be tweaked to make it faster.

This is the actual code:

var words = ['anita lava la tina',' a ', 'civvic', 'ddaa', 'aa', 'dca', '332', null,     'toyota','racecar'];
function getPalindrome(word){
    if(!word || (word = word.replace(/ /g,'')).length<2)
        return "-1";

    var charsCount = {};
    word = word.toLowerCase();
    var chars = word.split("");

    //ahorra un poco de tiempo cuando las palabras son grandes
    if(chars.reverse().join('')===word)
        return word;

    for(var i=0;i<chars.length;i++){
        if(charsCount[chars[i]])
            charsCount[chars[i]] += 1;
        else
            charsCount[chars[i]] = 1;
    }

    //si hay mas de un impar, ya no puede ser palindromo
    var oddCounter = 0;
    var palindromo = [];
    var oddChar = '';
    for(var letter in charsCount){
        if(charsCount[letter] % 2 !== 0)
            oddCounter++;

        if(oddCounter>1)
            return "-1";

        var times = charsCount[letter];
        if(times===1){
            oddChar = letter;
        }
        else{
            for(var i=0;i<times;i++){
                if(i%2==0)
                    palindromo.push(letter);
                else
                    palindromo.unshift(letter);
            }
        }
    }
    //agrego el impar a la mitad, si es que hay
    palindromo.splice(palindromo.length/2,0,oddChar);

    return palindromo.join('');
}

for(var i in words){
    console.log(words[i],":",getPalindrome(words[i]));
}

What I do here is this:

If word is null or the length of word after eliminating spaces is lower than 2, then it's not a palindrome, so return -1.
If the reverse of word equals word, then it is a palindrome, so return the word.
Make a map with all the chars, adding the occurrence of each char.
Loop the map of the chars: if the count of more than one char is an odd number, then it's not a palindrome, so return -1.
Simply put one char at the beginning of an array and one at the end and loop n times if the char was found.
Store the odd char in a variable for later use.
When loop (4) ends, insert the odd char in the middle of the array.
Return the built palindrome.

The function works, but how can I make it faster?

Make a local instance of word and work with that variable through all your function code, every reference to word it's been fetched through the scope chain.

During code execution, identifiers such as variable and function names are resolved by searching the scope chain of the execution context. Identifier resolution begins at the front of the scope chain and proceeds toward the back. Consider the following code:

function add(num1, num2){
    return num1 + num2;
}

var result = add(5, 10);

When this code is executed, the add function has a [[Scope]] property that contains only the global variable object. As execution flows into the add function, a new execution context is created, and an activation object containing this, arguments, num1, and num2 is placed into the scope chain.

This resolution is performed by inspecting each object in the scope chain until the specific identifier is found. The search begins at the first object in the scope chain, which is the activation object containing the local variables for the function. If the identifier isn’t found there, the next object in the scope chain is inspected for the identifier. When the identifier is found, the search stops. In the case of this example, the identifiers num1 and num2 exist in the local activation object and so the search never goes on to the global object.

Understanding scopes and scope chain management in JavaScript is important because identifier resolution performance is directly related to the number of objects to search in the scope chain. The farther up the scope chain an identifier exists, the longer the search goes on and the longer it takes to access that variable; if scopes aren’t managed properly, they can negatively affect the execution time of your script.

Use Local Variables

Local variables are, by far, the fastest identifiers both to read from and write to in JavaScript. Because they exist in the activation object of the executing function, identifier resolution involves inspecting a single object in the scope chain. The amount of time necessary to read the value of a variable increases with each step along the scope chain, so the greater the identifier depth, the slower the access is going to be.


Convert and print Celsius to Fahrenheit from certain range of values I just started learning C, and got to the "for loops"—where an exercise is given to convert Celsius to Fahrenheit ( from -50 to 1000 with a step of 20 ), and print the values. I decided to play with it by adding different steps when certain values are reached.

I do have some programming experience ( Python/Java/JavaScript ) but it's all in beginner level scope.My concern is that i could have used one loop and check each (bp) with a condition... or maybe use module operator to determine a step change ... or something third.
I need an expert opinion on the code i wrote.

Is this a good approach for this particular problem?

Is this considered bad or good practice?If bad what would you change and why?

Would you (assuming as an expert) go with this or other way ? If other, why ?

#include <stdio.h>

int main(){

    float fahr, cels, step, min, max;

    float count[4] = {10,1,10,100};
    float bp[4] = {-50,-10, 10, 100};

    size_t bp_size, st_size;
    int c,c2;

    bp_size = *(&bp + 1) - bp;
    st_size = *(&count + 1) - count;

    min = -50; max = 1000;
    step = count[0];
    c2 = 0;

    for (cels = min; cels <= max; cels += step){

        fahr = cels * 1.8 + 32;
        printf("%4.0f C\t= %6.1f F\n", cels, fahr);

        for(c = 0; c < bp_size; c++){
            if (cels == bp[c]){
                if (c2 == st_size){ c2 = 0;}
                step = count[c2];
                c2++;
            }
        }
    }
}

The program works correctly, but there are a number of things you might be able to improve here.  Here are some observations in no particular order.

Reduce the number of variables

There are a lot of variables here, which makes it a bit difficult to read and understand the program.  Some of the suggestions below will show some ways to reduce that number and add more clarity to the program.

Use const where practical

A number of places in the code should have the const keyword added.  For example instead of this:

float count[4] = {10,1,10,100};
float bp[4] = {-50,-10, 10, 100};

write this:

const float count[4] = {10,1,10,100};
const float bp[4] = {-50,-10, 10, 100};

This indicates, both to the compiler and to human readers of the code, that these values will not be altered.

Use more meaningful variable names

The variable names step, min and max are useful and descriptive of what those variables actually do.  However, count and bp are not very good names.  I'd suggest calling them something more closely related to their function as with the following suggestion.

Consider using a struct

The count and bp values are closely related, but this is not clearly indicated in the code.  It would be easier to understand, I think, if a structure were used instead:

struct {
    float startTemp;
    float stepSize;
} const limits[] = {
    { -50,  10 },
    { -10,   1 },
    {  10,  10 },
    { 100, 100 },
    {1000,   0 },
};

Note that the minimum and maximum values are built into the table so there isn't any need for additional variables for those uses.

Declare variables in the smallest practical scope

By declaring variables in the smallest practical scope, you reduce the chance for name collisions and make it clear to the reader of your code where variables are and are not needed.  With any C compiler conforming to the 1999 specification (which should be all of them at this point!) one could rewrite the outer for loop like this:

for (float cels = min; cels <= max; cels += step) {
    float fahr = cels * 1.8 + 32;
    /* etc.*/
}

Initialize variables when they are declared

If you follow the advice above about reducing the scope of variables, you can also easily initialize variables when they are declared.  This helps to make sure that variables aren't used with uninitialized values.

Declare each variable on its own line

Declaring variables like this:

float fahr, cels, step, min, max;

is not good current practice.  Instead, declare one variable per line and initialize it.

Use the sizeof operator

These two lines are quite peculiar:

bp_size = *(&bp + 1) - bp;
st_size = *(&count + 1) - count;

A better way to find the size of such arrays would be this:

size_t bp_size = sizeof(bp);

Don't test floating point numbers for equality

One of the lines in the current code is this:

if (cels == bp[c]){

It will probably work just fine in this context, but generally, one should avoid testing floating point numbers for equality.  It's usually a better bet to test instead for < or <=.  See this question for more details.

Put if and for code on separate line(s)

The code contains this line:

if (c2 == st_size){ c2 = 0;}

It's not technically wrong and the compiler will accept it, but it's too easy for a human reader to overlook the contents of the if when it's hiding at the end of the same line.  Prefer instead to write it like this:

if (c2 == st_size) {
    c2 = 0;
}

Think carefully about loops

In this code, the outer loop seems clear enough, but the inner loop is not.  The point of it is to determine what step size to use but the code to do this is more complex than it needs to be:

for(c = 0; c < bp_size; c++){
    if (cels == bp[c]){
        if (c2 == st_size){ c2 = 0;}
        step = count[c2];
        c2++;
    }
}

First, we could just keep track of the step size as we go instead of searching for it each time.  Second, once the step size is found, there's no reason to continue through the rest of the loop.

Here's an alternative implementation that uses all of these ideas:

int main(){
    struct {
        float startTemp;
        float stepSize;
    } const limits[] = {
        { -50,  10 },
        { -10,   1 },
        {  10,  10 },
        { 100, 100 },
        {1100,   0 },
    };

    for (int i=0; limits[i].stepSize != 0; ++i) {
        for (float cels = limits[i].startTemp;
            cels < limits[i+1].startTemp;
            cels += limits[i].stepSize)
        {
            float fahr = cels * 1.8 + 32;
            printf("%4.0f C\t= %6.1f F\n", cels, fahr);
        }

    }
}

Note that the last item in the list is 1100 degrees rather than 1000.  This is done because we want to include the value of 1000 in the list.


Converting ASCII to an image So after I finished my fractal generator, I decided that text is ugly, at least compared to pictures, so I made a text-to-picture converter:

require 'chunky_png'
require 'etc'
require 'pathname'

class Array
  def chunk(chunk_size)
    self.each_with_index.with_object(Array.new(size / chunk_size) { [] }) do |(elt, ind), result|
      result[ind / chunk_size] << elt
    end
  end
end

raise ArgumentError, 'Wrong argument number. Run with -h to see usage.' unless ARGV.length % 2 == 1

filename = ARGV.pop

colors = Hash[
    ARGV.chunk(2).map do |(char, color)|
      color = ChunkyPNG::Color.from_hex(color)
      [char, color]
    end
]

text = []
text << $_.chomp.chars until $stdin.gets.nil?
row_length = text[0].length
raise ArgumentError, 'Input cannot be jagged' unless text.all? { |row| row.length == row_length }

image = ChunkyPNG::Image.new(row_length, text.length, ChunkyPNG::Color::WHITE)
text.each.with_index do |row, y|
  row.each_with_index do |char, x|
    raise ArgumentError, "No valid color provided for `#{char}`" if colors[char].nil?
    image[x, y] = colors[char]
  end
end

image.save(filename)

puts "Image saved to #{File.join(Dir.getwd, filename)}"

Note that it uses the ChunkyPNG gem, which I wholeheartedly recommend for any of your pixel-by-pixel image generation needs.

Since this script takes the input data via standard input, it's meant to be used in a command like this:

ruby build_fractal.rb <fractal options> | ruby to_image.rb "X" 000 " " FFF fractal.png

which makes pretties like these.

I'm looking for advice specifically on:

Efficiency: I want to spend as little time as possible drawing the image, so I can spend more time on generating the fractal.

That's it. Since this is purely a utility script that, as far as I can tell, is complete, there's no reason to worry about maintainability or code prettiness. I'm willing to sacrifice everything else to get every last bit of speed out of this.

class Array
  def chunk(chunk_size)
    self.each_with_index.with_object(Array.new(size / chunk_size) { [] }) do |(elt, ind), result|
      result[ind / chunk_size] << elt
    end
  end
end

You reinvented Enumerable#each_slice! It returns an enumerator, which is fine for your code. Simply write ARGV.each_slice(2).map do |char, color| instead of ARGV.chunk(2).map.

ARGV.length % 2 == 1

Change to ARGV.length.odd?.

'Run with -h to see usage.'

Doesn't work.

colors = Hash[
    ARGV.each_slice(2).map do |char, color|
      color = ChunkyPNG::Color.from_hex(color)
      [char, color]
    end
]

I'd use to_h instead of Hash[] and inline color, to make the code shorter:

colors = ARGV.each_slice(2).map do |char, color|
  [char, ChunkyPNG::Color.from_hex(color)]
end.to_h

text << $_.chomp.chars until $stdin.gets.nil?

Instead of creating many arrays of characters, simply store the strings. Remove the call to .chars and iterate over a string using row.each_char.with_index. This makes the code a little faster.

raise ArgumentError, "No valid color provided for `#{char}`" if colors[char].nil?
image[x, y] = colors[char]

ChuckyPNG already throws an ArgumentError if the color is nil. You can remove your check for this, which may improve performance. (If you need to catch this error, you can wrap the loop with a begin-rescue block)


Is this PHP code snippet safe? Mostly asking for critiques of vulnerability. Am I using any functions or methods that are unsafe?

<?php
$menu = array( "page1","page2","page3" );
$defpage = "page1";

$section = $defpage;
if ( isset( $_GET['section'] ) ) $section = $_GET['section'];
if ( !in_array( $section, $menu ) ) $section = $defpage;
?>

This is code that checks if the section is in the array and then sets it as such, but hardwires it back to default if it's not valid.

Is it safe? Yes, it will currently do the right thing.

One of the features that plays in to best practice though, is how future proof it is. Over time, code gets edited, changed, etc. What you want is to make the code 'fail safe' in the future too. What if someone comments out the second line, you end up with a problem.

A better way to write your code would be to set the default, and only change it if the input is valid:

<?php
$menu = array( "page1","page2","page3" );

$section = "page1";
$input = $_GET['section'];
if ( isset( $input ) && in_array( $input, $menu ) ) {
    $section = $input;
}

?>


Program to accept numbers, then calculate the mean, median, and range I created a basic program that allows a user to continuously input values until he or she is satisfied; furthering, the program outputs the mean, median, and range. Then, the code gives you the option to run it again or exit.

#Mean, median, and range
from clear import *
def main():
    clear()
    list_one = []
    while True:
        q = input('Type a number or type stop at any time: ').lower()
        if q.isnumeric():
            list_one.append(int(q))
            print('''
Great choice! Choose again or type stop the sequence.''')
        elif q == 'stop':
            list_one.sort(key=int)
            print('''
And the values are...''')

            def mean():
                return 'Mean: ' + str(sum(list_one)/len(list_one))

            def median():
                length = len(list_one)
                if length % 2 == 0:
                    return  'Median: ' + str(sum(list_one[length//2-1:length//2+1])/2)
                else:
                    return 'Median: ' + str(list_one[length//2])

            def range_():
                return 'Range: ' + str(list_one[-1] - list_one[0])

            print(mean())
            print(median())
            print(range_())
            break

        else:
            print('''
That\'s not a number or stop! Try again brudda man.
''')

def end():
    while True:
        q = input('Type anything and and press enter to repeat or press only \'Enter\' to exit. ')
        if len(q) >= 1:
            main()
            break
        else:
            exit()
main()

while True:
    q = input('''
Type anything and and press enter to repeat or press only \'Enter\' to exit. ''')
    if len(q) >= 1:
        main()
    else:
        clear()
        exit()

Additionally, the "clear" library that I imported is a custom library of my making which only prints out a ton of blank lines so the script "clears" and outputs are then at the bottom. If there is a better way of doing this please let me know.

I want to get familiar with what is "right" or "wrong" in terms of programming with Python. I would like my work to look professional and practice more respected coding ethics, so please be as harsh as you wish.

On top of Thomas Ward answer, here is a short and important comment:

More beautiful functions

Your functions are performing some mathematical computations and handling the formatting to show the result to the 2 users. These correspond to 2 concerns that could be separated.

Also, your functions rely on the list_one global variable. It would be clearer to have this provided to the functions using an argument.

You'd have something like:

def mean(lst):
    return sum(lst) / len(lst)

def range_(lst):
    return lst[-1] - lst[0]

def median(lst):
    length = len(lst)
    if length % 2 == 0:
        return sum(lst[length//2-1:length//2+1]) / 2
    else:
        return lst[length//2]

...
            print('Mean: ' + str(mean(list_one)))
            print('Median: ' + str(median(list_one)))
            print('Range: ' + str(range_(list_one)))

These are easier to understand and to reason about. They are also easier to test. This is an interesting exercise that you can try to practice.

Small improvements in median

The expression length//2 is used in multiple places which makes the code tedious to read and less efficient than it could be.
Using a temporary variable makes things clearer:

def median(lst):
    length = len(lst)
    mid = length//2
    if length % 2 == 0:
        return sum(lst[mid-1:mid+1]) / 2
    else:
        return lst[mid]

From here, I think it's pretty good already. Let's see what can be still be done for the sake of learning new techniques.

When you want to compute both the quotient and the remainder of the division, you can use "/" and "%" just like you did. Another option is to use the divmod builtin which returns both directly:

def median(lst):
    length = len(lst)
    mid, rem = divmod(length, 2)
    if rem == 0:
        return sum(lst[mid-1:mid+1]) / 2
    else:
        return lst[mid]

The operation sum(lst[mid-1:mid+1]) / 2 looks a lot like things we've just somewhere else: we compute the sum of a list and we divide by the number of elements which is happens to be 2.

We could reuse our mean function here.

def median(lst):
    length = len(lst)
    mid, rem = divmod(length, 2)
    if rem == 0:
        return mean(lst[mid-1:mid+1])
    else:
        return lst[mid]

To go further, we could use the ternary operator to have a single return value.
We could also use the fact that in a boolean context, non-zero integers are equivalent to True and zero is equivalent to false.
This would lead to the following code which is, from my point of view, not really better:

def median(lst):
    length = len(lst)
    mid, rem = divmod(length, 2)
    return lst[mid] if rem else mean(lst[mid-1:mid+1])


Reversing only vowels in a string Given a string, reverse only the vowels present in it and print the resulting string.
example
input: ransomware
output: rensamwora

Here is my solution:

import java.util.Scanner;

public class ReversingVowels {
    public static void main(String[] args) {
        Scanner scanner = new Scanner(System.in);
        int numberOfTestCases = scanner.nextInt();

        while (numberOfTestCases --> 0) {
            StringBuilder stringWhoseVowelsShouldBeReversed = new StringBuilder(scanner.next());
            System.out.println(reverseVowels(stringWhoseVowelsShouldBeReversed));
        }

        scanner.close();
    }

    private static String reverseVowels(StringBuilder string) {

        if (string.length() == 0 || string.length() == 1) return string.toString();

        int start = 0;
        int end = string.length() - 1;

        while (true) {
            while (! isVowel(string.charAt(start))) {
                start++;
            }
            while (! isVowel(string.charAt(end))) {
                end--;
            }
            if (start >= end) {
                break;
            }
            else {
                swapVowels(string, start, end);
                start++;
                end--;
            }
        }
        return string.toString();
    }

    private static boolean isVowel(char character) {
        switch(character) {
        case 'a':
        case 'e':
        case 'i':
        case 'o':
        case 'u': return true;
        default: return false;
        }
    }

    private static void swapVowels(StringBuilder string, int index1, int index2) {
        char tmp = string.charAt(index1);
        string.setCharAt(index1, string.charAt(index2));
        string.setCharAt(index2, tmp);
    }
}

Questions:

Are there any bad practises
Are there any issues
Can this be improved

1) No more than one instruction per line.

if (string.length() == 0 || string.length() == 1) return string.toString();

i.e.:

if (string.length() == 0 || string.length() == 1) {
    return string.toString();
}
// or, better:
// if (string.length() < 2) {
//     return string.toString();
// }

2) Your main while feels strange, I don't like this creative use of postdecrement and greater then operator

while (numberOfTestCases --> 0)  // BAD! See https://stackoverflow.com/questions/1642028/what-is-the-operator-in-c

In this case a for loop is better because you know the number of iterations.

3) while (true) is bad and force the developer to read all the code to understand how to break the loop

4) No need to pass a StringBuilder to reverseVowels: the method itself should create it.

5) Use temporary variables to improve readability:

...
for (int i = 0; i < numberOfTestCases; i++) {
    String input = scanner.nextLine();

    String vowelsReversedString = reverseVowels(input);

    System.out.println("OUTPUT: " + vowelsReversedString);
}
...

...
private static String reverseVowels(String string) {
    if (string == null || string.length() < 2) {
        return string;
    }

    StringBuilder sb = new StringBuilder(string);

    int start = 0;
    int end = sb.length() - 1;

    while (start < end) {
        ...
    }

    return sb.toString();
}
...


Suppress API call in Redux Saga based on a timer I have an API that provides data that is updated on an infrequent schedule, about once every 2½ minutes on average. Assuming that it's okay to fetch only as often as every 2½ minutes or less often, I'd like to construct a saga (I use redux-saga) in a way that it does nothing if it's not yet time to do an API call. The code I came up with is quite simplistic (API error handling omitted):

import { select, put, call } from "redux-saga/effects";

const FETCH_INTERVAL = 1000 * 60 * 60 * 2.5;

function* fetchApiSaga(apiUrl) {
  // get data from the store
  const data = select(state => state.theDataWeNeed);

  // signal to the user that we're about to fetch data
  put(WAITING_FOR_DATA);

  // if fetched 2.5 ago or earlier, fetch again
  if (!data || new Date() - !data.lastFetchedAt > FETCH_INTERVAL) {
    const response = yield call(apiUrl);

    // signal to the user that now we have new data from the API
    put(REWRITE_DATA, response);
  } else {
    // signal to the user that we just reuse old data
    put(JUST_USE_CACHED_DATA);
  }
}

Now, I have two questions:

Is this an idiomatic way to suppress consecutive actions in Redux Saga based on time?
Is this idiomatic Redux Saga if the generator function selects from the store and therefore is not pure?

Disclaimer: I´m not an expert in redux / redux-saga. This post is not directly answering your questions. Rather, it is focused on code design / architecture and what is the most useful approach for the underlying problem.

For me, the main question is: Who is in charge of determining when the data is invalid?

Imho, there are at least three answers to this:

The API / backend / server
The frontend application
One particular UI component

If it is 1., then I would use HTTP cache headers like Cache-Control: max-age=150. This way, the backend can change the value without requiring to adapt the frontend. Also, it removes the need for any conditional code. The saga can always call the API and the browser does the caching.

If it is 2., then the saga should contain the conditional code to determine whether to fetch new data or to used cached values. Where to put the actual cache and how to do this is a question on its own with regards to frontend application architecture.

If it is 3., then the particular UI component should contain the conditional code and it should also contain / be in charge of the cached values.


Comparing client lists with Cross Joins I've written a query to compare the clients in our database with the people in a list that I've received. It needs to check if anyone from the list is one of our clients. I've created a temporary table which has been filled with the names and the query below makes a cross-join with the two tables. That query works very slowly (two very large tables), so I was wondering if there's any way to speed this up.
Would it be faster to just join everything in the database and compare strings in code (java or something similar)?

(The second list was just names, so I can't use any indexed columns in our database.)

select C.NUMCLI, C.NAAM, T.FULLNAME, (UTL_MATCH.EDIT_DISTANCE_SIMILARITY(NAAM, FULLNAME))
as DIFF from (select LASTNAME ||' '|| FIRSTNAME ||' '|| MIDDLENAME  as FULLNAME from
TMP_CONTROL) T, (select NUMCLI, NOMCLI ||' '|| PRNCLI as NAAM from CLIENT
where CODLAN = 3 and STAANN <> 'D') C
where (UTL_MATCH.EDIT_DISTANCE_SIMILARITY(NAAM, FULLNAME)) >= 60

NOTE: A large portion of people on the list may have their name written in a slightly different manner, which is why I'm using the EDIT_DISTANCE SIMILARITY >= 60. The goal is just to filter out the large differences, so I can easily compare the smaller ones.

Update

This is the explain-plan for the query:

SQL> SET LINESIZE 130
SQL> SET PAGESIZE 0
SQL> SELECT *
  2  FROM   TABLE(DBMS_XPLAN.DISPLAY);
Plan hash value: 1462516232

---------------------------------------------------------------------------------
| Id  | Operation          | Name       | Rows  | Bytes | Cost (%CPU)| Time     |
---------------------------------------------------------------------------------
|   0 | SELECT STATEMENT   |            |    74M|  3041M|  2197K  (3)| 07:19:29 |
|   1 |  NESTED LOOPS      |            |    74M|  3041M|  2197K  (3)| 07:19:29 |
|*  2 |   TABLE ACCESS FULL| CLIENT     | 71843 |  2174K|  2386   (2)| 00:00:29 |
|*  3 |   TABLE ACCESS FULL| TMP_CONTROL|  1033 | 12396 |    31   (4)| 00:00:01 |
---------------------------------------------------------------------------------

Predicate Information (identified by operation id):

2 - filter("STAANN"<>'D' AND TO_NUMBER("CODLAN")=3)
3 - filter("UTL_MATCH"."EDIT_DISTANCE_SIMILARITY"("NOMCLI"||'
          '||"PRNCLI","LASTNAME"||' '||"FIRSTNAME"||' '||"MIDDLENAME")>=60)

17 rows selected.

Fundamentally, at some point, you have to do a cross-join to calculate your results. Performance will be a problem... but there are things that can be done.

First though, why the ugly SQL? Formatting SQL to make it readable is not hard to do:

select C.NUMCLI,
       C.NAAM,
       T.FULLNAME,
       (UTL_MATCH.EDIT_DISTANCE_SIMILARITY(NAAM, FULLNAME)) as DIFF
from (
        select LASTNAME ||' '|| FIRSTNAME ||' '|| MIDDLENAME  as FULLNAME
        from TMP_CONTROL
     ) T,
     (
        select NUMCLI,
               NOMCLI ||' '|| PRNCLI as NAAM
        from CLIENT
        where CODLAN = 3
          and STAANN <> 'D'
     ) C
where (UTL_MATCH.EDIT_DISTANCE_SIMILARITY(NAAM, FULLNAME)) >= 60

We can see a few things in here.....

Your source data contains a middle-name but your main table does not. Are you sure you want to be joining them this way? Will it create the correct results?

Now, about those performance improving options....

Let's assume that the best way to solve this problem is to do a cross-product of the two tables. Compare each value in table a with all values in table b. Oracle ha a few options for doing this. I will list them in what I consider to be a worst-to-best order:

true cross-product - It can do a true Cartesian product of the data - build an in-memory join of each value in the temp table joined to each value in the primary table, and then scan the results once, calculating the edit distance, and then discarding those results which are < 60. It can either calculate the string concatenation of the names before or after the Cartesian product, either option will be (very) slow.
nested-loop T->C - It can loop through each temp table value, and for each of them, it can do the name-concatenation, and then it will need to join to the primary table (with the conditions), do the name-concatenation, and discard bad edit-distances.
nested-loop C->T - It can loop through each value in the primary table, check the conditions, and for each successful value, it can do the String-concatenation, it can scan all values in the temp table, compute the name-concatenation, and then calculate the edit distance, and discard the bad results.
nested-loop C'->T' or T'->C' - It can produce a temporary, or in-memory version of the pre-filtered C table and name-concatenated T and C tables, and then do a nested-loop of these two in-memory tables, discarding bad edit distances.

Your SQL is written to suggest you want it to do the last option, create two in-memory data sets, each of them pre-computed to contain just the name-concatenated values, and then you only perform the cross-product on these tables.... but, just because you wrote the SQL that way does not mean that Oracle will do the process that way....

... have you done an explain-plan to figure out which option it has taken?

My guess is that it has chosen nested-loop T->C, with a second possibility of nested-loop C->T. The reasons?

the temp table probably does not have statistics up to date, and is not indexed at all
the primary table probably has some indexes on CODLAN and/or STAAN
when Oracle optimizes the query, it will likely decide the temp-space required to store all the name-concatenated values will require too much memory, or even physical IO

Now, we want/need Oracle to choose the most efficient cross-join mechanism... and we do not want it calculating the name-concatenation on the fly because it will need to repeat that many times for at least one side of the cross-product.

The solution is to force Oracle's plan to do what we want, and the way to do that is to pre-compute the data needed for the cross-product.

Your temp table should already have the pre-computed name-concatenated values... why are you having to do the name-concatenation as part of the query? Call this column FULLNAME

Then, create a second table as:

create table TMP_CLIENTNAME (
    NUMCLI INTEGER NOT NULL,
    NAAM NVARCHAR(255) NOT NULL)
)

insert into TMP_CLIENTNAME
select NUMCLI,
       NOMCLI ||' '|| PRNCLI as NAAM
from CLIENT
where CODLAN = 3
  and STAANN <> 'D'

Then your cross-product query becomes:

select C.NUMCLI,
       C.NAAM,
       T.FULLNAME,
       (UTL_MATCH.EDIT_DISTANCE_SIMILARITY(NAAM, FULLNAME)) as DIFF
from TMP_CONTROL T,
     TMP_CLIENTNAME C
where (UTL_MATCH.EDIT_DISTANCE_SIMILARITY(NAAM, FULLNAME)) >= 60

This query forces Oracle not to do any calculations other than the edit-distance in the join. This will make a difference....

The down-side is that you need additional storage for the data.

Once you have your data in this format you can consider some other options.... (which will likely affect the results of the edit-distance calculations)

index the first letter of each name, and only calculate the edit distances where the first letters are the same.
only calculate the edit distance when the length of the two names are less than say 5 characters different.
.....


Best practice and alternatives for string manipulation with an emphasis on readability As a personal habit I generally prefer to use string replace and format methods, along with string "patterns", in lieu of string concatenation and other methods in most languages I use (typically C# and Javascript).

For example, if I want to generate a URL with parameters (C#), I use:

string url = "page.aspx?id={id}&ref={ref}";
return url.Replace("{id}", id)
          .Replace("{referrer}", referrer);

// could also be expressed as
return "page.aspx?id={id}&ref={ref}".Replace("{id}", id)
                                    .Replace("{referrer}", referrer);

Instead of:

return "page.aspx?id=" + id + "&ref=" & referrer;

Or:

// StringBuilder is typically more efficient than string concatenation
StringBuiler sbUrl = new StringBuilder();
sbUrl.Append("page.aspx?");
sbUrl.AppendFormat("id={0}", id);
sbUrl.AppendFormat("ref={0}", referrer);
return sbUrl.ToString();

I do the same in Javascript:

function generateUrl(p1, p2) {
    return "page.aspx?1={0}&2={1}".replace("{0}", p1)
                                  .replace("{1}", p2);
}

The reason is I prefer the readability of the code, especially when large amounts of textual data must be processed, such as in the case of a form letter or e-mail:

// this would typically be stored in an XML or other configuration file as a "pattern"
string body = @"Dear {name}:

Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec aliquam blandit risus, quis imperdiet nibh congue quis.

Pellentesque ullamcorper malesuada ante, ac auctor felis feugiat id. Etiam id eros convallis nisi feugiat tincidunt. Phasellus fringilla erat eu tortor egestas volutpat. Vestibulum in odio lorem, quis fringilla sapien.

Sincerely,

{from-name}
";

return body.Replace("{name}", name).Replace("{from-name}", fName);

My concern is, however, that my methods are generally inefficient and that there may be better, more standardized ways of doing this.

Thoughts?

In C#, you could use the built-in string.Format instead:

string parameterizedUrl = "page.aspx?id={0}&ref={1}";
return string.Format(parameterizedUrl, id, referrer);

I haven't profiled it, but I think it would at worst be not any less efficient than your double Replace() call.


Merging with different modules We have an action merge for two projects. In the projects we have different modules like Feed, Files, Post etc....

modules Convert
class Merge
    attr_accessor :source_ids, :destination_id, :sources, :destination

    def initialize(source_ids, destination_id)
        @source_ids = source_ids.class == Array ? source_ids : [source_ids]
        @destination_id = destination_id
        @sources = Modelname.where("id in (?)", @source_ids).all
        @destination = Modelname.where("id in (?)", @destination_id).first
    end

    def merge
        sources.each do |source|
          ActiveRecord::Base.transaction do
            Convert::Feed.new(source, destination).merge
            Convert::File.new(source, destination).merge
            Convert::Post.new(source, destination).merge
            ## There are many more modules - code comes here
          end
        end
    end
end

class Feed
    def initialize(source, destination)
        @source = source
        @destination = destination
    end

    def merge
        #Feed merge code here
    end

end

class File
    def initialize(source, destination)
        @source = source
        @destination = destination
    end

    def merge
        #File merge code here
    end

end

class Post
    def initialize(source, destination)
        @source = source
        @destination = destination
    end

    def merge
        #Post merge code here
    end

end
end

a = Convert::Merge.new(source_id, destionation_id)
a.merge

Is this correct way to handle this action ? How can I handle this in a better way ?

This is nowhere near the full answer, but I just want to touch on the initialize method of Merge real quick since it was the first thing to jump out to me.

def initialize(destination_id, *source_ids)
  @source_ids = source_ids
  @destination_id = destination_id
  @sources = Modelname.where(id: @source_ids).all
  @destination = Modelname.where(id: @destination_id).first
end

This code is functionally equivalent. Using the splat operator at the end I guarantee the source ids will be in an array.  I'll keep looking at the rest of the code and go from there, but that was the first thing I wanted to point out.


Any better way to copy from Readable to OutputStream? I neglected java.nio package in the past and now I am wondering if this is the best way to copy everything from a Readable to an OutputStream:

public static void copy(@Nonnull Readable src, @WillNotClose @Nonnull OutputStream dest, @Nullable Charset targetCharset) throws IOException {
    targetCharset = safeCharset(targetCharset);
    CharBuffer buf = CharBuffer.allocate(CHAR_BUF_SIZE);
    ByteBuffer byteBuffer = null;
    WritableByteChannel outChannel = Channels.newChannel(dest);

    while (src.read(buf) != -1) {
      buf.flip();
      byteBuffer = targetCharset.encode(buf);
      outChannel.write(byteBuffer);
      buf.clear();
    }
}

General

Is this the best way to do it? Am I missing something? What should I do to improve this code?

Closing the channel or not

I wonder if I must/should close the channel created by Channels.newChannel(dest)?
From the general contract of my class the caller of this method here is responsible for closing the provided OutputStream, because every resource should be closed where it was created. It's bad that closing the channel also closes the underlying stream, too.

On the other hand not closing the channel relies on undocumented implementation details of Channels$WritableByteChannelImpl where calling close() is just a delegate to the underlying stream and setting a close flag.

I do not want to break my general contract and I do not want to rely on undocumented implementation details.

The third option is most probably the best and addressing the source of the problem:
Write the content directly to the OutputStream without using a channel or any other wrapper around it which must be closed then. But I could not figure out how to achieve this.

Well, last night I came up with that. I am pretty satisfied with it right now but would be happy to see if I am still missing something.

This solution does not rely on implementation details any more and provided streams are left open as the general contract demands it.

public static void copy(@Nonnull Readable src, @Nonnull @WillNotClose OutputStream dest, @Nullable Charset destCharset) throws IOException {
    CharBuffer charBuf = CharBuffer.allocate(CHAR_BUF_SIZE);

    while (src.read(charBuf) != -1) {
      charBuf.flip();
      copy(charBuf, dest, destCharset);
      charBuf.clear();
    }
}

public static void copy(@Nonnull CharBuffer src, @Nonnull @WillNotClose OutputStream dest, @Nullable Charset destCharset) throws IOException {
    copy(safeCharset(destCharset).encode(src), dest);
}

public static void copy(@Nonnull ByteBuffer src, @Nonnull @WillNotClose OutputStream dest) throws IOException {
    int len = src.remaining();
    int totalWritten = 0;
    byte[] buf = new byte[0];

    while (totalWritten < len) {
        int bytesToWrite = Math.min((len - totalWritten), BYTE_BUF_SIZE);

        if (buf.length < bytesToWrite) {
            buf = new byte[bytesToWrite];
        }

        src.get(buf, 0, bytesToWrite);
        dest.write(buf, 0, bytesToWrite);
        totalWritten += bytesToWrite;
    }
}


Simply print the string backwards This code is very, very simple, but I was wondering how it could be improved:

#include <iostream>

int main() {
    std::string input;
    std::getline(std::cin, input);

    for(int i = 1; i <= input.length(); i++){
        std::cout << input[input.length() - i];
    }

    std::cout << '\n';
    return 0;
}

This code takes a string input, and prints it backwards. For example:

This will be printed backwards.

Becomes:

.sdrawkcab detnirp eb lliw sihT

Given the simplicity of this code, is there a way to optimize it? I understand that since it is very short, it will run fast no matter what. However, I hate inefficiencies in code, and I want to help improve the way that I write code.

This is related to, but not copied directly from, this SO answer. (I only found this SO answer after writing the code)

One simple improvement would be to restructure the for() loop:

for(int i = input.length() - 1; i >= 0; --i){
    std::cout << input[i];
}

A simple decrement operation might lead to more efficient machine code than calculating input.length() - i in every iteration.

Another improvement would be to use a more c++-ish way. But that means to change the string itself:

 std::getline(std::cin,input);
 std::reverse(std::begin(input),std::end(input));
 std::cout << input << '\n';

Or using the reverse iterators in the loop:

 for(auto it = input.rbegin(); it != input.rend(); ++it) {
      std::cout << *it;
 }

As I've mentioned in my comment, the most generic (and c++stylish) way to achieve that is to write your own adapter to achieve iterating a sequence in the reverse order. So I'm citing that solution here:

 // -------------------------------------------------------------------
 // --- Reversed iterable
 using namespace std; // for rbegin() and rend()

 template <typename T>
 struct reversion_wrapper { T& iterable; };

 template <typename T>
 auto begin (reversion_wrapper<T> w) { return rbegin(w.iterable); }

 template <typename T>
 auto end (reversion_wrapper<T> w) { return rend(w.iterable); }

 template <typename T>
 reversion_wrapper<T> reverse (T&& iterable) { return { iterable }; }

Above taken from here.