Iteritems on a slice of a Python list iteritems on a dict can useful.

Occasionally iteritems is useful for a slice of a list and this effect can be (crudely) implemented as follows:

class List(list):
  def iteritems(self, slice=None):
    if slice is None: return enumerate(self)
    else: return itertools.izip(range(*slice.indices(len(self))), self[slice])

if __name__ == "__main__":
  l=List("hAnGtEn")
  print l
  print list(l.iteritems())
  print list(l.iteritems(slice(1,None,2)))

Output:

['h', 'A', 'n', 'G', 't', 'E', 'n']
[(0, 'h'), (1, 'A'), (2, 'n'), (3, 'G'), (4, 't'), (5, 'E'), (6, 'n')]
[(1, 'A'), (3, 'G'), (5, 'E')]

Is there a more "pythonic" list slicing syntax that should be used?

This:

range(slice.start,slice.stop,slice.step)

does not handle certain special cases very well: e.g. where stop=-1, start=None or step=None. How can the example range/slice implementation be also improved?

edit:

range(slice.start,slice.stop,slice.step)

is better handled with:

range(*slice.indices(len(self)))

Instead of

range(slice.start,slice.stop,slice.step)

you could use this expression that handles the special cases too

range(len(self))[slice]

(This works with range on both Python 2 and 3, but not with Python 2 xrange even though it is mostly equivalent to Python 3 range)


Method for downloading a PDF from a website in Android How is this? Any ways to improve?

public void DownloadPDF(Uri uri) {
    downloadProgress.setVisibility(View.VISIBLE);
    downloadComplete.setVisibility(View.INVISIBLE);
    openFile.setText("Wait..."); // My button
    openFile.setEnabled(false);
    if (uri != null) {
        DownloadManager.Request request = new DownloadManager.Request(uri);
        request.setTitle(pdf + ".pdf");
        request.setDescription("Website: " + pdf);
        request.setMimeType("application/pdf");
        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) {
            request.allowScanningByMediaScanner();
            request.setNotificationVisibility(DownloadManager.Request.VISIBILITY_VISIBLE_NOTIFY_COMPLETED);
        }
        request.setDestinationInExternalPublicDir(Environment.DIRECTORY_DOWNLOADS + "/SavedPDFs/", pdf + ".pdf");
        manager = (DownloadManager) getSystemService(Context.DOWNLOAD_SERVICE);
        downloadId = manager.enqueue(request);
        onComplete = new BroadcastReceiver() {
            public void onReceive(Context ctxt, Intent intent) {
                try {
                    unregisterReceiver(onComplete);
                } catch(IllegalArgumentException e){
                    e.printStackTrace();
                }
                downloadComplete.setVisibility(View.VISIBLE);
                downloadProgress.setVisibility(View.INVISIBLE);
                downloadStatus.setText("Download Complete!");
                updateNumberOfFiles();
                openFile.setText("Open!");
                openFile.setEnabled(true);
            }
        };
        registerReceiver(onComplete, new IntentFilter(DownloadManager.ACTION_DOWNLOAD_COMPLETE));
    }
}

Android's method naming convention follows that of Java, so your method name DownloadPDF() will probably be better written as downloadPDF() (or maybe even downloadPdf()).

Your null check for uri comes in after you have toggled your UI elements, wouldn't that mean the state is possibly inconsistent if the method really received a null argument, and then the UL elements already indicated the code is downloading something? You may want to return from your method first in this case...

public void downloadPdf(URL uri) {
    if (uri == null) {
        return;
    }
    // ...
}

DownloadManager.Request methods return the object itself, so you may also consider daisy-chaining the setters together:

request.setTitle(pdf + ".pdf")
    .setDescription("Website: " + pdf)
    .setMimeType("application/pdf");


printf-style string formatter As part of a larger project I'm working on, I've made a function that aims to replicate, as best as I can, the placeholder part of the console.log() function usable in most browsers.

It replaces %s, %d , %i , %f and ,%o. It works by having 'unlimited' parameters (the replacements), looping through the parameters, checking for any placeholders and replacing them by index with the parameter given. The switch is for checking whether the passed parameter (replacement) is the correct type - if not, it will add NaN, just like the real console would.

My main questions are:

Is my code readable?
Are there any better ways to do any part of this?
Can it be cleaned up anymore?

function replacePlaceholders(string) {
    var args = Array.prototype.slice.call(arguments).splice(1); //get all the extra arguments (exclude the original string)
    var counter = 0; //set a counter

    for (var i = 0; i < args.length; i++) { //loop through the extra arguments (should match the number of placeholders in the string)
        var match = /%s|%d|%i|%f|%o/.exec(string); //regex
        if (match) { //if match found
            var index = match.index; //store the index
            switch (match[0]) { //check whether the placeholder has a real value with the correct type
                case '%s': //for strings
                    string = (typeof args[counter] == 'string') ? string.substr(0, index) + args[counter] + string.substr(index + 2) : string.substr(0, index) + NaN + string.substr(index + 2);
                    break;
                case '%d': //for numbers....
                case '%i':
                case '%f':
                    string = (typeof args[counter] == 'number') ? string.substr(0, index) + args[counter] + string.substr(index + 2) : string.substr(0, index) + NaN + string.substr(index + 2);
                    break;
                case '%o': //for objects
                    string = (typeof args[counter] == 'object') ? string.substr(0, index) + args[counter] + string.substr(index + 2) : string.substr(0, index) + NaN + string.substr(index + 2);
                    break;
            }
        }
        counter++;
    }
    return string;
}

console.log(replacePlaceholders("string %s %d %i %f %o", 'string', 1, 4, 5.5, {one: 1})); //test case

You can test this here - check the console for the returned string.

Before I start: Thank you so much for making this code easily readable! Thank you from the bottom of my heart!

Second thing that I have noticed was the name. It's wrong! If it is a sptrinf-like function, give it the right name. sprintf! Done! No overly complicated boring names.

You have an argument called string. Please, don't do that. In fact, remove it's name! You know it is the first element.

You can do this:

var text = arguments[0];
for (var i = 1; i < arguments.length; i++) {

Done! But it won't be needed and you will see why.

You still didn't learn: Store the length in a local variable. Always! Like this:

for (var i = 1, length = args.length; i < length; i++) {

Accessing a property in an object (in this case, an array) is slower than accessing a loval variable. By setting the local variable with that slow property, we increase performance.

It is really a great idea to have literal regular expressions, but the whole code is quite... shady... Specially the useless validations!

You expect a string to be a real string so you can do something... But why? What about a number? What about an array? What around something else? Why can't I convert anything into a string? Every single object has the .toString() method.

And why that NaN there? What is it for? Is that to say it is NaN?

The %o is doing something somewhat somewhere that I don't get. By the C++ documentation and the PHP documentation specify that this is the octal representation.

And now, the questions:

Why do you check if an argument is a string before converting it to string?
Why do you check if an argument is og any type before converting?
why don't you do any kind of validation?
Why are you using exec?

I would never do it on this way. It's so frail and picky. It's so hard to change anything without breaking it. You have to deal with things you should, like breaking a string in 3 parts and replacing the middle part with something. That's a bad way to do a .replace().

I would do like this:

function sprintf(){
    var toBase = function(value, base){
        //converts to the required bases
        return (parseInt(value, 10) || 0).toString(base);
    };
    var map = {
        s: function(value){
            return value.toString();
        },
        d: function(value){
            return toBase(value, 10);
        },
        i: function(value){
            return this.d(value);
        },
        b: function(value){
            //binary conversion
            return toBase(value, 2);
        },
        o: function(value){
            //octal conversion
            return toBase(value, 8);
        },
        x: function(value){
            //hexadecimal conversion
            return toBase(value, 16);
        },
        X: function(value){
            //HEXADECIMAL CONVERSION IN CAPITALS
            return toBase(value, 16).toUpperCase();
        },
        e: function(value){
            //scientific notation
            return (parseFloat(value, 10) || 0).toExponential();
        },
        E: function(value){
            return this.e(value, 10).toUpperCase();
        },
        f: function(value){
            //floating point
            return (parseFloat(value, 10) || '0.0').toString();
        }
    };

    //crude way to extract the keys
    var keys = '';
    for(var k in map)
    {
        keys += k;
    }
    var args = Array.prototype.slice.call(arguments).slice();

    return args.shift().toString().replace(new RegExp('%([' + keys + '])','g'),function(_, type){
        if(!args.length)
        {
            throw new Error('Too few elements');//appropriate type here?
        }

        return map[type](args.shift());

    });
}

Come on! It's so easy to understand! You can change it's functionality easily!

If you want, you can remove that loop to fetch the keys and use this literal regular expression: /%([diobsxXeEf])/g. Same thing, but faster and less flexible.

And this is a "good enough" clone of the sprintf function. It doesn't support padding, float-number size specifitation and others. But, it works for the bare minimum.

Outside the scope of this review, thanks to you, I finally have a function to write my polyglot questions! Also, you may want to look at http://phpjs.org/functions/sprintf/ if you want a perfect clone of the sprintf function.

To reflect closely the desired implementation, one can change the map variable. I went a bit further and changed all unnecessary things. To be consistent with the MDN documentation and the Chrome console API documentation, you can do this:

function sprintf(){
    var map = {
        s: function(value){
            return value.toString();
        },
        d: function(value){
            return (parseInt(value, 10) || 0).toString();
        },
        i: function(value){
            return this.d(value);
        }
        f: function(value){
            return (parseFloat(value, 10) || '0.0').toString();
        }
    };

    var args = Array.prototype.slice.call(arguments).slice();

    return args.shift().toString().replace(/%([difs])/g,function(_, type){
        if(!args.length)
        {
            throw new Error('Too few elements');
        }

        return map[type](args.shift());

    });
}

The %o, %O and %c specifiers are implemented internally and isn't possible to implement using this method. %c and %Cwork on elements, by creating a list in the console or a link to the element to show in the Element Inspector. The %c uses CSS styles to style the output in the console itself.


scatter subplot for iris dataset I'm new to data science. I wrote this script for plotting all different kinds of iris data set scatter plot. trying not to plot something with itself . how can I optimize my code ?

        from sklearn.datasets import load_iris
        import matplotlib.pyplot as plt
        import numpy as np

        import pandas as pd

        iris=load_iris()

        list1=[]

        fig, ax =plt.subplots(nrows=3,ncols=2,figsize=(10,10))

    for ii in range(4):
      for jj in range(1,4):
        if ii==jj:
          break
        if ii*jj not in list1[1::2]:
          list1.extend((ii+jj,ii*jj))
        elif ii+jj in list1[::2]:
          break
        a=ii
        b=jj
        x_index=ii
        y_index=jj
        colors=['blue','red','green']
        if ii==0:
          b=b-1
        elif jj==1:
          a=a-2
          b,a=a,b
        elif ii==3:
          a=a-1
          b=b-1
          a,b=b,a
        for label , color in zip(range(len(iris.target_names)),colors):
          ax[b,a].scatter(iris.data[iris.target==label,x_index]
                  , iris.data[iris.target==label,y_index]
                  , label=iris.target_names[label]
                  , color=color)

        ax[b,a].set_xlabel(iris.feature_names[x_index])
        ax[b,a].set_ylabel(iris.feature_names[y_index])
        ax[b,a].legend(loc="upper right")
        fig.tight_layout()
        fig.show()

this is the output
how would you write it if it was you?
I appreciate any help.

Two remarks on your code:

There seems to be an indentation error in your code, everything before your first for loop has too much indentation.

In general, it is a good idea to use telling names for variables. For example, column and row instead of a and b.

Following is my version of the code. It is not an exact reproduction of your plot, but the way I would approach an analysis like this:
from sklearn.datasets import load_iris
import pandas as pd
import seaborn as sns

iris = load_iris()

# convert dataset to pandas DataFrame
iris_df = pd.DataFrame(iris['data'])
iris_df.columns = iris['feature_names']
iris_df['species'] = iris['target']
iris_df['species'] = iris_df['species'].replace([0, 1, 2],  iris['target_names'])

# alternative: load dataset from seaborn library (they use slightly different column names)
# iris_df = sns.load_dataset('iris')

sns.pairplot(data=iris_df, corner=True, hue='species')

This produces the following plot:

As you can see, the code has become much shorter and the resulting figure still contains all the plots you wanted (plus a few extra ones). Great thing about using pandas and other related libraries (e.g., seaborn) is that you can often write very compact code (e.g., by getting rid of many for loops).
First, I converted the iris dataset to a pandas DataFrame (documentation). A DataFrame is the main data type in pandas and makes analysis and processing of your data much easier. As shown in the code, there is an alternative way of loading the iris dataset into python using the seaborn library (sns.load_dataset('iris')) This will give you the dataset directly as a DataFrame, no more need to convert it.
Then, I call pairplot from the seaborn library. seaborn is a plotting library based on matplotlib that works with DataFrame objects. It has a high-level interface, meaning you can produce complex plots such as the one you see with often a few lines of code.

I just found out that pandas already has a very similar plot function, check out  scatter_matrix. So you don't necessarily need to use seaborn (even though it's a great library and I recommend it).


Splitting strings in C I have created some code to split strings. The code will split the given string by the given delimiter into an array which is inside a struct it returns.

I am using strtok to do the actual tokenizing, but in my code the original string will not be changed.

split.h

#ifndef SPLIT_H
#define SPLIT_H

#ifdef __cplusplus
extern "C" {
#endif

    struct splitString {
        char **tokens;
        unsigned total;
        unsigned size;
    };

    struct splitString *split(const char *, const char *);
    void splitFree(struct splitString **);

#ifdef __cplusplus
}
#endif

#endif /* SPLIT_H */

split.c

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "split.h"

#define DEFAULT_TOKEN_SIZE 10

static struct splitString* initSplitString() {
    struct splitString *s = malloc(sizeof (struct splitString));

    if (s) {
        s->total = 0;
        s->size = DEFAULT_TOKEN_SIZE;
        s->tokens = malloc(sizeof (char *) * DEFAULT_TOKEN_SIZE);

        for (int i = 0; i < DEFAULT_TOKEN_SIZE; i++) {
            s->tokens[i] = NULL;
        }

        return s;
    }

    return NULL;
}

/**
 * Adjust the size of the tokens variable if necessary
 * @param s
 */
static void adjustSize(struct splitString *s) {
    if (s->total == s->size) {
        s->size += DEFAULT_TOKEN_SIZE;

        char **temp_tokens = realloc(s->tokens, sizeof (char *) * s->size);

        if (temp_tokens) {
            s->tokens = temp_tokens;
        }
    }
}

static void clearAndCopy(char *destination, const char *source, unsigned size) {
    memset(destination, 0, size);
    memcpy(destination, source, size);
}

struct splitString* split(const char *string, const char *delimiter) {
    unsigned size = strlen(string) + 1;
    char tmp[size];
    char *token = NULL;
    struct splitString *s = initSplitString();

    if (s == NULL) {
        goto error;
    }

    clearAndCopy(tmp, string, size);

    token = strtok(tmp, delimiter);

    while (token != NULL) {
        int bsize = strlen(token) + 1;

        s->tokens[s->total] = malloc(bsize);

        if (s->tokens[s->total]) {
            clearAndCopy(s->tokens[s->total], token, bsize);
        }
        else {
            splitFree(&s);
            goto error;
        }

        s->total++;

        adjustSize(s);
        token = strtok(NULL, delimiter);
    }

    return s;

error:

    return NULL;
}

/**
 * Clear all token and the splitString
 * @param s
 */
void splitFree(struct splitString **s) {
    for (int i = 0; i < (*s)->total; i++) {
        free((*s)->tokens[i]);
        (*s)->tokens[i] = NULL;
    }

    free(*s);
    *s = NULL;
}

Here is just some code I am using to test:

#include <stdio.h>
#include <stdlib.h>
#include "split.h"

int main(int argc, char** argv) {

    char *test_string = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.";
    struct splitString *test = split(test_string, ",");

    printf("Total: %d\n", test->total);

    for (int i = 0; i < test->total; i++) {
        printf("String: %s\n", test->tokens[i]);
    }

    splitFree(&test);

    return EXIT_SUCCESS;
}

Unnecessary code

This function is puzzling to me:

static void clearAndCopy(char *destination, const char *source, unsigned size) {
    memset(destination, 0, size);
    memcpy(destination, source, size);
}

Why do you need to clear the destination buffer when you are going to copy over it immediately afterwards?  All calls to this function could simply be replaced by calls to memcpy instead.

Memory leak

In split_free(), you forgot to free (*s)->tokens.

Allocation strategy

There are two things I would do differently with respect to how you allocate your tokens:

Instead of using realloc to dynamically resize your token array, I would prescan the input and determine how many tokens there will be, and just allocate once.  This avoids a possible \$O(n^2)\$ reallocation time.
Instead of allocating a buffer for each token and copying the token to it, I would make a single copy of the input string, and then have s->tokens[i] be pointers to the tokens within the single copy.


Numbers adder via generics This method add all numbers in list and return it as double

public HashMap<String, Double> add(HashMap<String, List<? extends Number>> map){
    HashMap<String, Double> result = new HashMap<>();

    map.forEach((key, value) -> {
        Double sum = 0.0;

        for (Number num : value){
            sum += num.doubleValue();
        }

        result.put(key, sum);
    });
    return result;
}

Testing my method with different types

public static void main(String[] args) {
    Adder adder  = new Adder();
    HashMap<String, List<? extends Number>> hashMap = new HashMap<>();

    List<Integer> list1 = new ArrayList<>();
    list1.add(1);
    list1.add(2);
    list1.add(3);

    List<BigDecimal> list2 = new ArrayList<>();
    list2.add(new BigDecimal("9.87654321e300"));
    list2.add(new BigDecimal("987654321"));
    list2.add(new BigDecimal("987654321"));

    hashMap.put("list1", list1);
    hashMap.put("list2", list2);

    System.out.println(adder.add(hashMap));

}

Is there way to improve this?

The following code is very inefficient:

    Double sum = 0.0;

    for (Number num : value){
        sum += num.doubleValue();
    }

It creates a Double object for 0.0, and then, since Double objects are immutable, for every num in the value list, it creates yet another Double object for the resulting partial sum.  If you replaced Double with double, then you won't be creating new objects for each iteration through the loop.

Since you are already using the stream API, why not use DoubleStream::sum to sum the values as a double?

    double sum = value.stream().mapToDouble(Number::doubleValue).sum();

Finally, you could use the Collectors.toMap() stream method to create the map.

public Map<String, Double> add(Map<String, List<? extends Number>> map) {
    return map.entrySet().stream()
          .collect(Collectors.toMap(Map.Entry::getKey,
                                    e -> e.getValue().stream()
                                              .mapToDouble(Number::doubleValue)
                                              .sum()));
}

Whether or not this last step is an "improvement" may be a matter of taste.

Note: the return type is no longer a HashMap; it just a Map.

The add() method doesn't depend on any members of Adder, so the method should be static.  Then you can get rid of the Adder adder = new Adder() object.

Using List.of(...) and Map.of(...) can simplify your test code:

public static void main(String[] args) {

    List<Integer> list1 = List.of(1, 2, 3);
    List<BigDecimal> list2 = List.of(new BigDecimal("9.87654321e300"),
                                     new BigDecimal("987654321"),
                                     new BigDecimal("987654321"));

    Map<String, List<? extends Number>> hashMap = Map.of("list1", list1,
                                                         "list2", list2);

    System.out.println(Adder.add(hashMap));
}


Sort ArrayList in a better way I have a list of categories (in this example I used String but in my project I have a model) which must be sorted like this:

on top we should have 2 categories which have a fixed position in the list (in this example (C, F))
at the bottom of the list we should have 3 categories which have a fixed position in the list (in this example (R, M, P))
in the middle of the list we should have the rest of the elements sorted alphabetically

I want to know if there is a better solution than I used, for sorting the list this way. I do not want to use HashMap, so if anyone could find a better solution for sorting the ArrayList, I would highly appreciate it.

List<String> categories = new ArrayList<>();
    categories.add("A");
    categories.add("B");
    categories.add("C");
    categories.add("D");
    categories.add("E");
    categories.add("F");
    categories.add("G");
    categories.add("H");
    categories.add("I");
    categories.add("J");
    categories.add("K");
    categories.add("L");
    categories.add("M");
    categories.add("N");
    categories.add("O");
    categories.add("P");
    categories.add("Q");
    categories.add("R");

    sortCategories(categories);

The sortCategories() method is:

public void sortCategories(List<String> categories) {
    List<String> topCategories = new ArrayList<>();
    List<String> bottomCategories = new ArrayList<>();

    for (String category : categories) {
        if (category.equals("C")) {
            topCategories.add(category);
            break;
        }
    }

    for (String category : categories) {
        if (category.equals("F")) {
            topCategories.add(category);
            break;
        }
    }

    for (String category : categories) {
        if (category.equals("R")) {
            bottomCategories.add(category);
            break;
        }
    }

    for (String category : categories) {
        if (category.equals("M")) {
            bottomCategories.add(category);
            break;
        }
    }

    for (String category : categories) {
        if (category.equals("P")) {
            bottomCategories.add(category);
            break;
        }
    }

    categories.removeAll(topCategories);
    categories.removeAll(bottomCategories);

    Collections.sort(categories, new Comparator<String>() {

        @Override
        public int compare(String lhs, String rhs) {
            return lhs.compareTo(rhs);
        }
    });

    categories.addAll(0, topCategories);
    categories.addAll(categories.size(), bottomCategories);
}

And the output is:

Category NAME C
Category NAME F
Category NAME A
Category NAME B
Category NAME D
Category NAME E
Category NAME G
Category NAME H
Category NAME I
Category NAME J
Category NAME K
Category NAME L
Category NAME N
Category NAME O
Category NAME Q
Category NAME R
Category NAME M
Category NAME P

You should not hard-code category values into the code, but rather make them configurable, so that your code may be re-used with other top/bottom category values. Since you have a specific sorting algorithm, it is probably best to create a custom comparator, which encapsulates all of the sorting logic. That comparator would be initialized with the values allowed for top/bottom categories, and would do the sorting, taking these values into account.

Using the comparator should be as simple as:

    List<String> allCategories = Arrays.asList(
            "A", "B", "C", "D", "E", "F",
            "G", "H", "I", "J", "K", "L",
            "M", "N", "O", "P", "Q", "R"
            );
    List<String> topCategories = Arrays.asList("C", "F");
    List<String> bottomCategories = Arrays.asList("M", "P", "R");
    Collections.sort(allCategories, new CategoryComparator(topCategories, bottomCategories));

There are many ways to implement such a comparator.The main idea is to extend the comparison logic: in addition to comparing values, you also compare their belonging to a particular category, and adjust the score consequently. As a result, values belonging to the top category should be put on top, and if two values belong to the same category, they should be compared according to their natural order.

public class CategoryComparator implements Comparator<String> {

    private static final int POSITIVE_SCORE = +1;
    private static final int NEGATIVE_SCORE = -1;
    private static final int TOP_SCORE_ADJUSTMENT = -2;
    private static final int BOTTOM_SCORE_ADJUSTMENT = +2;

    private Set<String> topCategories;
    private Set<String> bottomCategories;

    public CategoryComparator(Collection<String> topCategories, Collection<String> bottomCategories) {
        this.topCategories = new HashSet<>(topCategories);
        this.bottomCategories = new HashSet<>(bottomCategories);
    }

    @Override
    public int compare(String o1, String o2) {
        return normalizeScore(o1.compareTo(o2)) + ajustScoreForCategory(o1) - ajustScoreForCategory(o2);
    }

    private int normalizeScore(int score) {
        if (score < 0) {
            return NEGATIVE_SCORE;
        }
        if (score > 0) {
            return POSITIVE_SCORE;
        }
        return 0;
    }

    private int ajustScoreForCategory(String value) {
        int scoreAdjustment = 0;
        if (topCategories.contains(value)) {
            scoreAdjustment += TOP_SCORE_ADJUSTMENT;
        }
        if (bottomCategories.contains(value)) {
            scoreAdjustment += BOTTOM_SCORE_ADJUSTMENT;
        }
        return scoreAdjustment;
    }
}


SPOJ "Pesel" challemge Here is a solution to the SPOJ's JPESEL problem. Basically the problem was to calculate cross product of 2 vectors modulo 10. If there is a positive remainder, it's not a valid PESEL number; (return D) if the remainder equals 0, it's a valid number (return N).

import re, sys
n = input()
t = [1,3,7,9,1,3,7,9,1,3,1]
while(n):
    p = map(int, re.findall('.',sys.stdin.readline()))
    # Another approach I've tried in orer to save some memory - didn't help:
    # print 'D' if  sum([int(p[0]) * 1,int(p[1]) * 3,int(p[2]) * 7,int(p[3]) * 9,int(p[4]) * 1,int(p[5]) * 3,int(p[6]) * 7,int(p[7]) * 9,int(p[8]) * 1,int(p[9]) * 3,int(p[10]) * 1]) % 10 == 0 else 'N'
    print 'D' if ( sum(p*q for p,q in zip(t,p)) % 10 ) == 0 else 'N'
    n-=1

The solution above got the following results at SPOJ:

Time: 0.03s
Memory: 4.1M

where the best solutions (submitted in Python 2.7) got:

Time: 0.01s
Memory: 3.7M

How can I optimize this code in terms of time and memory used?

Note that I'm using different function to read the input, as sys.stdin.readline() is the fastest one when reading strings and input() when reading integers.

Likely what is taking up extra time is the line:

 p = map(int, re.findall('.',sys.stdin.readline()))

There are a few things here. Firstly, regular expressions are relatively expensive. Underneath, it likely needs to compile the regular expression (which in this case is very simple, but still). Further, you don't need regular expressions to do this in python. For example, assume the string was something like '343898934'. Then this is equivalent to:

p = [int(k) for k in '343898934']

The above will likely be slightly faster.

The second place here that could be optimized (memory wise) is that map in Python 2.x returns a list as opposed to a generator. So instead we could do:

p = (int(k) for k in sys.stdin.readline())

The fact that this also removes the re import may also impact timing and memory usage, depending on how they measure.


Valid palindrome solution I'm working on this valid palindrome problem. Any advice on code bug, better idea for low algorithm execution time complexity, code style, etc. are highly appreciated.

  Problem

  Given a string, determine if it is a palindrome, considering only
  alphanumeric characters and ignoring cases.

  Example

  "A man, a plan, a canal: Panama" is a palindrome. "race a car" is not
  a palindrome.

  Note

  Have you consider that the string might be empty? This is a good
  question to ask during an interview. For the purpose of this problem,
  we define empty string as valid palindrome.

Source Code

def check_valid(source):
    i = 0
    j = len(source)-1
    source =source.lower()
    while i <= j:
        while i<=j and not ('a'<=source[i]<='z'):
            i+=1
        while i<=j and not ('a'<=source[j]<='z'):
            j-=1
        if i<=j:
            if source[i] != source[j]:
                return False
            else:
                i+=1
                j-=1
        if i > j:
            return True

if __name__ == "__main__":
    print check_valid('A man, a plan, a canal: Panama') # return True
    print check_valid('race a car') # return False

Some small nitpicks:

Instead of i <= j you can stop before j by doing: i < j in the last two while loops. That way, you'll save one iteration per while. If you weren't constraint by: "For the purpose of this problem, we define empty string as valid palindrome" you might've remove that as well
Here: ('a'<=source[i]<='z') you can remove the redundant parentheses and also write it like: source[i].isalnum()
It seems like you always forget to put a space between operators: i+=1 should be i += 1.
Add docstrings to your function

Reviewed code:
def check_valid(source):
    """ Return True/False if a string is a palindrome """

    i, j, source = 0, len(source) - 1, source.lower()
    while i <= j:
        while i < j and not source[i].isalnum():
            i += 1
        while i < j and not source[j].isalnum():
            j -= 1
        if i <= j:
            if source[i] != source[j]:
                return False
            else:
                i += 1
                j -= 1
        if i > j:
            return True

if __name__ == "__main__":
    print check_valid('A man, a plan, a canal: Panama')  # return True
    print check_valid('race a car')  # return False


Approximation of error-function erf(x) The code beyond approximates the error-function erf(x) with following formular \$erf(x)=1-(a_1t+a_2t^2+a_3t^3)e^{-x^2})\$ for \$x\geq0\$ inclusive the identity \$erf(-x)=-erf(x)\$.

Is it okay to end the program in the middle of a code with exit(EXIT_FAILURE)? I just tried to avoid it and came to following solution, where the program always ends with return 0 at the end of the main-programm:
https://codepaste.net/ek8ed5 So the question is whats's the more correct way?

(I hope a moderator can paste this code section in here please, because somehow I didn't make it work to show it as nicely formatted code, sorry!)

What else can I improve?

.

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

#define p 0.47047
#define a_1 0.3480242
#define a_2 -0.0958798
#define a_3 0.7478556

double ErrorFunction(double x);

int main(void)
{
  double x = 0;

  do
  {
    printf("Enter any value x to show erf(x): ");

    if (scanf_s("%lf", &x) != 1)
    {
      printf("error: invalid input\n");
      exit(EXIT_FAILURE);
    }
    else
    {
      printf("\n erf(x)=%f\n\n", ErrorFunction(x));
    }

  } while (x != 0);

  return 0;
}

double ErrorFunction(double x)
{
  double t = 0;

  if (x < 0)
  {
    x = -x;
    t = 1 / (1 + p*x);
    return -(1 - (a_1*t + a_2*t*t + a_3*t*t*t)*exp(-(x*x)));
  }
  else
  {
    t = 1 / (1 + p*x);
    return 1 - (a_1*t + a_2*t*t + a_3*t*t*t)*exp(-(x*x));
  }
}

DRY. Actual computations are repeated twice, which is always a signal for improvement:

if (x < 0) {
    t = 1 / (1 - p*x);
} else {
    t = 1 / (1 + p*x);
}

result = compute_the_formula(t, x);

if (x < 0) {
    result = -result;
}
return result;

Try to cut down multiplications. Instead of

a_1*t + a_2*t*t + a_3*t*t*t

consider

t*(a_1 + t*(a2 + t*a_3)))

Three multiplications instead of original six is not only faster, but generally more accurate.


Translating status codes to messages in log files I'm not overly efficient when it comes to coding, and I am looking for ways to improve the speed of a log translator I built.

Several files are uploaded to a folder on a server and log files are selected for processing. Speed here is fine. Each log file has 5000-10000 lines possible a bit higher. They are space delimited files that look like this:

20:04:13 + MA 00 61
20:04:17 - MA 00 61
20:04:18 + MA 00 61 optionaltxt
20:04:20 - MA 00 61

There are at least 5 fields for every entry but more can be added if optional text exists. Field 5 is a status code, and it has to be cross referenced with a language file then replaced with the corresponding message so the result is like this:

17:29:48 - MA 00 (061) Feed hold! X / Y axis is locked
17:29:48 + MA 00 (061) Feed hold! X / Y axis is locked
17:29:50 - MA 00 (061) Feed hold! X / Y axis is locked optionaltxt
17:29:51 + MA 00 (061) Feed hold! X / Y axis is locked

The translated logs are saved to a new folder for download later. At the moment translation takes about 7-10 seconds per log file. The problem is that it is common to have 90 files or more and the time starts adding up. I have no control over the log format or language file format.

This part of my code looks like this:

$shLANG = file_get_contents("ErrCODES/data_en.properties");
$SHarray = explode("\n", $shLANG);

$logARRAY = glob($uploadDIR.$filepath."/original/*.[lL][oO][gG]");
foreach ($logARRAY as $logFILEarray) {

$name = basename($logFILEarray);
$logFILE = file_get_contents($uploadDIR.$filepath."/original/".$name);
$logFILEarray = explode("\n", $logFILE);
foreach ($logFILEarray as $key => $line) {
        $newline="";
        $LINEarray = explode(" ", $line);

        if(isset($LINEarray[0])){$newline .= $LINEarray[0]." ";}
        if(isset($LINEarray[1])){$newline .= $LINEarray[1]." ";}
        if(isset($LINEarray[2])){$newline .= $LINEarray[2]." ";}
        if(isset($LINEarray[3])){$newline .= $LINEarray[3]." ";}
        if(isset($LINEarray[4])){
            foreach ($SHarray as $code) {
                if (strlen($LINEarray[4])>0){
                    if (substr($code, 0, strpos($code, '=')) == $LINEarray[4]) {
                        $newline .= trim(explode('null;', $code)[1]). " ";
                    }
                }
            }
        }

        if(isset($LINEarray[5])){$newline .= $LINEarray[5]." ";}
        if(isset($LINEarray[6])){$newline .= $LINEarray[6]." ";}
        if(isset($LINEarray[7])){$newline .= $LINEarray[7]." ";}
        if(isset($LINEarray[8])){$newline .= $LINEarray[8]." ";}
        $newline .= "\r";
        $logFILEarray[$key] = $newline;
        }

//var_dump($logFILEarray);
$info = implode("\n",$logFILEarray);
file_put_contents($uploadDIR.$filepath."/translation/".$name, $info);

}//for all glob .log

The code works fine, but how can I improve the speed (greatly)?

What you have looks problematic in a few ways.

You have a poor data structure for your localization strings.  You have these in an array the must be iterated every single time you are doing a lookup against it. This is an linear complexity (O(n)) operation that when executed inside your loop (another O(n) iteration) give you O(n^2) overall performance. @TimSparrow has touched on a good alternative - to load the localization strings into an associative array the will allow O(1) lookup when being used - but perhaps didn't explain how critical this is to your performance.  By building such a data structure up front, you whole algorithm goes to O(m) (to build localization data structure) + O(n) (from your log file loop) instead of O(m * n).  To illustrate the impact to your performance, let assume you have a 10K line log file and a 100 line localization file.  With your current code, this means you could have up to 1 Million lookup operations you have to perform vs. 10,100 if using a proper data structure.
This code is a memory hog.  You store every file you are working with wholly in memory, as well as storing the same data in duplicate in the result arrays you are building, not to mention all the temporary array/string conversion you have that, again mean you are going to be holding that information in duplicate in memory.  If you can get the memory utilization more optimized here, then you may be more able to do things like for each of the individual log into their own process to parallelize the work.
You should consider working with the rows in a more structured manner vs. working with them primarily as strings.  fgetcsv(), fputcsv() or similar give you better ways to parse through and write your files.

Incorporating the above thoughts should allow you to greatly streamline both the performance of your script as well as just simplify how it works.  You might end up with something like:

$localizations = [];
$handle = fopen('ErrCODES/data_en.properties','r');
// read lines from file one at a time.
while ($line = fgetcsv($handle, 0, ' ')) {
    // this part may vary based on your format
    $localizations[$line[0]] = $line[1];
}
fclose($handle);

$logFiles = glob($uploadDIR.$filepath."/original/*.[lL][oO][gG]");
// here is where at some point you may want to fork processes to paralellize
// now it is serial foreach loop
foreach ($logFiles as $logFile) {
    $translatedFile = str_replace('/original', '/translation/', $logFile);
    $logHandle = fopen($logFile,'r');
    $translateHandle = fopen($translatedFile, 'w+');
    while($line = fgetcsv($logHandle, 0, ' ') {
        // apply localization
        if(!empty($line[4]) && isset($localization[$line[4]])) {
            $line[4] = $localization[$line[4]];
        }
        fputcsv($translateHandle, $line, ' ');
    }
    fclose($translateHandle);
    fclose($logHandle);
}

A few final thoughts on style:
 - Your style is a little bit all over the place, which makes your code a littel hard to read.
 - Particularly variable naming is odd.  PHP tends to use a mix of camelCase and snake_case in most code bases you will find (internal PHP functions mix both unfortunately).  Your approach to variable syntax is very haphazard, sometimes oddly putting portions of variable names in upper case with apparent cohesive approach, sometimes first letter is uppercase, sometimes it is lower case, etc.
 - Your indentations are inconsistent.
 - You could be well-served by better use of spacing around flow control operators.  A line of code like if(isset($LINEarray[0])){$newline .= $LINEarray[0]." ";} is extremely hard to read.
 - Consider familiarizing yourself with the PHP-FIG PHP standards, particularly PSR-1 and PSR-2 which deal with aspects of coding style.  These are pretty much the de-facto industry standards.


Create a matrix where if an element in an M×N matrix is 0, its entire row and column are set to 0 How is my implementation? Are there improvements I can make to the variable names or any edge cases I might have missed?

public class ZeroMatrix {
    public static void main(String[] args) {
        int[][] matrix = new int[][]{
                {1, 0, 2},
                {3, 4, 1}
        };
        System.out.println(Arrays.deepToString(setZeroes(matrix)));
    }

    private static int[][] setZeroes(int[][] matrix) {
        boolean[] rowsWithZero = new boolean[matrix.length];
        boolean[] columnsWithZero = new boolean[matrix[0].length];

        for (int i = 0; i < matrix.length; i++) {
            for (int j = 0; j < matrix[0].length; j++) {
                if (matrix[i][j] == 0) {
                    rowsWithZero[i] = true;
                    columnsWithZero[j] = true;
                }
            }
        }
        // Iterate through the rowsWithZero array and transform all rows with 0 to 0
        for (int i = 0; i < rowsWithZero.length; i++) {
            if (rowsWithZero[i]) {
                transformRow(i, matrix);
            }
        }

        // Iterate through columnsWithZero and transform all columns with 0 to 0.

        for (int i = 0; i < columnsWithZero.length; i++) {
            if (columnsWithZero[i]) {
                transformColumn(i, matrix);
            }
        }

        return matrix;

    }

    private static void transformColumn(int column, int[][] matrix) {
        for (int row = 0; row < matrix.length; row++) {
            matrix[row][column] = 0;
        }
    }

    private static void transformRow(int row, int[][] matrix) {
        for (int column = 0; column < matrix[0].length; column++) {
            matrix[row][column] = 0;
        }
    }
}

comments

Comments should say why the code is like it is. Any other information to the reader should be given to the reader by the identifiers to chose.

Your comments structure the method in 3 logical sections (the first without a dedicated comment). You should better move this three sections into separate methods with names derived from the comments:

private static int[][] setZeroes(int[][] matrix) {
    boolean[] rowsWithZero = new boolean[matrix.length];
    boolean[] columnsWithZero = new boolean[matrix[0].length];

    findZeros(matrix,rowsWithZero,columnsWithZero);
    transformColumns(matrix,columnsWithZero);
    transformRows(matrix,rowsWithZero);

    return matrix;
}
private static void findZeros(int[][] matrix, boolean[] rowsWithZero, boolean[] columnsWithZero){
    for (int i = 0; i < matrix.length; i++) {
        for (int j = 0; j < matrix[0].length; j++) {
            if (matrix[i][j] == 0) {
                rowsWithZero[i] = true;
                columnsWithZero[j] = true;
            }
        }
    }
}
private static void transformColumns(int[][] matrix,  boolean[] columnsWithZero){
    for (int i = 0; i < columnsWithZero.length; i++) {
        if (columnsWithZero[i]) {
            transformColumn(i, matrix);
        }
    }
}
private static void transformRows(int[][] matrix, boolean[] rowsWithZero){
    for (int i = 0; i < rowsWithZero.length; i++) {
        if (rowsWithZero[i]) {
            transformRow(i, matrix);
        }
    }
}


Graceful way to find combinations in JavaScript I'm looking for combinations, not permutations, and have found some really great examples ranging from using flatMap such as https://stackoverflow.com/a/54329187/669843

The result of ['a', 'b', 'c'] would be an array of arrays of 3 (a, b, and c). The result I'm looking for would also contain arrays of length 1 and 2.

Example:

['a']
['b']
['c']
['a', 'b']
['a', 'b', 'c']
['a', 'c']
...

I'm accomplishing the results with something like this but this seems extraordinarily awful and requires de-duplication.

for(let i = 0; tci < combo.length; tci++) {
  let newcombo = [allitems[item]].concat(combo.slice(tci, combo.length));
  ...
  push newcombo;

You can write this better.  The best way to explain it is to see the code example in Python's itertools module:

def combinations(iterable, r):
    # combinations('ABCD', 2) --> AB AC AD BC BD CD
    # combinations(range(4), 3) --> 012 013 023 123
    pool = tuple(iterable)
    n = len(pool)
    if r > n:
        return
    indices = list(range(r))
    yield tuple(pool[i] for i in indices)
    while True:
        for i in reversed(range(r)):
            if indices[i] != i + n - r:
                break
        else:
            return
        indices[i] += 1
        for j in range(i+1, r):
            indices[j] = indices[j-1] + 1
        yield tuple(pool[i] for i in indices)

This generates the combinations of length 'r' efficiently using counting.  You can create all combinations by either adding length-1 'nulls' to the pool or just iterating over the lengths.

Combination generation is a bit hard to grok, and really you should step through this example until you get it.  It's a variation on the idea of manually counting used throughout many algorithms.

The base idea for combinations('ABCD', 2) is to start with 'AB' (first yield), increment the last letter 'AC' then 'AD' with the for j loop doing nothing, on the next loop i will be the index 0, so 'A' -> 'B' and the second resets to one more than the first character and you yield 'BC'.

Trace the code.  Keep hacking!  Keep notes.


Print a square graph and dot based on co-ordinates I've written a simple script, that takes a 0, 0 style co-ordinate input, following the pattern of:
$$0 \le n \le 9$$
Tacked onto the end of the output program, is a simple script that runs through all iterations of 0,0 - 9, 9.
Below is a gif of that.

Note: This code DOES not map multiple, I am not asking how you can map multiple, I am asking for suggestions on my working code.

import os
import time

def plot_point(x, y):
    os.system('cls') if os.name == 'nt' else os.system('clear')
    print 'Printing', x, y # Turned on for single use, off for mass use
    row_with_entry = ''
    rows = []
    for i in range(x):
        rows.append([i, '|_' * 10 + '|'])

    for i in range(y):
        row_with_entry += '|_'

    row_with_entry += '|*|'

    for i in range(y, 9):
        row_with_entry += '_|'

    rows.append([x, row_with_entry])

    for i in range(int(x), 9):
        rows.append([i, '|_' * 10 + '|'])

    for i in rows:
        print i[1]

    time.sleep(.05) # Just for purpose of demonstration

for i in range(9, 0, -1):
    for k in range(9, 0, -1):
        if i == 1:
            if k == 1:
                plot_point(i, k)
                plot_point(i, k - 1)
                for n in range(9, 0, -1):
                    plot_point(0, n - 1)
                plot_point(0, 0)
            else:
                plot_point(i, k)
        else:
            plot_point(i, k)
            plot_point(i, k - 1)

I only looked at the plot_point function, but I would suggest using an alternative approach - first create a board full of _ and then set the correct cell to a * and finally print out the whole board. This is much shorter and more "pythonic" than iterating like you are currently doing (which is rather C like).

def plot_point(x, y):
    os.system('cls') if os.name == 'nt' else os.system('clear')
    print 'Printing', x, y # Turned on for single use, off for mass use

    # Create a board of 10 x 10 underscores
    board = [['_' for col in range(10)] for row in range(10)]

    # Set the appropriate cell to a *
    board[x][y] = '*'

    # Create a string with a | seperating each element for each row
    # Print out all these rows with newlines in between
    print '\n'.join(['|' + '|'.join(row) + '|' for row in board])

    time.sleep(.05) # Just for purpose of demonstration

We use list comprehensions to make our code a lot shorter (and still very readable). The one-line print statement may be considered a little bit hard to read, in which case you can consider splitting it up a bit.

for row in board:
    print '|' + '|'.join(row) + '|'

But I think that the comprehension is fine.


Tab 2 spaces convertor (for markdown) This program converts glob files' tabs to spaces.
There is a special case to ignore content inside triple back ticks.

Example usage is ./tab_2_spaces.py *.md

Main Concern: Can I write the while loop better?

#!/usr/bin/env python3
"""
This program is used to convert markdown file's tab indentations to 2 spaces.
"""

import argparse
from pathlib import Path
import os
import io
import itertools
from multiprocessing import Pool

_TAB_2_SPACE = "  "

def fix_indent(filename):
    with io.open(filename, mode="r", encoding="utf-8") as h:
        text = h.read()

    lines = text.splitlines(True)
    c = len(lines)
    i = 0

    # Replace tab to spaces
    while i < c:
        line = lines[i]

        if line.startswith("```"):
            i += 1
            while i < c:
                if lines[i].startswith("```"):
                    break
                i += 1
            else:
                continue
            break
        elif line.startswith("\t"):
            lines[i] = line.replace("\t", _TAB_2_SPACE)

        i += 1

    with io.open(filename, mode="w", encoding="utf-8") as h:
        h.write("".join(lines))

def main():
    parser = argparse.ArgumentParser(description="Fix indent")
    parser.add_argument(
        "files", metavar="N", type=str, nargs="+", help="List of files to scan"
    )

    args = parser.parse_args()
    all_files = tuple(
        itertools.chain(
            *[
                (filename for filename in Path().glob("**/" + file_))
                for file_ in args.files
            ]
        )
    )

    with Pool(2) as process_pool:
        process_pool.map(fix_indent, all_files)

if __name__ == "__main__":
    main()

A lot of the code in your while loop can be replaced with a for loop using enumerate() and a toggled boolean.

You should be able to replace your code from c = len(lines) (line 21) down through your entire while loop with:

skip = False
for index, line in enumerate(lines):
    if line.startswith("```"):
        skip = not skip
    else:
        if not skip:
            if line.startswith("\t"):
                lines[index] = line.replace("\t", _TAB_2_SPACE)

I can't test this right now, so please let me know if you run in to any problems with this implementation.


Handling events in a multi threaded environment I have a service that raises multiple events, some of them can be raised at the same time. I need to process those events and run a potentially long running method based on the event arguments.
What I did is to create a BlockingCollection<T> that will store the events and a Task that will keep taking one event at a time until it will be signaled to stop using a CancellationTokenSource.
This is my code:

public class EventsTest
{
    //private fields
    private BlockingCollection<int> _queue; //a concurrent collection to hold incoming events
    private CancellationTokenSource _tokenSource;
    private IoService _ioService; //a service that raises multiple events
    private static EventWaitHandle _eventWaiter; // used for waiting inside a method

    public EventsTest()
    {
        _queue = new BlockingCollection<int>();
        _tokenSource = new CancellationTokenSource();
        _eventWaiter = new EventWaitHandle(false, EventResetMode.AutoReset);

        _ioService = new IoService();
        _ioService.IoEvent += _ioService_IoEvent;

        //Start Listening to IO events
        var t = Task.Factory.StartNew(StartListening, _tokenSource, TaskCreationOptions.LongRunning);
    }

    //IO events listener
    private void _ioService_IoEvent(byte[] desc, int portNum)
    {
        //add events to a blocking collection
        _queue.Add(portNum);
    }

    //a while loop running on a separate task
    //and taking the event one by one
    private void StartListening(object dummy)
    {
        while (!_tokenSource.IsCancellationRequested)
        {
            try
            {
                var eve = _queue.Take(_tokenSource.Token);
                switch (eve)
                {
                    case 0:
                        //run the required method on a separate task so it won't block the loop
                        Task.Factory.StartNew(LongRunningMethod);
                        break;
                    case 1:
                        //invoke a potential blocking method
                        break;
                    default:
                        break;
                }
            }
            catch (OperationCanceledException)
            {
                // _tokenSource was signaled to stop. do nothing
            }
        }
    }

    static readonly object _locker = new object();
    //sample long running method
    private void LongRunningMethod()
    {
        lock (_locker)
        {
            Thread.Sleep(5000);
        }
    }
}

Am I on the right track here? what can I do in order to make it more robust?
UPDATE
The process of this class is as follows:

Listen to events raised by a remote service
Handle each event based on it's event arguments in a predefined method
A method execution time can be long
Different methods can run simultaneously (That's why I start each method with the Task.Factory.StartNew(LongRunningMethod);)
If an event handling method needs to run again when the previous one wasn't finished it should wait for it (that's why I placed the lock statement in the method implementation)

please note that I originally posted this question here but I guess that it's more suitable for this site

The method StartListening takes one argument object dummy. It is not used in the method and from its name I conclude that it is not intended to be used. It is possible to declare the method with no arguments and call a variant of Task.Factory.StartNew that takes a parameter of type Action and TaskCreationOptions to start the new task.
The field private static EventWaitHandle _eventWaiter is only initialized in the constructor and never used. Its purpose is unclear from the code.


MASM) assembly implementation of recursive Fibonacci I'm trying to teach myself assembly via MASM, below is my first implementation of the recursive form of the fibonacci sequence. I timed it against an equivalent C implementation in release mode and the C implementation was much better optimized. What can I do better? I haven't gotten the hang of how the various side effects of instructions impact the state of registers or flags yet, or, well, most things in assembly.
Assembly:
;extern "C" unsigned long fibonacci_asm_canonical(unsigned long number)
fibonacci_asm_canonical proc number: dword
    LOCAL fib_lhs:DWORD
    mov eax, number ; store number parameter in eax register (also used as return value).
    .IF eax > 1
        ; recursively call this function, retrieving the value for the left-hand side.
        dec eax
        mov edx, eax
        invoke fibonacci_asm_canonical, edx
        mov fib_lhs, eax ; store the result of fibonacci_asm_canonical(number - 1) into fib_lhs.
        mov eax, number ; store number parameter in eax register to set up right-hand side of addition.
        ; recursively call this function, retrieving the value for the right-hand side.
        sub eax, 2
        invoke fibonacci_asm_canonical, eax
        ;eax now contains result of fibonacci_asm_canonical(number - 2), following the invocation,
        ;so add it with the result of fibonacci_asm_canonical(number - 1) which is in fib_lhs.
        add eax, fib_lhs
    .ENDIF
    ret
fibonacci_asm_canonical endp

C version:
unsigned long fib_canonical(unsigned long number)
{
    if (number <= 1)
        return number;
    return fib_canonical(number - 1) + fib_canonical(number - 2);
}

```

dec eax
mov edx, eax

This you can write as the single instruction lea edx, [eax-1], but your code doesn't need to use the extra register EDX at all. The single dec eax is enough.
If you really want to optimize code then don't use constructs like .IF eax > 1 .ENDIF (and many others) for which you don't necessarily know how they get encoded.
invoke, proc, local, and endp will very probably entail that prologue and epilogue codes are inserted. This is fine for the interfacing with the main program but is demanding on the recursion.
push ebp        ; Prologue
mov  ebp, esp
sub  esp, 4     ; Local
...
mov  esp, ebp   ; Epilogue
pop  ebp
ret  4          ; Parameter removal

With a separate recursive routine DoFib, you can:

avoid using the above and rely on the faster [esp] relative addressing.
reuse the argument from the first recursive call. All it takes is a second dec on the already decremented value.
pass the single dword argument via the EAX register instead of on the stack.

;extern "C" unsigned long fibonacci_asm_canonical(unsigned long number)
fibonacci_asm_canonical proc number: dword
    mov  eax, number
    call DoFib           ; -> EAX
    ret
fibonacci_asm_canonical endp

; IN (eax) OUT (eax)
DoFib:
    cmp  eax, 1
    jbe  .done
    sub  esp, 8         ; Room for local storage
    dec  eax
    mov  [esp], eax     ; (number - 1)
    call DoFib          ; -> EAX
    mov  [esp+4], eax   ; Store lhs result
    mov  eax, [esp]     ; (number - 1)
    dec  eax            ; (number - 2)
    call DoFib          ; -> EAX
    add  eax, [esp+4]   ; rhs + lhs
    add  esp, 8
  .done:
    ret


Another even Fibonacci numbers implementation This is intended as something of a comparative study. I'm including not just one, but two separate implementations of code to implement the Project Euler problem to sum the even Fibonacci numbers up to 4,000,000. The first is a very C-like implementation. The second attempts to make much more use of modern C++. To do so, it includes a specialized iterator to generate Fibonacci numbers, and a generic algorithm for summing items from a range conditionally (and uses a lambda expression to specify the condition).

If we include that generic algorithm, the latter is (marginally) longer than the former (though I've also included some timing code in the former that's currently absent in the latter). I'm curious to know what people think of them--whether the latter is really an improvement, which style you'd prefer to see in your code base, and so on.

More specific questions:

Do iterators and algorithms produce a net loss or gain for this particular code?
Is it problematic that the iterator's operator != actually does a < comparison internally, on the basis that it's looking for the end of a logical range, but doesn't expect you to necessarily know the exact value of the Fibonacci number at the end of the range you care about?

Version 1:

#include <iostream>
#include <time.h>

int main() {
    clock_t start = clock();
    unsigned long long total;
    int max_reps = 10000;

    for (int reps = 0; reps < max_reps; reps++) {
        total = 0;
        unsigned long long first = 1;
        unsigned long long second = 1;
        unsigned long long fib = first + second;

        while (fib < 4000000) {
            if ((fib % 2) == 0)
                total += fib;
            first = second;
            second = fib;
            fib = first + second;
        }
    }
    clock_t stop = clock();

    std::cout << total
              << "\ntime: "
              << (1000.0 * (stop - start)) / double(CLOCKS_PER_SEC * max_reps) << "ms\n";
}

...and version 2:

#include <iostream>

template <class T=unsigned>
class fib_iterator {
    T first, second;
public:
    fib_iterator(T v1=1, T v2 = 1) : first(v1), second(v2) {}
    int operator *() { return first + second; }
    fib_iterator &operator++() {
        int temp = first + second;
        first = second;
        second = temp;
        return *this;
    }
    bool operator!=(fib_iterator const &r) { return first < r.first; }
};

template <class InIt, class F, class T>
T accumulate_if(InIt b, InIt e, T accum, F f) {
    while (b != e) {
        if (f(*b))
            accum += *b;
        ++b;
    }
    return accum;
}

int main() {
    std::cout << accumulate_if(fib_iterator<>(), fib_iterator<>(4000000),
        0U, [](unsigned v) { return v % 2 == 0; });
}

Of course, the questions I've asked above are not intended to limit the scope of reviews--only to add to the normal review I'd hope for when posting any code here.

Sorry if I sound Fortranish. Project Euler is about math, not programming.

A Fibonacci number is even if its index is \$3n+2\$. A Binet's formula reduces their sum to the sum of 2 geometric progressions. That, and some error estimation, is pretty much it.


Generated code proxy In my library Transfuse I use a code generator to build a handful of classes that look up resources.  Each of these look up classes are proxied by a static utility class so they may be referenced before the generated class is built.  For instance, the Parcels class can be used to wrap an object with a generated Parcelable class:

ExampleParcel parcel = new ExampleParcel();
Parcelable parcelable = Parcels.wrap(parcel);
// ... load into bundle as extra, etc

Parcels proxies a generated class called Transfuse$Parcels.

Parcels:

public final class Parcels {

    public static final String PARCELS_NAME = "Parcels";
    public static final String PARCELS_REPOSITORY_NAME = "Transfuse$Parcels";
    public static final String PARCELS_PACKAGE = "org.androidtransfuse";

    private static ParcelRepository instance;

    static{
        try{
            Class injectorClass = Class.forName(PARCELS_PACKAGE + "." + PARCELS_REPOSITORY_NAME);
            instance = (ParcelRepository) injectorClass.newInstance();
        } catch (ClassNotFoundException e) {
            instance = null;
        } catch (InstantiationException e) {
            throw new TransfuseRuntimeException("Unable to instantiate generated ParcelRepository", e);
        } catch (IllegalAccessException e) {
            throw new TransfuseRuntimeException("Unable to access generated ParcelRepository", e);
        }
    }

    private Parcels(){
        // private utility class constructor
    }

    public static Parcelable wrap(Object input) {
        if(instance == null){
            throw new TransfuseRuntimeException("Unable to find " + PARCELS_REPOSITORY_NAME + " class");
        }
        return instance.wrap(input);
    }
}

Transfuse$Parcels:

@Generated(value = "org.androidtransfuse.TransfuseAnnotationProcessor", date = "01/12/2013 16:56:02 MST")
public class Transfuse$Parcels
    implements ParcelRepository
{

    private final Map<Class, ParcelableFactory> parcelWrappers = new HashMap<Class, ParcelableFactory>();

    public Transfuse$Parcels() {
        parcelWrappers.put(...);
    }

    @Override
    public Parcelable wrap(Object input) {
        return parcelWrappers.get(input.getClass()).buildParcelable(input);
    }
    // Define ParcelableFactory classes...
}

Is this technique of loading the generated class using the static initialization block optimal?  As you can see, if the class is not found (not generated possibly) then the ParcelRepository instance ends up being null and wrap() would always return null.  Should wrap() throw a runtime exception if an instance is not found instead?

If you take a look at the library, the code above is a simplification for example purposes: Parcels

I ended up taking a bit of a different route with this problem.  It seems I was not adding additional proxied resources if they were not contained within the current compiled package... as in, they were included in included libraries.  So, my proxy utility looks like the following:

public abstract class GeneratedCodeRepository<T> {

    private ConcurrentMap<Class, T> generatedMap = new ConcurrentHashMap<Class, T>();

    public GeneratedCodeRepository(String repositoryPackage, String repositoryName) {
        loadRepository(getClass().getClassLoader(), repositoryPackage, repositoryName);
    }

    public T get(Class clazz){
        T result = generatedMap.get(clazz);
        if (result == null) {
            T value = findClass(clazz);
            if(value == null){
                return null;
            }
            result = generatedMap.putIfAbsent(clazz, value);
            if (result == null) {
                result = value;
            }
        }

        return result;
    }

    public abstract T findClass(Class clazz);

    /**
     * Update the repository class from the given classloader.  If the given repository class cannot be instantiated
     * then this method will throw a TransfuseRuntimeException.
     *
     * @throws TransfuseRuntimeException
     * @param classLoader
     */
    public final void loadRepository(ClassLoader classLoader, String repositoryPackage, String repositoryName){
        try{
            Class repositoryClass = classLoader.loadClass(repositoryPackage + "." + repositoryName);
            Repository<T> instance = (Repository<T>) repositoryClass.newInstance();
            generatedMap.putAll(instance.get());

        } catch (ClassNotFoundException e) {
            //nothing
        } catch (InstantiationException e) {
            throw new TransfuseRuntimeException("Unable to instantiate generated Repository", e);
        } catch (IllegalAccessException e) {
            throw new TransfuseRuntimeException("Unable to access generated Repository", e);
        }
    }
}

findClass() then can be defined to lookup the given repository class using reflection.


Reduce JavaScript code. Double event, selector into one Any idea how to write this code better."the html is nested tabs"
Two selectors and two similar events, in a function would be better or a pattern to reduce lines. eg .jsbin

$(function() {
  var $items  = $('#vtab>ul>li'),
      $items2 = $('#vtab2>ul>li');

  $items.mouseover(function() {
    var index = $items.index($(this));
    $items.removeClass('selected');
    $(this).addClass('selected');
    $('#vtab>div').hide().eq(index).show();
  }).eq(1).mouseover();

  $items2.mouseover(function() {
    var index = $items2.index($(this));
    $items2.removeClass('selected');
    $(this).addClass('selected');
    $('#vtab2>div').hide().eq(index).show();
  }).eq(1).mouseover();

});

Give those #vtabs a class and it should be simple:

$(function() {
    $('.vtab > ul > li').mouseover(function() {
        var $this = $(this);

        $this.addClass('selected').siblings().removeClass('selected');
        $this.closest('.vtab').find('div').eq($this.index()).hide().siblings().show();
    }).each(function() {
        $(this).find('> ul > li').eq(1).mouseover();
    });
});

Although I'd try turning it into a plugin at this point.


Using C# generics and factory classes to map IDataReader to POCO? I'm in the process of doing some mapping of POCOs via IDataReader. I'd love to hear thoughts on the approach and if they are any good ways to improve what I have done.

public class Code
{
    public int Id { get; set; }
    public string Type { get; set; }
    public string Value { get; set; }
    public string Description { get; set; }
}

public class CodeRepository : GenericRepository
{
    public CodeRepository(IDbConnection connection) : base(connection) { }

    public IEnumerable<Code> GetCodesForDemographics()
    {
        var sql = @"
            Select
                CODE_TYPE,
                CODE,
                DESCRIPTION
            From
                Codes
            Where
                CODE_TYPE In ('language', 'lang_service', 'transfer', 'admit_type', 'Value',
                                'admit_src', 'state', 'county', 'ethnic', 'race',
                                'exempt_unit_code', 'alt_care_type_code')";

        using (var manager = new DbCommandManager(this.Connection, sql))
        {
            using (var reader = manager.GetReader())
                return DbAutoMapper.MapReaderToList<Code, CodeFactory>(reader);
        }

    }

public class CodeFactory : IFactory<Code>
{
    public Code CreateTFromReader(IDataReader reader)
    {
        try
        {
            var code = new Code();
            code.Type = (reader["CODE_TYPE"] as string) ?? String.Empty;
            code.Description = (reader["DESCRIPTION"] as string) ?? String.Empty;
            code.Value = (reader["CODE"] as string) ?? String.Empty;
            return code;
        }
        catch (Exception)
        {
            throw new Exception("Error creating Code objects from reader.");
        }
    }
}

Most of it looks good.  That is until I get to your catch statement:

catch (Exception)
{
    throw new Exception("Error creating Code objects from reader.");
}

really does not explain why you are having.  I would suggest either creating your own exception i.e. InvalidCodeDataException, or throwing an ApplicationException.  Either of these should take the original exception as an inner exception:

catch (Exception ex)
{
    throw new InvalidCodeDataException("Error creating Code objects from reader.", ex);
}

This way, you do not lose what cause the original exception.

Two suggestions:

don't use the this. statement, I find it overly clutters code up if
you are using standard naming conventions, which you are.
Use the object initializer format when creating objects:

return new Code
    {
         Type = (reader["CODE_TYPE"] as string) ?? String.Empty,
         Description = (reader["DESCRIPTION"] as string) ?? String.Empty,
         Value = (reader["CODE"] as string) ?? String.Empty
    }


Implementing RSA in C++ I am implementing RSA algorithm in C++ and here's my design(code structure).

keygen.h

namespace rsa{
    class keygen{
        public:
        //Constructor
        keygen(size);
        //Generate keys
        void generate();
        //Getters
        char* gete(){ return xyz; }
        ..
        ..
        ..

        private:
        //initializes bignums
        void initall();
        //Private Member variables goes here
   }
}

prime.h

namespace rsa{
    //First 100 primes
    unsigned int primes[]={2,3,5,7,11.....541};

    //MillarRabin Primality
    bool isPrime(mpz_t, unsigned short);
    //Get Random Prime
    void getPrime(mpz_t, mpz_t)
}

endec.h

namespace rsa{
    //Encryption
    const char* encryption(const char* text, const char* n, const char* e);
    //Decryption
    const char* decryption(const char* cipher, const char* n, const char* d);
}

Is this a good design? How can I make it better? I want to improve the structure or the overall design, that's why dint post any code. Things like naming standards, using classes wherever applicable, standard function signature and similar is what I'm looking for.

Just a couple of short comments as a starting point.

First of all, you don't need separate encrypt and decrypt functions -- the algorithm is identical for both. You just pass different parameters for them.

Second, I wouldn't use char const * for the input or output of the encrypt/decrypt function. It might be barely allowable for the plaintext (since you could restrict that to not contain zero bytes) but is definitely not acceptable as the encrypted data (since you can't stop it from containing zero bytes). Since, however, the same function can do either encryption or decryption, you don't want to use char const * for input or output. Personally, I'd probably have it take iterators for the input and output, so you can work with string, vector<char>, vector<wchar_t>, etc.

Finally, I don't think I'd put implementation details like the first 100 primes directly in the rsa namespace -- I'd have rsa contain a detail namespace to contain details like that. I'm not sure I'd have keygen either -- I think I'd make key a class, probably with the default ctor deleted, and a static member function to create a key, and overloading operator>> and operator<< to serialize keys in a standard format (e.g., X.509, PKCS #8). About all that would be visible to the outside world would be key and crypt (or whatever you decide to call the combined encrypt/decrypt funct(ion|or).

Edit: the key class and signatures would be something like this:

class key {
    key() = delete; // `=delete;` is C++11 only; without that just declare private
public:
    static key create_key();
};


Find duplicate files in Linux This code finds duplicate files in Linux.
I assume that MD5 hash will return unique hash number.
What can I improve in this code?

#!/usr/bin/python;

import sys
import subprocess
import os
import hashlib

FIND_CMD = "find . -name \*.* -print"
BUFFER = 65536

#Get path from the user
root_path = sys.argv[1]
#Checks that the user sent path
if not root_path:
 print("Error: No file specified.Please try again ")
 sys.exit(1)
#Chage working path
os.chdir(root_path)
#Get all the possible paths under the given directory
dirty_out = subprocess.run(FIND_CMD.split(" "), stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)
file_list = dirty_out.stdout.decode("utf-8").splitlines()
#Find files with the same size and add to dic
same_size = dict()
for file in file_list:
   path = root_path + file[1:]
   if os.path.isdir(path):
        continue
   if os.path.exists(path):
        file_size = os.path.getsize(path)
        #Add the path to dictionary by size
        if file_size not in same_size:
         same_size[file_size] = list([path])
        else:
         same_size[file_size].append(path)

#Find files with the same size and hash code
file_signatures = dict()
for size in same_size:
 if len (same_size[size]) > 1:
    i = 0
    while i < len (same_size[size]):
     # Hash file content with read buffer
     md5 = hashlib.md5()
     path = same_size[size][i]
     md5 = hashlib.md5()
     with open(path, 'rb') as f:
            while True:
                data = f.read(BUFFER)
                if not data:
                    break
                md5.update(data)
            md5_sig = md5.hexdigest()
            # Add to dictionary only files with the same size and hash
            if md5_sig not in file_signatures:
                file_signatures[md5_sig] = list([path])
            else:
                file_signatures[md5_sig].append(path)
     i=i+1
#Prints the path of all the duplicate files separated with ,
for sig in file_signatures:
    if len(file_signatures[sig]) > 1:
        print("{0}\n".format(",".join(file_signatures[sig])))

Please be careful with the indentation because it looks like there are different number of spaces for different indentation depths and it looks very weird.

Minor improvements:

Repetition

 md5 = hashlib.md5()
 path = same_size[size][i]
 md5 = hashlib.md5()

I am almost sure there is no reason to repeat md5 = hashlib.md5() twice.

"Native" iteration

while i < len (same_size[size]):
 # Hash file content with read buffer
 md5 = hashlib.md5()
 path = same_size[size][i]
 ...
 i=i+1

You are iterating over all the files with the given size:

for path in same_size[size]:
   ...

In general in Python you can use for x in list_ to iterate over all items in any collection.

Zero length automatically skipped

if len (same_size[size]) > 1:

If you ask python to iterate over a zero length collection nothing happens, so you can remove this line as it does nothing but increases indentation and line count.

edit: No @miracle173 correctly noted that this is an efficiency gain to skip files of which there is only one of a certain size so it cannot be skipped.

Default dict to simplify logic

    if file_size not in same_size:
     same_size[file_size] = list([path])
    else:
     same_size[file_size].append(path)

Not tested, but using a defaultdict you can avoid the branch and only use append.

Same size?

Are you sure you need to calculate also the size in order to check if two files are equal? Isn't the hash enough?


CodeChef's Tree MEX (Minimum Excludant) challenge This code is a solution for CodeChef's Tree MEX problem:

  Minimum excludant (or MEX for short) of a collection of integers is
  the smallest non-negative integer not present in the set.

  You have a tree \$ T \$ with \$n\$ vertices. Consider an ordering \$ P=(v_1,\ldots,v_n) \$
  of vertices of \$T\$. We construct a sequence \$A(P)=(a_1,\ldots,a_n)\$ using the
  following process:

  Set all \$a_i=−1\$.
  Process vertices in order \$v_1,\ldots,v_n\$. For the current vertex \$v_i \$ set  \$a_i=\operatorname{MEX}(a_{u_1},\ldots,a_{u_k})\$, where \$u_1,\ldots,u_k\$ is the set of neighbours of
  \$v_i\$.

  For instance, let  \$n=3\$ and \$T\$ be the tree with edges \$(1,2)\$ and
  \$(2,3)\$. Then,  for the ordering \$P=(1,2,3)\$ we obtain the sequence
  \$A(P)=(0,1,0)\$, while    for the ordering \$P=(2,3,1)\$ we obtain
  \$A(P)=(1,0,1)\$.

  Consider all \$n!\$ orders \$P\$. How many different sequences
  \$A(P)\$ can we obtain? Print the answer modulo \$10^9+7\$.

I have created a graph from list of tuples and then calculated mex values for each permutation.

Ex: 3    # 3 vertices
    1 2  # (1,2) edge
    2 3  # (2,3) edge

permutations(1,2,3) ==> we get 6 combinations

For 6 combinations we will get 6 values. I need to print distinct count of that 6 values.

Code:

# https://www.codechef.com/problems/TREEMX

from collections import defaultdict
from itertools import permutations

class Graph:
    """ Graph is data structure(directed). """

    def __init__(self, connections):
        """ Initializing graph with set as default value. """
        self._graph = defaultdict(set)
        self.add_connections(connections)

    def add_connections(self, connections):
        """ Add connections(list of tupules) to graph. """
        for node1, node2 in connections:
            self.add(node1,node2)

    def add(self, node1, node2):
        """ Add node1 and node2 to graph which is initialized with set by default. """
        self._graph[node1].add(node2)
        self._graph[node2].add(node1)

    def get_graph(self):
        return dict(self._graph)

def mex(arr_set):
    mex = 0
    while mex in arr_set:
        mex+=1
    return mex

def process(graph, order):
    a_p = [-1] * len(order)
    for el in order:
        a_p[el-1] = mex([a_p[u-1] for u in graph[el]])
    return a_p

t = int(input())
for _ in range(t):
    v = int(input())
    e = []
    for i in range(v-1):
        e.append(tuple(map(int, input().split())))

    g = Graph(e)
    all_vertices = {s for i in e for s in i}
    result = []
    for p in permutations(all_vertices, v):
        out = process(g.get_graph(), p)
        result.append(out) if out not in result else None

    print(len(result) % ((10**9)+7))

Constraints:

\$1≤T≤10\$
\$1≤n≤10^5\$
\$1≤u_i,v_i≤n\$
\$u_i≠v_i\$

How can I optimize the code, to get rid of the "time limit exceeded" error?

for p in permutations(all_vertices, v):

  \$1≤n≤10^5\$

Well, \$(10^5)! \approx \left(\frac{10^5}{e}\right)^{10^5} \approx 10^{35657}\$ so it's a waste of time trying to optimise this. The only thing to do is go back to the drawing board and spend a few hours thinking about the mathematics. At best the code you've written will serve to analyse all trees up to a small number of vertices (maybe 7 or 8) to see what patterns you can spot which can help to guide you in the right direction.


Providing factory method to Lazy when factory exists on another lazy instance? The Problem

I found myself needing an instance of an IObjectContext interface that should never be null, but wanting to delay instantiation until after some required resources have been loaded.

I decided to approach this with Lazy<T>, but quickly ran into an issue -- I also require an IObjectSet<TEntity> that is never null and lazy instantiated. The problem is this:

// lazyObjectSet needs to be created directly after lazyContext, but I need to
// delay instantiating both. The desired factory method exists on lazyContext,
// but accessing lazyContext.Value instantiates context before it is needed:
//
var lazyObjectSet = new Lazy<IObjectSet<TEntity>>(lazyContext.Value.FactoryMethodName);

My Solution

I would like suggestions for improvements on my approach, or alternative approaches to the problem.

public static class LazyExtensions {
    private class LazyFactory<T, TResult> where TResult : class {
        private readonly Lazy<T> _dependency;
        private readonly MethodInfo _factory;

        public LazyFactory(Lazy<T> lazyDependency, MethodInfo lazyFactory) {
            // Argument null checks omitted for brevity
            Contract.Requires(typeof(TResult).Equals(lazyFactory.ReturnType));
            Contract.Requires(lazyFactory.GetParameters().Count() == 0);

            _factory = lazyFactory;
            _dependency = lazyDependency;
        }

        public TResult Invoke() {
            return (TResult) _factory.Invoke(_dependency.Value, null);
        }
    }

    public static Func<TOut> GetLazyFactory<TIn, TOut>(
        Lazy<TIn> dependency, MethodInfo factoryInfo) where TOut : class {
        return new LazyFactory<TIn, TOut>(dependency, factoryInfo).Invoke;
    }
}

Now I can use GetLazyFactory() to write the following, and the context should not be instantiated until lazyObjectSet.Value is accessed:

var lazyFactory = LazyExtensions.GetLazyFactory<IObjectContext, IObjectSet<TEntity>>(
    new Lazy<IObjectContext>(factory.Create),
    typeof (IObjectContext).GetMethod("ObjectSet"));

var lazyObjectSet = new Lazy<IObjectSet<TEntity>>(lazyFactory);

How can this be improved, and is there a better way of doing this?

In conjunction with the rest of the code, the following line exhibits a bug:

Contract.Requires(typeof(TResult).Equals(lazyFactory.ReturnType));

The problem is that my IObjectContext.ObjectSet() method has the generic return type IObjectSet<TEntity>, and lazyFactory doesn't have type information for TEntity.

Specifically, the type parameter for lazyFactory is unknown because it's obtained using GetMethod(). As a result, Equals() returns false and the constraint fails with an exception.

I solved the problem by modifying GetLazyFactory to accept a method name string and obtain the MethodInfo itself, calling MakeGenericMethod() as necessary.

public static Func<TOut> GetLazyFactory<TIn, TOut>(Lazy<TIn> dependency, string factoryName)
    where TOut : class
{
    Type outT = typeof(TOut);       // Expected return type of factory method
    MethodInfo factoryMethodInfo =  // Should return TOut
        typeof(TIn).GetMethod(factoryName);

    if(outT.IsGenericType)
    {
        // Get generic type argument of TOut, making sure exactly 1 exists.
        Type tParam = outT.GetSingleGenericArgument();

        // We must specify the generic type or Type.Equals() will yield false.
        factoryMethodInfo = factoryMethodInfo.MakeGenericMethod(tParam);
    }
    return new LazyFactory<TIn, TOut>(dependency, factoryMethodInfo).Invoke;
}


Retro Rocket ASCII Art Inspired by Nested for-loop ASCII art, I looked at building a more complicated ASCII art than just the normal 'diamond' and 'triangle' variations that crop up occasionally here.

The challenge: Produce the following rocket

     /**\
    //**\\
   ///**\\\
  ////**\\\\
 /////**\\\\\
+=*=*=*=*=*=*+
|../\..../\..|
|./\/\../\/\.|
|/\/\/\/\/\/\|
|\/\/\/\/\/\/|
|.\/\/..\/\/.|
|..\/....\/..|
+=*=*=*=*=*=*+
|\/\/\/\/\/\/|
|.\/\/..\/\/.|
|..\/....\/..|
|../\..../\..|
|./\/\../\/\.|
|/\/\/\/\/\/\|
+=*=*=*=*=*=*+
     /**\
    //**\\
   ///**\\\
  ////**\\\\
 /////**\\\\\

The question on Stack Overflow has issues, and those issues got answered before I could put a response together, but, the problem intrigued me, and I built the following code to solve it in a more compact way, using loops as much as possible, and parameterizing almost all aspects of the problem.

public class Rocket {

    private enum Direction {
        FORWARD, BACKWARD
    }

    private static final char NONE = (char) 0;

    private static final String buildSymmetrical(
            char[] chars, char mid, int from, int len) {

        StringBuilder sb = new StringBuilder(len * 2 + 2);
        for (int i = 0; i < len; i++) {
            sb.append(chars[from + i]);
        }
        if (mid != NONE) {
            sb.append(mid).append(mid);
        }
        for (int i = 0; i < len; i++) {
            sb.append(chars[chars.length - from - len + i]);
        }
        return sb.toString();
    }

    private static final String buildLines(
            char bracket, char[] source, int len, char midchar, int charfrom,
            Direction chardir, int linecount, int duplicates) {

        StringBuilder sb = new StringBuilder();
        for (int line = 0; line < linecount; line++) {
            sb.append(bracket);
            int pos = charfrom + line * (chardir == Direction.FORWARD ? 1 : -1);
            String block = buildSymmetrical(source, midchar, pos, len);
            for (int d = 0; d < duplicates; d++) {
                sb.append(block);
            }
            sb.append(bracket).append('\n');
        }
        return sb.toString();
    }

    public static final String buildRocket() {
        char[] wider = "../\\/\\/\\/\\..".toCharArray();
        char[] narrower = "..\\/\\/\\/\\/..".toCharArray();
        char[] cone = "    //////\\\\\\\\\\\\    ".toCharArray();
        char[] boundary = "+=*=*=*=*=*=*+\n".toCharArray();

        StringBuilder sb = new StringBuilder();
        sb.append(buildLines(' ', cone, 5, '*', 0, Direction.FORWARD, 5, 1));
        sb.append(boundary);
        sb.append(buildLines('|', wider, 3, NONE, 0, Direction.FORWARD, 3, 2));
        sb.append(buildLines('|', narrower, 3, NONE, 2, Direction.BACKWARD, 3, 2));
        sb.append(boundary);
        sb.append(buildLines('|', narrower, 3, NONE, 2, Direction.BACKWARD, 3, 2));
        sb.append(buildLines('|', wider, 3, NONE, 0, Direction.FORWARD, 3, 2));
        sb.append(boundary);
        sb.append(buildLines(' ', cone, 5, '*', 0, Direction.FORWARD, 5, 1));
        return sb.toString();
    }

    public static void main(String[] args) {
        System.out.println(buildRocket());
    }

}

My particular concerns are the number of parameters I have going to the inner methods. How can those be rationalized? Should they be? What mechanisms can be used to simplify the interfaces?

Wells, for the sake of loop-ifying everything, you may want to consider changing boundary as such:

final char[] boundary = "=*=*=*".toCharArray();
...
sb.append( buildLines( '+', boundary, 3, NONE, 0, Direction.CENTER, 1, 2 ) );

That will create =*=*=*, duplicate it once, and add the + character at both ends.

The ternary operator below looks like a good candidate for adding a helper method into the Direction enum:

int pos = charfrom + line * (chardir == Direction.FORWARD ? 1 : -1)

My version is expanded as such:

private enum Direction {
    CENTER( 0 ), UPWARD( 1 ), DOWNWARD( -1 );

    private final int multiplier;

    Direction( final int multiplier ) {
        this.multiplier = multiplier;
    }

    int getMultiplier() {
        return multiplier;
    }
}

And calculating pos is just charfrom + line * chardir.getMultiplier();

One more minor suggestion: The usage of 5 and 3 seems to be repeated quite often, maybe refer to them as coneSize and blockSize? The main body will look something like:

sb.append( buildLines( ' ', cone, coneSize, '*', 0, Direction.UPWARD, coneSize, 1 ) );
sb.append( buildLines( '+', boundary, blockSize, NONE, 0, Direction.CENTER, 1, 2 ) );
sb.append( buildLines( '|', wider, blockSize, NONE, 0, Direction.UPWARD, blockSize, 2 ) );
sb.append( buildLines( '|', narrower, blockSize, NONE, 2, Direction.DOWNWARD, blockSize, 2 ) );
sb.append( buildLines( '+', boundary, blockSize, NONE, 0, Direction.CENTER, 1, 2 ) );
sb.append( buildLines( '|', narrower, blockSize, NONE, 2, Direction.DOWNWARD, blockSize, 2 ) );
sb.append( buildLines( '|', wider, blockSize, NONE, 0, Direction.UPWARD, blockSize, 2 ) );
sb.append( buildLines( '+', boundary, blockSize, NONE, 0, Direction.CENTER, 1, 2 ) );
sb.append( buildLines( ' ', cone, coneSize, '*', 0, Direction.UPWARD, coneSize, 1 ) );

Due to the fact that our sources are fixed lengths now, I don't see much of an improvement on the method signatures since it's not like the current code can be easily expanded to create rockets of different sizes. Maybe I'll attempt another stab at that when I have some more time...


Inject a list of processes to execute using spring DI I have couple of MessageProcessors. One of the Processor will log the payload. Other Processor will save into an embedded database.

Here is the code I have written so far:

I am using annotation-driven configuration so I have omitted other classes which are not used in this use case.

@Component
public class ProcessorConfiguration {
    @Bean
    @Qualifier("textMp")
    public MessageProcessor textMessageProcessor() {
        return new TextMessageProcessor();
    }

    @Bean
    @Qualifier("employMp")
    public MessageProcessor employMessageProcessor() {
        return new EmployMessageProcessor();
    }

    @Bean
    private List<MessageProcessor> messageProcessorList(@Qualifier("textMp") MessageProcessor textMessageProcessor,
            @Qualifier("employMp") MessageProcessor employMessageProcessor) {
        List<MessageProcessor> list = new ArrayList<>();
        list.add(textMessageProcessor);
        list.add(employMessageProcessor);
        return list;
    }
}

This class is responsible for all the JMS messages the application receives.

public class MessageHandler {
    @Autowired
    private List<MessageProcessor> messageProcessors;

    public void handleMessage(Notification notification) {
        messageProcessors.forEach(processor -> processor.doProcess(notification));
    }
}

public interface MessageProcessor {
    void doProcess(Notification notification);
}

public class TextMessageProcessor implements MessageProcessor {
    private static final Logger logger = Logger.getLogger(TextMessageProcessor.class);
    @Override
    public void doProcess(Notification notification) {
        logger.info("The payload is " + notification.getText());
    }

}

I have created a builder which takes String as input and returns an Employ object.

@Service
public class EmployMessageProcessor implements MessageProcessor {
    @Autowired
    private EmployDao dao;

    @Override
    public void doProcess(Notification notification) {
        Employ employ = EmployBuilder.buildEmploy(notification.getText());
        dao.save(employ);
    }

}

public interface Notification {
    String getText();
}

I think, the way I am injecting the processors can be improved. Please review my code and provide your valuable feedback.

This is a learning exercise for me as well so I hope I can be helpful with my answer.

I think you can loose the Configuration class entirely. Add
@Component/@Service annotation to your TextMessageProcessor. Spring should be
capable of figuring out MessageProcessor List wiring on its own. Have you already tried that?

If you are worried about the order of the beans inside MessageProcessor List then you can use @Order annotation on your Service and/or Component.

@Component
@Order(value=1)
public class TextMessageProcessor implements MessageProcessor {
}

Your Configuration class in its current form is conflicting with Open Closed principle. In other words, whenever you add a new MessageProcessor, your configuration will grow. Imagine that configuration after you have 20 processors.
Spring creates qualifier names automatically as your bean names.
From Spring documentation:

  For a fallback match, the bean name is considered as a default qualifier value.

I recommend doing your wiring through constructors instead of fields  (I am especially eyeballing that DAO of yours). That way you preserve an ability to test your code later on. You will need to inject objects through constructors in your tests.
You could move Logging to MessageProcessor if it was an abstract class. Make a field or a method for Logging which returns the logging instance for current class. (Stackoverflow link)


Unit converter in Python I am new to programming and I am trying to make a simple unit converter in python. I want to convert units within the metric system and metric to imperial and vice-versa. I have started with this code and I found this method is slow and in-efficient, How can I code this more efficiently?

import math
import time
"""Unit Converter"""
#Welcome and variable setting
print ("Welcome to Sam's Unit Converter")
cat = raw_input ("Which category would you like to convert? we support length(l) and Weight(w):  ")
if cat == ("l"):
unit1 = raw_input ("Which unit would you like to convert from: ")
unit2 = raw_input ("Which unit would you like to convert to: ")
num1 = raw_input ("Enter your value: " )

    ##Calculations

if unit1 == "cm" and unit2 == "m":
    ans = float(num1)/100
elif unit1 == "mm" and unit2 == "cm":
    ans = float(num1)/10
elif unit1 == "m" and unit2 == "cm":
    ans = float(num1)*100
elif unit1 == "cm" and unit2 == "mm":
    ans = float(num1)*10
elif unit1 == "mm" and unit2 == "m":
    ans = float(num1)/1000
elif unit1 == "m" and unit2 == "mm":
    ans = float(num1)*1000
elif unit1 == "km" and unit2 == "m":
    ans = float(num1)*1000
elif unit1 == "m" and unit2 == "km":
    ans = float(num1)/1000
elif unit1 == "mm" and unit2 == "km":
    ans = float(num1)/1000000

I recommend nesting dictionaries for your conversions. Usually if you have a long chain of elif comparisons to a string or number, you should use a dictionary.

You can store the first unit as the primary key, and then the second value is the key of the second unit. The values then all need to be a number that you can multiply by in order to perform the conversion. So to divide by 1000, instead the value to multiply by is 1/1000. You could technically store functions to call in order to evaluate the conversions but just numbers is simpler.

conversions = {
                "mm": {"mm": 1, "cm": 1/10, "m": 1/1000, "km": 1/1000000},
                "cm": {"mm": 10, "cm": 1, "m": 1/100, "km": 1/100000},
                "m":  {"mm": 1000, "cm": 100, "m": 1, "km": 1/1000},
                "km": {"mm": 100000, "cm": 10000, "m": 1000, "km": 1},
              }

This makes it easier to see if all of them are implemented, they're kept in neat rows so you can make sure none of the math seems off. Another advantage is that you can automatically generate a list of values to tell the user about.

unit1 = raw_input ("Which unit would you like to convert from?\n"
                   "We support: " + ", ".join(conversions.keys()))
unit2 = raw_input ("Which unit would you like to convert to?\n")
                   "We support: " + ", ".join(conversions[unit1].keys()))

Also you could now make sure that the user is typing in a valid key this way.

while True:
    try:
        unit1 = raw_input ("Which unit would you like to convert from?\n"
                           "We support: "
                           ", ".join(conversions.keys())).lower()
        unit2 = raw_input ("Which unit would you like to convert to?\m")
                           "We support: "
                           ", ".join(conversions[unit1].keys())).lower()
        convert = conversions[unit1][unit2]
    except KeyError:
        print ("That is not a valid key, please try again")

conversions[unit1][unit2] is just called in order to test whether the key exists and will raise an error if it doesn't so that the user will be told to enter new keys. I also added .lower() so that if the user inputs CM, Cm or even cM they'll all be converted to cm to match the key.

I recommend wrapping the number input similarly, as it prevents errors later.

while True:
    try:
        num1 = float(raw_input("Enter your value: " ))
    except ValueError:
        print ("That is not a valid float, please try again.")

Now when it comes to the actual logic, it's really quite simple since both values are already prepared:

ans = num1 * convert