1-line conditional statement needs to be trimmed down I have this conditional statement with use of regex:

do_stuff if (@string.match(/<(?:ingr?)>/i) &&
            @string.match(/<(?:prod:alchemy?)>/i)) ||
            @string.match(/<(?:key?)>/i)

It's a 1-line conditional statement, but due to it's length, it taking 3 lines instead. I'm wondering if there's a way to trim this down, so that I can fit it into 1 line. Preferably to be something like

do_stuff if (regex here)

You could combine all the tests into one regular expression, but I don't recommend it.  The following is equivalent to what you wrote:

do_stuff if @string =~ /(?'i'<ingr?>).*(?'a'<prod:alchemy?>)|\g<a>.*\g<i>|<key?>/i

Your regular expressions are a bit odd, so I suspect that what you wrote is not what you meant to write.

The non-capturing grouping parentheses (?:…) don't seem to serve any purpose, so you can get rid of them.

The other question marks make the preceding letter optional.  For example, /<key?>/i matches <key> or <Key> or <ke> but not <ken>.

The tricky obstacle to merging the regular expressions is that the <ingr?> and <prod:alchemy?> tests both have to pass, and they can occur in either order.  Therefore, you have to allow <ingr?>.*<prod:alchemy?> as well as <prod:alchemy?>.*<ingr?>  That's possible with

<ingr?>.*<prod:alchemy?>|<prod:alchemy?>.*<ingr?>

Of course, repeating yourself is bad for verbosity and maintainability.  You can lessen the pain slightly by using named groups and subexpression calls.

After having done all that, though, the one long regex is more difficult to understand than your original code.  Therefore, I recommend leaving it alone.

If you're dissatisfied with the original code, though, there may be other improvements that you could make, but you haven't provided any context for your question so I can't make alternative suggestions.


My solution for Advent of Code 2020, Day 4, in C Here is my solution for Advent of Code 2020, Day 4, Part 1  in C. I tried using fscanf or fgets, but couldn't find a nice of doing it, so I ended up using getc. Is there a way of using fscanf or fgets that makes this code better? Also, how does the code look in general?
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef struct keyValuePair
{
    char key[4];
    char value[20];
} KeyValuePair;

typedef struct passport
{
    KeyValuePair fields[8];
    int numberOfFields;
} Passport;

int main()
{
    FILE* f = fopen("../input.txt", "r");
    if (f == NULL)
    {
        printf("Fail opening file\n");
        return -1;
    }

    Passport passports[1000];
    int currentPassport = 0;
    int currentField = 0;

    char key[4];
    char value[20];

    int c;
    while (!feof(f))
    {
        c = getc(f);
        if (c == '\n')
        {
            ++currentPassport;
            currentField = 0;
            continue;
        }
        else
        {
            // read key
            key[0] = (char)c;
            key[1] = (char)getc(f);
            key[2] = (char)getc(f);
            key[3] = '\0'; // place null terminator at the end

            // discard ':'
            c = getc(f);

            // read value
            int index = 0;
            while (c != ' ' && c != '\n')
            {
                c = getc(f);
                if (c == EOF) { break; } // at the last line, instead of a new line we have EOF
                else if (c != ' ' && c != '\n') { value[index++] = (char)c; }
            }
            value[index] = '\0';  // place null terminator at the end

            strcpy(passports[currentPassport].fields[currentField].key, key);
            strcpy(passports[currentPassport].fields[currentField].value, value);
            ++currentField;

            ++passports[currentPassport].numberOfFields;
        }
    }

    int numberOfValidPassports = 0;
    for (int i = 0; i <= currentPassport; ++i)
    {
        if (passports[i].numberOfFields == 8) { ++numberOfValidPassports; }
        else if (passports[i].numberOfFields == 7)
        {
            int cidFound = 0;
            for (int j = 0; j < passports[i].numberOfFields; ++j)
            {
                if (strcmp(passports[i].fields[j].key, "cid") == 0)
                {
                    // passport contains cid and has 7 fields, therefore invalid
                    cidFound = 1;
                    break;
                }
            }
            if (!cidFound) { ++numberOfValidPassports; }
        }
    }

    printf("Number of valid passports: %d", numberOfValidPassports);

    fclose(f);
    return 0;
}

Split your program into multiple functions
You can improve the readability and maintainability of your program greatly by splitting it into multiple functions that each do a simple, well-defined task. For example, you can reduce main() to:
int main()
{
    FILE* f = fopen("../input.txt", "r");
    int numberOfValidPassports = countValidPassports(f);
    fclose(f);
    printf("Number of valid passports: %d", numberOfValidPassports);
};

So the only responsibility of main() is now the I/O: reading the file, and writing the result to stdout. The function countValidPassports() now has to do the same as the original main(), but it doesn't have to worry about I/O anymore. Of course, this new countValidPassport() function can itself be split into multiple functions.
Check for errors while reading from an open file
You only added a check to ensure the file was opened correctly, however there can also be errors while reading the file. It is good practice to check for those as well, and to report them properly by printing an error message (to stderr) and by exitting the program with a non-zero exit code.
If a read error would occur, your code would enter an infinite loop, since you used feof() to check for the end of the file. If there is a read error before the end of the file, feof() will not return true. In general, avoid a while(!feof(...)) loop. Just check the result from an actual read operation to check whether it succeeded:
while ((c = fgetc(f)) != EOF) {
    ...
}

The fgetc() function will return EOF when it cannot read a character, either because there was a read error or because the end of file was reached.
Avoid making too many assumptions about the input
Your code makes a lot of assumptions of what the input looks like. For example, you assume that keys are always 3 characters long, and that the values are at most 20 characters long. You also assume that the only possible keys are valid passport keys. But what if you get this line in the input?
this:isaverylongkeyandvaluepair

This would cause you to write pas the end of the value arrays. Even if there is no buffer overflow issue, consider the following input:
eyr:1 iyr:2 byr:3 byr:4 byr:5 byr:6 byr:7 byr:8

You would count this as a valid passport, but it has several issues:

Not all seven required fields are present
Some fields are repeated (is that allowed?)
The expiration year is earlier than the issue year
The issue year is earlier than the birth year

Perhaps not all things need to be validated by your program, after all the task is only to check if all the required fields are present. It also doesn't mention whether any other key besides the eight mentioned in the task are allowed. But for sure you should check whether all seven required fields are present.
Don't store data unnecessarily
While you are parsing, you are storing copies of the keys and values you found in the array passports. But nothing in that array is used in the end, you only need to print the number of valid passports. So it is just a waste of memory. The only thing you have to remember is, for the current passport you are parsing, which keys you have seen so far. Once you have finished parsing a single passport, you can immediately decide whether it is valid or not, and increment numberOfValidPassports if valid.
This also solves the potential issue you have if the input contains more than 1000 passports.
Using fscanf() or fgets()
Indeed, it is good to consider using fscanf() or fgets() when parsing a file. Your current method of parsing it character by character is somewhat inefficient. Ideally, you would write something like:
while (...) {
    char key[...];
    char value[...];

    if (fscanf(f, " %[^: \n]:%s", key, value) == 2) {
        // we read a valid key:value pair
        ...

    }
}

In the format string " %[^: \n]:%s", there first is a space which will "eat" all the leading whitespace, then we match a string that can contain any character except a colon, space or newline. Then it expects a colon, and after that anything until the next whitespace.
However, it is impossible to distinguish whitespace separating key:value pairs from empty lines separating passports this way. So instead you can use fgets() to read in whole lines. It is easy to check if a line is empty, and if so you know it separated two passports. If there is anything in it, you can use sscanf() to parse it, like so:
char line[...];
while (fgets(line, sizeof line, f)) {
    if (/* check for empty line */) {
        // Passport separator
        ...
    } else {
        // Scan this line for key:value pairs
        char key[...];
        char value[...];
        char *start = line;
        int n;

        while (sscanf(start, " %[^: \n]:%s%n", key, value, &n) == 2) {
             // we read a valid key:value pair
             ...
             start += n;
        }
    }
}

The trick here is using %n to check how many characters the key:value pair was, so we can skip it for the next call to sscanf().
However: be aware of possible buffer overflows. There are several ways to deal with them, either use field width specifiers in the format string to tell sscanf() to not read more characters than your buffer is long, or if you can use POSIX extensions, consider using %ms to have sscanf() allocate a large enough buffer for the string for you. Since you don't care about the values, you can avoid storing them by using assignment-suppression: %*s. Read the documentation for sscanf() to see all its possibilities.


TLE in JNEXT problem in SPOJ Problem Statement: JNEXT

  DevG was challanged to find the just next greater number which can be formed using digits of a given number. Now DevG needs your help to find that just next greater number and win the challenge.

  Input

  The first line have t number of test cases (1 <= t <= 100). In next 2*t lines for each test case first there is number n (1 <= n <= 1000000) which denotes the number of digits in given number and next line contains n digits of given number separated by space.

  Output

  Print the just next greater number if possible else print -1 in one line for each test case.

  Note : There will be no test case which contains zero in starting digits of any given number.

  Example

  Input:

2
5
1 5 4 8 3
10
1 4 7 4 5 8 4 1 2 6

  Output:

15834
1474584162

My Solution is

public static void main(String[] argh) throws Exception{
        Reader br = new Reader();
        int t= br.nextInt();

        for(int aa=0; aa<t; aa++){
            int n= br.nextInt();
            int[] arr= new int[n];
            int breakPoint= -1;
            int mI= -1;

            for(int qq=0; qq<n; qq++){
                arr[qq]= br.nextInt();
                if(qq !=0 && arr[qq] > arr[qq-1]){
                    breakPoint= qq-1;
                }
                if(breakPoint != -1){
                    if(arr[breakPoint] < arr[qq]){
                        mI= qq;
                    }
                }
            }
            if(breakPoint == -1 || n == 1){
                System.out.println(-1);
                continue;
            }

            // swap
            arr[breakPoint]= arr[breakPoint] + arr[mI];
            arr[mI]= arr[breakPoint] - arr[mI];
            arr[breakPoint]= arr[breakPoint] - arr[mI];
            breakPoint++;

            for(int ii=0; ii<breakPoint; ii++){
                System.out.print(arr[ii]);
            }
            for(int ii=n-1; ii>=breakPoint; ii--){
                System.out.print(arr[ii]);
            }
            System.out.println("");
        }
    }

My solution is having O(N) time complexity but still I am getting TLE.
Can anyone point out issue?

int[] arr

You can probably create it only once, with maximum possible size, outside for for loop.

System.out.print(arr[ii]);

You need to use StringBuilder and output only whole string in single println call. Multiple System.out.print calls are what makes it slow.


Count balanced substring Chandan got bored playing with the arrays all the time. Therefore he has decided to buy a string \$S\$ consists of \$N\$ lower case letters. Once he purchased the string, He starts formulating his own terminologies over his string \$S\$. Chandan calls a string str A Balanced String if and only if the characters of the string str can be paritioned into two multisets \$M_1\$ and \$M_2\$ such that \$M_1=M_2\$ .

For example:

Strings like "abccba" , "abaccb" , "aabbcc" are all balanced strings as their characters can be partitioned in the two multisets \$M_1\$ and \$M2\$ such that \$M_1=M_2\$.

$$M1 = {a,b,c} \\
M2 = {c,b,a}$$

whereas strings like ababab , abcabb are not balanced at all.

Chandan wonders how many substrings of his string \$S\$ are Balanced Strings? Chandan is a little guy and do not know how to calculate the count of such substrings.

For input "abccba" Balanced substring are "cc" , "bccb" , "abccba" ie count=3 (Provided as per problem statement discussion) But I guess "aa", "bb", "cc", "abba", "acca", "cbbc" are also balanced sub string for the same input which makes count=6 Any wrong in my interpretation ?

Program

 import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.Scanner;
 import java.util.Set;

    public class BalancedStrings {
    static int  returnbalance(String input){
    HashMap<Character, Integer> characters=new LinkedHashMap<>();
    int count=0;
    Character c=null;
    for(int i=0;i<input.length();i++){
        c=input.charAt(i);
        if(characters.containsKey(c)==true){
            count=characters.get(c);
            characters.put(c, count+1);
        }
        else
            characters.put(c,1);
    }
    int countunique=0;
    Set<Character> s=characters.keySet();
    for(Character cc:s){
        count=characters.get(cc);
        if(count%2==0)
            countunique++;
    }
    if(countunique!=s.size())
        return 0;
    Object[] sar= s.toArray();
    int len=0;
    for(int i=sar.length-1;i>=0;i--){
        len=len+i;
    }

    return len+countunique;

}
public static void main(String[] args) {
    Scanner in=new Scanner(System.in);
    int numberOfInputs=in.nextInt();
    in.nextLine();
    for(int i=0;i<numberOfInputs;i++){
        System.out.println(BalancedStrings.returnbalance(in.nextLine()));
    }

}

}

For input "abccba" Balanced substring are "cc" , "bccb" , "abccba" ie
  count=3 (Provided as per problem statement discussion) But I guess
  "aa", "bb", "cc", "abba", "acca", "cbbc" are also balanced sub string
  for the same input which makes count=6. Any wrong in my interpretation
  ?

A substring is a string that appears unbroken within another string. So for example, the provided answers are substrings

cc       ab|cc|ba
bccb     a|bccb|a
abccba   |abccba|

The other balanced substrings "aa", "bb", "cc", "abba", "acca", and "cbbc" are not substrings but a subset (w/ duplicates) of the characters in the original string.

The main thing to note is that order of the characters as well as the count of each characters matters.

EDIT: Formal Definition of Substring

@OP: Beware not to confuse substring with Subsequence


Turning nested jQuery Promises into flat code structure using .done() and .then() This code uses jQuery promises in combination with callbacks does the following:

Saves Group UUID to server using storeGroupOnServer()
Creates a DOM element for the group using createGroupDomNode()
Creates child elements for each image, using createNewChildDomNode()

How can this be written more flat (avoiding the nesting), using .then()?

var groupPromise = groupInstance.promise();
var arrayOfFiles = groupInstance.files();

groupPromise.done(function(fileGroupInfo) {
    storeGroupOnServer(fileGroupInfo.uuid, function(groupid){

        createGroupDomNode(groupid, function(groupnode){

            $.each(arrayOfFiles, function(i, file) {
                file.done(function(fileInfo) {
                    createNewChildDomNode(fileInfo.cdnUrl, groupnode);
                });
            });
        });
    });
});

Step 1: storeGroupOnServer():

storeGroupOnServer = function(uuid, callback) {
    $.post('saveGroup.php', {uuid:uuid},
    function(data) {
        callback(data.groupid);
    },'json');
};

Step 2: createGroupDomNode():

createGroupDomNode = function(groupid, callback) {
    var cloner = $('#cloner');
    var newnode = cloner.clone(true);
    newnode.attr('id',groupid);
    newnode.removeClass('hide');
    cloner.after(newnode);
    callback(newnode);
}

Step 3: createNewChildDomNode():

function createNewChildDomNode(imgpath, groupdom){
    imgnode = $('<img/>').attr('src',imgpath);
    picnode = $('<picture/>').append(imgnode);
    $(groupdom).first().prepend(picnode);
}

You can substitute returning Promise objects from functions for using callbacks as parameters to function, to chain .then() and .catch(). Array.prototype.map() and Promise.all() can be substituted for jQuery.each() which does not await the fulfillment of Promise objects iterated.

Which requires returning the jQuery promise object from storeGroupOnServer

storeGroupOnServer = function(uuid, callback) {
    return $.post('saveGroup.php', {uuid:uuid},'json')
           .then(function(data) {
             return data.groupid;
           });
};

and returning a jQuery promise object of Promise from createGroupDomNode

createGroupDomNode = function(groupid, callback) {
    var cloner = $('#cloner');
    var newnode = cloner.clone(true);
    newnode.attr('id',groupid);
    newnode.removeClass('hide');
    cloner.after(newnode);
    return $.Deferred().resolve(newnode);
}

and awaiting <img> element load event

function createNewChildDomNode(imgpath, groupdom){
    return $.Deferred(function(dfd) {
             imgnode = $('<img/>')
                       .on("load", dfd.resolve)
                       .on("error", dfd.reject)
                       .attr('src',imgpath);
             picnode = $('<picture/>').append(imgnode);
             $(groupdom).first().prepend(picnode);
           }).promise();
}

For example

const data = {
  uuid: {
    1: 4,
    2: 5,
    3: 6
  }
};
const response = {
  4: "a",
  5: "b",
  6: "c"
};

const groupPromise = Promise.resolve(data.uuid[1]); // initial `Promise`
const arrayOfFiles = [Promise.resolve({
  fileInfo: {
    cdnUrl: 10
  }
}), Promise.resolve({
  fileInfo: {
    cdnUrl: 20
  }
}), Promise.resolve({
  fileInfo: {
    cdnUrl: 30
  }
})];

const storeGroupOnServer = (uuid) => {
  console.log({
    uuid
  });
  // `return` response `Promise`
  return Promise.resolve(response[uuid])
};

const createGroupDomNode = (groupid) => {
  console.log({
    groupid
  });
  // `return` `groupid`
  return Promise.resolve(groupid)
};
const createNewChildDomNode = (groupdom, {
  // destructure `cdnUrl` define as `imgpath`
  fileInfo: {
    cdnUrl: imgpath
  }
}) => {
  console.log({
    imgpath,
    groupdom
  });
  // process `<img>` elements here
  // `return` resolved or rejected `Promise`
  // at `load` or `error` event
  return Promise.resolve("done processing " + imgpath)
};
let handleFiles = (groupnode, file) => {
  console.log({
    groupnode,
    file
  });
  // handle each `file` `Promise`, pass `group` node to
  // function passed to `.then()`
  return file.then(createNewChildDomNode.bind(null, groupnode))
};
const handleGroupDomNodes = (groupnode) => {
  console.log({
    groupnode
  });
  // use `Promise.all()`, `.map()` to process array of `Promise` objects
  return Promise.all(
    arrayOfFiles.map(handleFiles.bind(null, groupnode))
  )
};

groupPromise
.then(storeGroupOnServer)
.then(createGroupDomNode)
.then(handleGroupDomNodes)
.then(complete => console.log(complete)) // to illustrate chain completes
.catch(err => console.error(err)); // handle errors in chain


Building a Building - Is the building clean? I'm writing a small game and I use Kotlin for that. I want to model my buildings with the following class:

class Building (
    val name: String,
    val inputs: Array<Resource>,
    val outputs: Array<Resource>
) {}

I want to create multiple instances of this class for the map but I don't want to fill the constructor parameters every time with the same data for the same building. So my idea was to make the constructor private and write creator functions in the companion object.

class Building private constructor(
    val name: String,
    val inputs: Array<Resource>,
    val outputs: Array<Resource>
) {

    companion object {
        fun createWoodcutter(): Building = Building(
            "Woodcutter",
            arrayOf(
                Resource(ResourceType.WOOD, 1),
                Resource(ResourceType.WORKFORCE, 3),
                Resource(ResourceType.MONEY, 10)
            ),
            arrayOf(Resource(ResourceType.PLANKS, 1))
        )
    }
}

But I don't know if this strategy counts as clean code or is there a better way or design pattern for this?

First of all, I would recommend using a data class. In order to do this, it's better to use List rather than Array (because Array does not support hashCode/equals)

data class Building (
    val name: String,
    val inputs: List<Resource>,
    val outputs: List<Resource>
)

Actually, considering that the order of elements doesn't matter you could use Set or any iterable data structure as well, but my experience is that List is usually the fastest to iterate through.

As for using the companion object, that's entirely possible and will be similar to static factory methods, but I like to go beyond that and have a completely separate factory class for this. The reasoning being that I don't want to tightly couple my Building with the actual data, what if I would like to have two entirely different games in the future that both use the Building ? What if I would like to let the user design their own buildings? Either way, I see many reasons to use a separate factory class/object than use the companion object.

In Kotlin it is as easy as defining an object BuildingFactory.

object BuildingFactory {
    fun woodcutter(): Building = Building(
        "Woodcutter",
        listOf(
            Resource(ResourceType.WOOD, 1),
            Resource(ResourceType.WORKFORCE, 3),
            Resource(ResourceType.MONEY, 10)
        ),
        listOf(Resource(ResourceType.PLANKS, 1))
    )
}

Then you can create instances as easy as BuildingFactory.woodcutter()

Another approach could be to have a configuration data file specifying the buildings available and reading from that file. Then construct a building based on a name.


onResize and onLoad handlers to toggle mobile layout I'm currently using onResize and onLoad in javascript code. Both events are coded identically so I was wondering if there is a way I could combine both into just one.

function MobileNavigation(el)
{
    this.$el = el;
}

MobileNavigation.prototype.init = function ()
{
    // Event handlers
    window.addEventListener('load', this._onLoad.bind(this));
    window.addEventListener('resize', _.debounce(this._onResize.bind(this), 150));
};

MobileNavigation.prototype._onLoad = function ()
{

    var windowHeight = $(window).outerHeight();
        headerHeight = $('header .wrapped').outerHeight();
        navigationHeight = windowHeight - headerHeight;

    // only apply to minimum width for sticky navigation to appear
    if (window.innerWidth >= MIN_WIDTH_BREAKPOINT)
    {
        $('.responsive-navigation .nav-wrap').insertAfter('header .navigation-context .mobile-menu');
        $('header .nav-wrap .region-secondary-menu').insertBefore('header .nav-wrap .region-primary-menu');
    }

    else
    {
        $('.responsive-navigation').append($('header .navigation-context .nav-wrap'));
        $('.responsive-navigation .nav-wrap .region-secondary-menu').insertAfter('.responsive-navigation .nav-wrap .region-primary-menu');
    }

    $('.responsive-navigation').css({'max-height' : navigationHeight, 'height' : navigationHeight});

    // Toogle Mobile Navigation
    $('header .mobile-menu').on('click', function() {
       $(this).toggleClass('active-menu')
       $('.responsive-navigation').toggleClass('show-navigation');
    })

    // Toggle AIM Sub Menus
    $('header .navigation > .menu > li').on('click', function() {
        console.log('expand menu');
        $(this).toggleClass('display-sub-menu');

        // $('.region-primary-menu > nav > .menu > li.display-sub-menu').removeClass('display-sub-menu');
        // $(this).addClass('display-sub-menu');
    })
};

MobileNavigation.prototype._onResize = function ()
{

    var windowHeight = $(window).outerHeight();
        headerHeight = $('header .wrapped').outerHeight();
        navigationHeight = windowHeight - headerHeight;

    // only apply to minimum width for sticky navigation to appear
    if (window.innerWidth >= MIN_WIDTH_BREAKPOINT)
    {
        console.log('normal');
        $('.responsive-navigation .nav-wrap').insertAfter('header .navigation-context .mobile-menu');
        $('header .nav-wrap .region-secondary-menu').insertBefore('header .nav-wrap .region-primary-menu');
    }

    else
    {
        console.log('mobile')
        $('.responsive-navigation').append($('header .navigation-context .nav-wrap'));
        $('.responsive-navigation .nav-wrap .region-secondary-menu').insertAfter('.responsive-navigation .nav-wrap .region-primary-menu');
    }

    $('.responsive-navigation').css({'max-height' : navigationHeight, 'height' : navigationHeight});

};

Your code is mainly about this test:

if (window.innerWidth >= MIN_WIDTH_BREAKPOINT)

You shouldn't be doing this using JavaScript to do this at all. Width-dependent layout is exactly what CSS media queries are for.

Furthermore, I would caution you against activating the mobile layout based on the width. This will break the "Request Desktop Site" feature in iOS Mobile Safari, which uses User-Agent switching instead.


Validation check and code-saving I have the ubiquitous "validate and save" scenario, wherein the Save() method performs various validity checks and proceeds to save the data. The solution works just fine, but I am interested to know how elegantly and correctly this code can be refactored.

First, the domain:

public class Car
{
    public int Id { get; set; }
    public decimal Length { get; set; }
    public int Years { get; set; }

    private List<IValidationRule<Car>> _validationList;

    public Car()
    {
        this._validationList = new List<IValidationRule<Car>>();
        this._validationList.Add(new ValidateCarId());
        this._validationList.Add(new ValidateCarLength());
    }

    public void Save()
    {
        // This commented code will also work!
        //_validationList.TrueForAll(x => x.Validate(this));

        foreach (var item in _validationList)
        {
            item.Validate(this);
        }

        // Proceed to Save the data
        Console.WriteLine("Done!");
    }
}

Validation interface and validation provider classes:

interface IValidationRule<T>
{
    bool Validate(T t);
}

public class ValidateCarId : IValidationRule<Car>
{
    public bool Validate(Car t)
    {
        if (t.Id <= 0)
        {
            throw new IdNotFoundException("This Id is not available!");
        }
        else
        {
            return true;
        }
    }
}

public class ValidateCarLength : IValidationRule<Car>
{
    public bool Validate(Car t)
    {
        if (t.Length <= 1000m)
        {
            throw new LengthInvalidException("Length must be more than 1000");
        }
        else
        {
            return true;
        }
    }
}

public class ValidateCarYear : IValidationRule<Car>
{
    public bool Validate(Car t)
    {
        if (t.Years <= 0 || t.Years >= 2)
        {
            throw new YearInvalidException("Year invalid!");
        }
        else
        {
            return true;
        }
    }
}

Custom exceptions:

public class IdNotFoundException : Exception
{
    public IdNotFoundException() { }
    public IdNotFoundException(string message) : base(message) { }
    public IdNotFoundException(string message, Exception inner) : base(message, inner) { }
    protected IdNotFoundException(
      System.Runtime.Serialization.SerializationInfo info,
      System.Runtime.Serialization.StreamingContext context)
        : base(info, context) { }
}

[Serializable]
public class LengthInvalidException : Exception
{
    public LengthInvalidException() { }
    public LengthInvalidException(string message) : base(message) { }
    public LengthInvalidException(string message, Exception inner) : base(message, inner) { }
    protected LengthInvalidException(
      System.Runtime.Serialization.SerializationInfo info,
      System.Runtime.Serialization.StreamingContext context)
        : base(info, context) { }
}

[Serializable]
public class YearInvalidException : Exception
{
    public YearInvalidException() { }
    public YearInvalidException(string message) : base(message) { }
    public YearInvalidException(string message, Exception inner) : base(message, inner) { }
    protected YearInvalidException(
      System.Runtime.Serialization.SerializationInfo info,
      System.Runtime.Serialization.StreamingContext context)
        : base(info, context) { }
}

And finally, the client:

class Program
{
    static void Main(string[] args)
    {
        Car vw = new Car { Id = -2, Length = 102, Years = 25 };

        try
        {
            vw.Save();
        }
        catch (Exception e)
        {
            Console.WriteLine(e.Message);
        }

        Console.WriteLine("Passed!");
    }
}

PS: I had to provide an else block in Validate(Cat t) for _validationList.TrueForAll(x => x.Validate(this)); to work. I felt this can be superfluous. How does it compare with foreach (var item in _validationList)?

Creating a custom validator class for each field in each class could potentially lead to bloated code, while you will not get much out of this.

Your model does not support user feedback. All it does is generate exceptions, and Exceptions are not suitable for representing error messages which are displayed to the user. You can catch these exceptions and display the messages to user, but you would abuse the exception mechanism that way. The rule is - never display exception content to users. It's a potential security hole.

Your model supports only a detection of the first error - an exception is thrown, and no further analysis can run. This way you can't provide a complete report of what's invalid in the model.

Initialization of static rule objects in model constructor is suboptimal - you are creating lots of these objects any time a constructor called. Simply making these static would optimize memory usage.

I would suggest returning a custom result object containing the status of the operation and a collection of rule violations from a 'Save' method, instead of throwing and catching exceptions. This way you would not abuse the exception handling mechanism with logic that is not exceptional - for example having an incorrectly entered string field is a normal situation, not an exception.

UPDATE (based on comment):
As for the displaying exception contents to UI, a better approach would be separate save and validate operation, and use validation result to change UI state, so the Save operation is not allowed in the first place when model is not valid. For example, you could disable the save button. Then you don't need to handle validation exception in Save method. I strongly encourage you to reconsider the practice of displaying exception contents to user. Let me reiterate - non-valid user input should be considered as normal program flow, not en exceptional situation.
Consider the following:
Is it a vulnerability to display exception messages in an error page?

So i would recommend the following workflow:
When initializing a form, update the ui based on validation result.
Handle every change to the model in the ui:
If model is not valid based on current user input - update UI with validation result, such as a Save button, display validation summary as text to the user, highlight form controls containing invalid values and so on.
If model is valid for particular operation - enable the UI controls which allow this operation (the 'Save' button), and clean validation summary and the highlights from user screen.
When Save button is pressed, you already sure that the model is valid, you don't need a check, because you have already sure you enabled it because of successful validation result, and it would be disabled on failed validation result.


Listing skills for role-playing characters After I completed writing this code, I started to wonder if the switches should be replaced with a more OOP approach. I originally picked the switch approach because there will never be anymore skill groups, only the 5 listed. It also made it a bit easier on the GUI side of things to associate a selection index to one of the enum values.

I figure the KISS rule applies since the structure of the data is going to be concrete, but I'm always open to broadening my view.

public class SkillsComponent
{
    private List<AttackSkillData> strikeSkills = new List<AttackSkillData>();
    private List<AttackSkillData> magicSkills = new List<AttackSkillData>();
    private List<AttackSkillData> techSkills = new List<AttackSkillData>();
    private List<AttackSkillData> dualTechSkills = new List<AttackSkillData>();
    private List<AttackSkillData> triTechSkills = new List<AttackSkillData>();

    public SkillsComponent(List<AttackSkillData> strikeSkills, List<AttackSkillData> magicSkills, List<AttackSkillData> techSkills, List<AttackSkillData> dualTechSkills, List<AttackSkillData> triTechSkills)
    {
        this.strikeSkills = strikeSkills;
        this.magicSkills = magicSkills;
        this.techSkills = techSkills;
        this.dualTechSkills = dualTechSkills;
        this.triTechSkills = triTechSkills;
    }

    public void AddNewSkill(SkillAttackType skillAttackType, AttackSkillData data)
    {
        switch (skillAttackType)
        {
            case SkillAttackType.Strike: { strikeSkills.Add(data); } break;
            case SkillAttackType.Magic: { magicSkills.Add(data); } break;
            case SkillAttackType.Tech: { techSkills.Add(data); } break;
            case SkillAttackType.DualTech: { dualTechSkills.Add(data); } break;
            case SkillAttackType.TriTech: { triTechSkills.Add(data); } break;
        }
    }

    public List<AttackSkillData> GetListOfLearnedSkills(SkillAttackType skillAttackType)
    {
        List<AttackSkillData> listToUse = null;
        switch(skillAttackType)
        {
            case SkillAttackType.Strike: { listToUse = strikeSkills; } break;
            case SkillAttackType.Magic: { listToUse = magicSkills; } break;
            case SkillAttackType.Tech: { listToUse = techSkills; } break;
            case SkillAttackType.DualTech: { listToUse = dualTechSkills; } break;
            case SkillAttackType.TriTech: { listToUse = triTechSkills; } break;
        }
        return listToUse;
    }
}

It might be a bit cleaner if you introduce a new class (or struct) containing both AttackSkillData and SkillAttackType.

Consider something like this;

public class AttackSkill
{
    public AttackSkillData data;
    public SkillAttackType type;
}

And consider adding a List<AttackSkill>() instead of five different lists.

Your code should then look like this;

public class SkillsComponent
{
    private List<AttackSkill> skills = new List<AttackSkill>();

    public SkillsComponent(List<AttackSkill> listOfAllSkills)
    {
        this.skills = listOfAllSkills;
    }

    public void AddNewSkill(SkillAttackType skillAttackType, AttackSkillData data)
    {
        skills.Add(new AttackSkill{ type = skillAttackType, data = data});
    }

    public List<AttackSkill> GetListOfLearnedSkills(SkillAttackType skillAttackType)
    {
        return skills.Where(s => s.type == skillAttackType).ToList();
    }
}

You can then acces any type of skill using a Where() where needed. For example, to get all skills having SkillAttackType.Strike;

List<AttackSkill> strikeSkills = skills.Where(s => s.type == SkillAttackType.Strike).ToList();

It will make your code less noisy and you will be rid of the switch. Double score!


Increasing readability of maximum path sum algorithm I had written a solution for Project Euler problem 18 in Python 3. It is good in its efficiency but the problem is the readability and I think reuse.

The algorithm used is that at every iteration the maximum for all numbers in a row is calculated starting from the top and going down to find the largest path.

#This function is in "files" and used in the solution
def get_lines(path, file_name):
    with open(path + os.sep + file_name) as f:
        for line in f:
            yield line.rstrip('\n')

def prob_018():
    matrix = [[int(i) for i in line.split(' ')]
              for line in files.get_lines(RESOURCES, "018.txt")]
    for row_num in range(1, len(matrix)):
        pre = matrix[row_num - 1]
        cur = matrix[row_num]

        for el_num in range(len(cur)):
            total = cur[el_num]
            if el_num == 0:
                total += pre[el_num]
            elif el_num == len(cur) - 1:
                total += pre[el_num - 1]
            else:
                total += pre[el_num - 1] if pre[el_num - 1] > pre[el_num] \
                    else pre[el_num]

            matrix[row_num][el_num] = total
    return max(matrix[len(matrix) - 1])

The purpose of your program was unclear to me before reading the problem description. I could understand the logic flow, but I had no idea what answer it was seeking.

The name matrix implies a rectangle. While triangle or pyramid are more descriptive, I'd go with rows for readability and a comment about the shape they form. The special handling of the first and last values of cur was confusing when assuming each row was the same size.
Since this same code will be reused in problem 67 and to improve testability, rename the function and move the specifics of problem 18 to its own method.

These two simple changes yield the following:

def prob_018():
    print(maximum_path(read_rows("018.txt")))

def read_rows(file):
    return [[int(i) for i in line.split(' ')]
            for line in files.get_lines(RESOURCES, file)]

def maximum_path(rows): ...

The main algorithm has three steps:

Iterate over each pair of rows.
Calculate the better path to each cell in the lower row from the two cells above it.
Return the maximum value from the last row.

One thing that makes this difficult is that you're modifying the rows along the way. This makes extracting functions harder since they have side-effects. Instead, leave the original values untouched and build up an accumulator to hold the best path sums. Extracting this to a new accumulate_steps function yields a very readable outer algorithm:

def maximum_path(rows):
    paths = None
    for row in rows:
        paths = accumulate_steps(paths, row)
    return max(paths) if paths else None

This introduces a common trick to simplify high-level logic: moving special-case checks into extracted methods. Writing accumulate_steps to accept None for paths also allows maximum_path to handle the empty list and single-row corner cases.

With that out of the way we can focus on combining the current best paths (accumulator) with the next row to form the new best paths. Rewriting your original code to build a new row is sufficient:

def accumulate_steps(paths, row):
    if paths is None:
        return row
    el_num in range(len(row)):
        total = row[el_num]
        if el_num == 0:
            total += paths[el_num]
        elif el_num == len(row) - 1:
            total += paths[el_num - 1]
        else:
            total += paths[el_num - 1] if paths[el_num - 1] > paths[el_num] \
                else paths[el_num]
        row[el_num] = total

but I found it easier to move the first/last element handling out of the loop. Also, negative list indexes reduce the calls to len which cleans up the logic a bit. My first refactoring built a new list in-place with similar procedural code:

def accumulate_steps(paths, row):
    if paths is None:
        return row
    sums = [0] * len(row)
    sums[0] = paths[0] + row[0]
    sums[-1] = paths[-1] + row[-1]
    for i in range(1, len(row) - 1):
        sums[i] = row[i] + max(paths[i - 1], paths[i])

While it works, converting this to a list comprehension and adding on the first and last cells using + seems more Pythonic to me:

def accumulate_steps(paths, row):
    if paths is None:
        return row
    return ([paths[0] + row[0]]
            + [row[i] + max(paths[i - 1], paths[i]) for i in range(1, len(row) - 1)]
            + [paths[-1] + row[-1]])

Putting it Together

def prob_018():
    return maximum_path(read_rows("018.txt"))

def read_rows(file):
    return [[int(i) for i in line.split(' ')]
            for line in files.get_lines(RESOURCES, file)]

def maximum_path(rows):
    paths = None
    for row in rows:
        paths = accumulate_steps(paths, row)
    return max(paths)

def accumulate_steps(paths, row):
    if paths is None:
        return row
    return ([paths[0] + row[0]]
            + [row[i] + max(paths[i - 1], paths[i]) for i in range(1, len(row) - 1)]
            + [paths[-1] + row[-1]])


Caret color and movement animation in Swing I'm developing a new Look and Feel that is based on animation ,
so I've decided to make animated text caret too !
The caret works as expected every thing is fine except that am worried about how I defined my animation rules in making the babySteps method and repaint in the movementTimer not the caret one but the component one !!

at the installation of the Caret I add PropertyChangeListener witch invokes prepareColorsList if the property is "caretColor" with the new value

private void prepareColorsList(Color c) {
        Color end = reFactorAlphaBy(c, 0);
        ar.clear();
        ar.add(c);
        while (!ColorsEquals(c, end)) {
            ar.add((c = step(c, end)));
        }
    }

ar is a List<Color> and reFactorAlphaBy just returns a new color with same red,green and blue but with the specified int as alpha .
the stepColor just call the babySteps method on each value of the color and return the new color .
I did this because I didn't want to invoke the step method in the timer , am afraid this will be bad practice.

The stepsColor(int from, int to, int max) provide the maximum addition to add it to from to be to but the return will be less than max

public static int babySteps(int from, int to, int max) {
        int ret = 0;
        if (from > to) {
            for (int j = 0; j <= max; j++) {
                if (from >= to + j) {
                    ret = -j;
                } else {
                    break;
                }
            }
        } else if (from < to) {
            for (int j = 0; j <= max; j++) {
                if (from + j <= to) {
                    ret = j;
                } else {
                    break;
                }
            }
        }
        return ret;
    }

I'm afraid of the usage of the for loops (I haven't faced a performance issue with them in any project yet but if this project made it to the light it gonna have a lot of users and i need to predict if it's gonna be issue).

Last thing, it's also about the babySteps as am using it in the movement timer too.
This is a brief explanation of what I am doing:

I have two Rectangle objects: one to draw and the other to get the location of where that drawing rect should go .
I have overridden the setDot method to change the second rect bounds .
The movement timer has an ActionListener that moves the bounds of the first rect to the other one using the babySteps method

@Override
        public void actionPerformed(ActionEvent e) {
            draw.height += babySteps(draw.height, follow.height, 2);
            draw.width += babySteps(draw.width, follow.width, 2);
            draw.x += babySteps(draw.x, follow.x, 2);
            draw.y += babySteps(draw.y, follow.y, 2);
            getComponent().repaint();
        }

Here I am calling the component repaint instead of the class one because the class repaint implementation will repaint the caret rect only witch it will lead in the old carets stick to the screen !!

Am just afraid of the for loops that is inside Timer objects would make an heavy effect to ui and making crashes if the user have a lot of JTextComponent and I don't know even how to predict what will happen.

Hey Not 100% sure what's up with the babySteps function

  "the stepsColor(int from, int to, int max) provide the maximum addition to add it to > from to be to but the return will be less than max"

Is a little confusing anyway I refactored

public static int babySteps(int from, int to, int max) {
    int j = 0;
    int currentPoint = to;
    for (;j <= max; j++) {
        if (!from >= currentPoint || !from <= currentPoint) {
            return j;
        }
    }
}

Can you send me the GitHub link so I can look into the code base thanks


Rotate an image by 90 degrees Looking for review, good practices, optimizations, clean code tips etc.

final class Pixel {

    private final int color;

    public Pixel(int color) {
        this.color = color;
    }

    public int getColor() {
        return color;
    }
}

/**
 * Rotates an image by ninety degrees.
 *
 * Complexity:
 * O(row * col)
 */
public final class Rotate {

    private Rotate() { }

    /**
     * Rotes the image in anti clockwise direction.
     *
     * @param   image the image to be rotated.
     * @return  the titled image.
     * @throws  NPE
     */
    public static Pixel[][] rotateAntiClockWise(Pixel[][] image) {
        int col = image.length;
        int row = image[0].length;

        final Pixel[][] rotatedPixel = new Pixel[row][col];

        for (int i = 0; i < image.length; i++) {
            for (int j = 0; j < image[0].length; j++) {
                rotatedPixel[image[0].length -1 -j][i] = image[i][j];
            }
        }
        return rotatedPixel;
    }

    /**
     * Rotates the image in clockwise direction.
     *
     * @param   image the image to be rotated.
     * @returns the rotated image.
     * @throws  NPE
     */
   public static Pixel[][] rotateClockWise(Pixel[][] image) {
       int col = image.length;
       int row = image[0].length;

       final Pixel[][] rotatedPixel = new Pixel[row][col];

       for (int i = 0; i < image.length; i++) {
           for (int j = 0; j < image[0].length; j++) {
               rotatedPixel[j][image.length -1 - i] = image[i][j];
           }
       }
       return rotatedPixel;
   }

   public static void main(String[] args) {
       //   int[][] m = { {1, 2, 3, 4} ,
       //                {10, 20, 30, 40}};
       Pixel[][] image = new Pixel[2][4];
       image[0][0] = new Pixel(1);
       image[0][1] = new Pixel(2);
       image[0][2] = new Pixel(3);
       image[0][3] = new Pixel(4);

       image[1][0] = new Pixel(10);
       image[1][1] = new Pixel(20);
       image[1][2] = new Pixel(30);
       image[1][3] = new Pixel(40);

       Pixel[][] rotatedAntiClockNinetyDegreeImage = rotateAntiClockWise(image);

       /*
        * Expected:
        *
        * 4  40
        * 3  30
        * 2  20
        * 1  10
        *
        */
       for (int i = 0; i < rotatedAntiClockNinetyDegreeImage.length; i++) {
           for (int j = 0; j < rotatedAntiClockNinetyDegreeImage[0].length; j++) {
               System.out.print(rotatedAntiClockNinetyDegreeImage[i][j].getColor() + " ");
           }
           System.out.println();
       }

       System.out.println("==========================================");

       Pixel[][] rotatedClockWiseNinetyDegreeImage = rotateClockWise(image);

       /*
        * Expected:
        *
        * 10  1
        * 20  2
        * 30  3
        * 40  4
        *
        */
       for (int i = 0; i < rotatedClockWiseNinetyDegreeImage.length; i++) {
           for (int j = 0; j < rotatedClockWiseNinetyDegreeImage[0].length; j++) {
               System.out.print(rotatedClockWiseNinetyDegreeImage[i][j].getColor() + " ");
           }
           System.out.println();
       }
    }
}

Assuming that you will want to do more interesting things with these images later, you should probably come up with a better OOP design.  I suggest:

public interface ImageTransformation {
    public Pixel[][] transform(Pixel[][] image);
}

public class Clockwise90Rotation implements ImageTransformation {
    ...
}

public class AntiClockwise90Rotation implements ImageTransformation {
    ...
}

It's practically no work to rearrange the code that way.  In return, you gain the flexibility to do things like:

ImageTransformation[] pipeline = new ImageTransformation[] {
    new Clockwise90Rotation(),
    new ContrastEnhancer(),
    new GaussianBlur(/* radius= */ 5)
};
for (ImageTransformation t : pipeline) {
    image = t.transform(image);
}


Data mapper reuse related domain object for setting data and return a clone of the domain object This example is simplified down to the bare minimum. No DatabaseAdapter IdentityMap or anything. No fetch() method. Just what is needed to demonstrate this.

If all your data mappers have a method:

/**
 * @return EntityType
 */
public function assign(EntityType $entity, Array $row);

You are basically creating a new blank entity each time you want to assign data to it.

I have began thinking what if all the data mappers get an entity of the related type injected into its constructor:

UserMapper::__construct(User $user)

Now the

UserMapper::assign(User $user, Array $row)

method could be changed to

UserMapper::assign(Array $row);

Since the mapper has an instance of a User object which it got when the mapper was constructed and it just assigns the row data to that.

There is a slight problem though because if I do something like the following:

$user1 = $userMapper->assign( array('username' => 'user1') );
$user2 = $userMapper->assign( array('username' => 'user2') );

// Now $user1 has the same username as $user2.
$user2->setUsername('user2_something');

Any changes I make to either user instance reflects in the other instance.

This can be solved by cloning the single user instance in the mapper when returning it in the assign method:

public function assign(Array $row) {
    $this->user->setUsername( $row['username'] );
    return clone $this->user;
}

UPDATE: Or Maybe Better...

// Now the user object in the mapper never changes, just gets cloned.
public function assign(Array $row) {

    $user = clone $this->user;
    $user->setUsername( $row['username'] );

    return $user;
}

And now the user instance you get back from the UserMapper::assign(Array $row) method does not point to the user instance inside the mapper.

I have never had to use the clone feature before so I am not too sure about it.

Is this implementation okay? Could it cause any problems?

I can't speak to cloning specifically, as I've never used it before, but here are a few basic things to think about.

Is the User class used in any other methods of your UserMapper?

If it is then injecting it in the constructor, or through a setter is ideal as it no longer depends on a user object being passed to it in each method.

This is basic OOP.

Are you perhaps trying to do too much for the scope of this class?

If we follow the Single Responsibility Principle, as we should, then our classes should be responsible for just one thing. According to WikiPedia:

  A Data Mapper is a Data Access Layer that performs bidirectional transfer of data between a persistent data store (often a relational database) and an in memory data representation (the domain layer).

To me, this means that a Data Mapper is essentially a database read/write object. What you have here seems to be a Factory object that builds individual User objects. If what you are trying to accomplish is indeed a Factory and not a Data Mapper, then you should call it what it is to avoid confusion. But for a Data Mapper the assign() method seems to be out of scope. You might consider refactoring your code, perhaps create a new class if necessary. If this is the only violation, then perhaps a better way of doing this would be to simply create the new object out of the class and remove the assign() method.

Is your User class perhaps doing too much?

It seems odd seeing a user object that allows you to change its name on the fly like this. Most user objects, that I'm familiar with, perform these kinds of operations in instantiation. Allowing this kind of change is invisible to the rest of your code. In other words, between one method and the next, the object can change and your code wont know the difference. So it is possible to encounter "state" errors, such as information from a previous user not being overwritten by the new user.

Sorry I couldn't be more precise, but with so little to work with its hard to determine what you are trying to accomplish. Hope this helps.


Am I handling "spot the missing DLL" right? I have a .net 4.5 desktop script which has dependencies on some third party libraries (dll's). These libraries are used in my MainWindowViewModel. I need to check that these dll's are present before their absence allow the program to break (which is basically at the starting point of the code). This is how I am handling it at the moment: wrapping the code which fires up the MainWindowViewModel's constructor inside a try/catch. Is this the right way to do this?

In my App.xaml.cs

    protected override void OnStartup(StartupEventArgs e)
    {
        base.OnStartup(e);

        MainWindow window = new MainWindow();

        //Create the ViewModel to which the main window binds.
        try
        {
            var viewModel = new MainWindowViewModel();
            // When the ViewModel asks to be closed,
            // close the window.
            EventHandler handler = null;
            handler = delegate
            {
                viewModel.RequestClose -= handler;
                window.Close();
            };
            viewModel.RequestClose += handler;

            window.DataContext = viewModel;
            window.Show();
        }
        catch (Exception ex)
        {
            MessageBox.Show(ex.Message);
            window.Close();
        }
    }

If the dll is missing, running the program causes a message box to pop up (as you see in the code), and tells the user:

  Could not load file or assembly 'xxx.net.xxxx, version = x.x.xx.x,
  culture ... or one of the dependencies. The system cannot find the
  file specified.

I would like to have a better message rather than the whole exception body in the message box. Maybe some dedicated method could return the dll name, version, etc. separately rather than me trying to clip it out of the exception.

As I mentioned in the comments, the code you've posted could very well be doing anything that could potentially throw any exception.

MainWindow window = new MainWindow();

By putting this constructor call outside the try block, any exception thrown in the MainWindow constructor will be unhandled and the program will terminate in a not-so-gracious way. I assume the constructor does nothing special, but your OnStartup override is also making that assumption; I'd move the call inside the try block.

var viewModel = new MainWindowViewModel();

Your usage of var is inconsistent - I'd declare the MainWindow with var as well, but you could also declare the MainWindowViewModel instance with an explicit type; all that matters is consistency.

The fact that the ViewModel's constructor isn't taking any parameters tells me that the MainWindowViewModel class is likely to be tightly coupled with whatever dependencies it might have; the ViewModel could instead be constructor-injected with some service whose sole purpose would be to perform the 3rd-party DLL late binding - if that service has its own dependencies, for example a Log4net logger, it can receive it in its own constructor:

var logger = LogManager.GetLogger(typeof(LateBindingService))
var service = new LateBindingService(logger);
var viewModel = new MainWindowViewModel(service);

That would make the intent of your code much clearer, and would keep the ViewModel class more focused on being a ViewModel.

Then if you want to use an IoC container to automatically resolve and inject dependencies, you'll have absolutely nothing to change!

I like that you are holding a reference to your handler so it can be unregistered, but I find it would be cleaner to just register a private method instead of a delegate:

var view = new MainWindow();

var logger = LogManager.GetLogger(typeof(LateBindingService))
var service = new LateBindingService(logger);
var viewModel = new MainWindowViewModel(service);

viewModel.RequestClose += ViewModel_RequestClose;
view.DataContext = viewModel;
view.Show();

And that would be the whole try block.

The LateBindingService would be responsible for knowing everything there is to know about the 3rd party DLL, and would expose a method to try loading it.

The catch block should catch a much more specific exception type than System.Exception, since you want to show that message box if the late binding fails, not if anything else goes wrong at one point or another in the View's lifetime.

I'd probably have the LateBindingService throw some LateBindingException which exposes properties for everything there is to know about the missing assembly; the Message property would be 100% under your control, and then the catch block could be like this:

catch(LateBindingException exception)
{
    MessageBox.Show(exception.Message);
    viewModel.OnRequestClose();
}

Notice that I'm not calling Close directly on the view, because I want the handler to unregister itself before exiting, so I'd just have the ViewModel expose a method that raises the RequestClose event - convention for such methods is On[EventName], so OnRequestClose.


A like/unlike system using MySQL and PDO I have created a Like/Unlike system. The user clicks one to indicate a "like", and clicks again to "unlike" it. Currently I do that like this:

try {                                             -- like
    $stm1 = $db_conn->prepare("INSERT into votes (post_id, user_id) VALUES (?, ?)");
    $stm1->execute(array($post_id, $_SESSION['id']));

} catch(PDOException $e){
    if ((int) $e->getCode() === 23000) {          -- unlike
        $stm2 = $db_conn->prepare("DELETE FROM votes WHERE post_id = ? AND user_id = ?");
        $stm2->execute(array($post_id, $_SESSION['id']));

    } else {
        $error = true;
    }
}

Note: $e->getCode() === 23000 means row is duplicate. because there is a unique index on post_id and user_id columns.

Well my code works as well. Just when a professional programmer see it, he says: your algorithm is wrong and you shouldn't delete something when insert fails (on duplicate).

Well, should I change my current code? What's wrong with it? Trust me it works perfectly :) ..!

This design is a consequence of a poorly designed API. It looks like the client will hit one URL that will cause the like/unlike state to be toggled. But what if the user has multiple tabs open on the same page, then clicks "like" on each one? The user might believe that they are reiterating the "like", when in fact they are undoing it.

My advice is not to toggle the state on the server side. Offer "like" and "unlike" as two separate calls. The client should be responsible for deciding which API to call, based on the client-side state.


Interface class for SQLite databases I am learning the basics about managing an SQLite database by using Python and, mainly for practice purposes, I have then defined an interface class in order to let operations on database as clear as possible. That said, I would like to ask you for some feedback about the following code:

import sqlite3, os

class DbInterface:
  def __init__(self, file):
    self.dbfile = file

  def open(self):
    try:
      assert os.path.exists(self.dbfile)
      assert os.path.isfile(self.dbfile)
      self._dbconn = sqlite3.connect(self.dbfile)
      return 0
    except sqlite3.Error as e:
      print(e)
    except AssertionError as e:
      print('ERROR: database file not found.')
    return -1

  def close(self):
    if hasattr(self, '_dbconn'):
      self._dbconn.close()
      return 0
    return -1

  def write(self, query, *args):
    try:
      assert any(map(lambda s: query.upper().strip().startswith(s), ('CREATE TABLE', 'ALTER TABLE', 'INSERT', 'UPDATE', 'DELETE')))
      assert query.count('?') == len(args)
      c = self._dbconn.cursor()
      c.execute(query, args)
      self._dbconn.commit()
      return 0
    except AssertionError:
      print('ERROR: inconsistent query arguments.')
    except sqlite3.Error as e:
      self._dbconn.rollback()
      print('SQL ERROR:', e)
    return -1

  def read(self, query, *args):
    try:
      assert query.count('?') == len(args)
      assert query.upper().strip().startswith('SELECT')
      c = self._dbconn.cursor()
      c.execute(query, args)
      rows = c.fetchall()
      return 0, rows;
    except AssertionError:
      print('ERROR: inconsistent query arguments.')
    except sqlite3.Error as e:
      print('SQL ERROR:', e)
    return -1, ()

Here is a test function I am using for code validation:

def test(path):
  db = DbInterface(path)
  res = db.open()
  q = """
    CREATE TABLE IF NOT EXISTS example(
      id integer PRIMARY KEY,
      sometext text NOT NULL
    );
  """
  res = db.write(q)
  q = 'INSERT INTO example(sometext) VALUES(?);'
  res = db.write(q, 'a')
  q = 'INSERT INTO example(sometext) VALUES(?, ?);'
  res = db.write(q, 'b')
  q = 'INSERT INTO example(sometext) VALUES(?);'
  res = db.write(q)
  q = 'INSERT INTO example(sometext) VALUES(?, ?);'
  res = db.write(q, 'c', 'd')
  q = 'SELECT * FROM example;'
  res, data = db.read(q)
  q = 'SELECT somevalue FROM example;'
  res, data = db.read(q)
  q = 'SELECT * FROM example WHERE sometext = ?;'
  res, data = db.read(q, 'a')
  q = 'SELECT * FROM example WHERE sometext = ?;'
  res, data = db.read(q)
  res = db.close()

if __name__ == '__main__':
  test('testdb.db')

Exception handling: you handle some exceptions but you print them to the console, where they can get lost/unnoticed. It would be better to use the logger module to record them to a file. What I like to do when writing classes is to attach to the logger routine of the main module. This way, all messages go to the console AND get written to the same log file
The code would be along these lines:

import logging

def __init__(self, file, logger=None):
    self.dbfile = file

    # acquire existing logger - https://fangpenlin.com/posts/2012/08/26/good-logging-practice-in-python/
    self.logger = logger or logging.getLogger(__name__)
    self.logger.debug('Running __init__')

See the section 'Use name as the logger name' in the link above.

Redundant code:

assert os.path.exists(self.dbfile)
assert os.path.isfile(self.dbfile)

isfile is sufficient.

  os.path.isfile(path)

  Return True if path is an existing regular file. This follows symbolic links, so both islink() and isfile() can be true for the same
  path.

Source: os.path.isfile(path)

But your code will fail if the DB file does not exist. By default SQLite connect will create the file if it doesn't already exist. So I think you should preferably make that check after the call to connect to determine that the file is indeed present.

Return codes: your procedures return either 0 or -1, I wonder why you made that arbitrary choice. You could simply return a boolean value, also in your except blocks if you want, so that there is a return value on all paths.

Your read procedure returns a tuple (note that fetchall returns a list):

return 0, rows;

Personally I would only return the rows, but in case of error I would return None instead, so I know that the operation failed. If you don't look at the error code, the (empty) returned list is still iterable.
The return code doesn't matter here because it doesn't give any additional information. If I need details, I will look at the error log.

I would make the call to close optional (it's going to be forgotten sometimes anyway). Simply register a cleanup routine for your class:

import atexit

def __init__(self, iface, logger=None):
    ...
    # run cleanup routine when class terminates
    atexit.register(self.cleanup)

def cleanup(self):
    """To perform cleanup tasks when class instance is shutting down
    """
    self.logger.debug('Closing DB')
    if hasattr(self, '_dbconn'):
        self._dbconn.close()

And in fact the open method is not required either:

res = db.open()

Just move the code to the __init__ function. No need to do open & close.

Your write routine only allows certain operations:

assert any(map(lambda s: query.upper().strip().startswith(s), ('CREATE TABLE', 'ALTER TABLE', 'INSERT', 'UPDATE', 'DELETE')))

Therefore no PRAGMAs allowed but this is your choice.

One possible enhancement would be to take an existing connection as a class argument, in case you want to do something special on an open DB (like a PRAGMA), that your class does not easily allow. Although I can imagine this code is sufficient for your needs.


Converting a natural number to a permutation matrix in Python. How to speed it up perhaps avoiding messing around with lists, sets and dicts? It is something a bit complex to explain here, but the  code bellow takes a 128-bit number and converts it into a permutation matrix, a beast which I have already faced before. The matrix is represented as a list of numbers. Each number is a row.
The way I've found to do the mapping number->matrix was to convert the number through a multi-radix (or something that could be considered one)  numeric system, so each digit corresponds to a row in the matrix. Since there can't be duplicate rows, some offset magic was needed (that is one of the uses of map taboo used below).
How could this code be improved regarding data structures and use of loops?
More conceptually, what about my choice of conversion through a multi-radix system?
Could it be simpler and still be a perfect mapping from naturals to permutation matrices?

ps. There are 2^128 numbers and 35! matrices.  2^128 < 35! , So all numbers can have a unique corresponding matrix.

from sortedcontainers import SortedSet, SortedDict
def permutmatrix2int(m):
    """Convert permutation matrix 35x35 to number."""
    taboo = SortedSet()
    digits = []
    rowid = 34
    for bit in m[:-1]:
        bitold = bit
        for f in taboo:
            if bitold >= f:
                bit -= 1
        taboo.add(bitold)
        digits.append(bit)
        rowid -= 1

    big_number = digits[0]
    pos = 0
    base = b = 35
    for digit in digits[1:]:
        big_number += b * digit
        pos += 1
        base -= 1
        b *= base

    return big_number

def int2permutmatrix(big_number):
    """Convert number to permutation matrix 35x35."""
    taboo = SortedDict()
    res = big_number
    base = 35
    bit = 0
    while base > 1:
        res, bit = divmod(res, base)
        if res + bit == 0:
            bit = 0
        for ta in taboo:
            if bit >= ta:
                bit += 1
        base -= 1
        taboo[bit] = base

    for bit in range(35):
        if bit not in taboo:
            break
    taboo[bit] = base - 1

    return list(map(
        itemgetter(0), sorted(taboo.items(), reverse=True, key=itemgetter(1))
    ))

I haven't really tried to follow your code, but it sounds like you're complicating things unnecessarily by talking about "permutation matrices" instead of just "permutations." IIUC, it's trivial to get from a permutation matrix to a permutation or vice versa. The hard part is getting from an integer index ("give me the 42nd permutation of these elements") to the actual permutation.

So, the non-trivial functions you're looking for are

def nth_permutation(num_elements, which):
    [...]

def index_of_permutation(num_elements, which):
    [...]

The speedy algorithm for nth_permutation is described here (with C and PHP code)
and here in Python, although apparently the first version doesn't produce permutations in the traditional order and the second version is quadratic.

On your actual code, I'm confused by this passage:

    res, bit = divmod(res, base)
    if res + bit == 0:
        bit = 0

How can res + bit == 0, unless (res == 0) and (bit == 0)? But if (bit == 0), then it's redundant to assign bit = 0.

I also don't understand the significance of 35. Is it significant that there are 10+26-1 non-zero "digits" available in base 36? If 35 was picked completely at random, then you should really make it a parameter to the function. It'd be what I called num_elements in my signatures above.

Trivial nit: you forgot

from operator import itemgetter

big_number = digits[0]
pos = 0
base = b = 35
for digit in digits[1:]:
    big_number += b * digit
    pos += 1
    base -= 1
    b *= base

This looks suspiciously like "convert string to int." Is this essentially equivalent to

alphabet = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXY"
big_string = ''.join([alphabet[i] for i in digits])
big_number = int(big_string[::-1]], 35)

?


The block should return the user's current_active_role def current_active_role
    user_roles.each do |r|
      if r.is_active_role
        return r
      end
    end
    return nil
  end

The block above returns whatever the user's currently active role is.  There are User, UserRoles (lookup table), and Roles tables.

I'm looking for a more concise way to say "go through all their individual roles, find the active one and return it, or implicitly return nil if none of them are the active role."

You never want to combine select with a first. Instead, use detect.

def current_active_role
  user_roles.detect(&:is_active_role)
end


Supporting different orientation in viewcontrollers will you please give your thoughts (code review) on these two way of solving orientation.

About the app:

We have a universal App. The main purpose of the app is to watch the videos. The app has four screens.

Shows the list of shows
show the list of episodes in the show
details of an episode
Player screen

The way we implemented is

a navigationalViewController that shows the viewController 1 (list of show)
then drill into viewController 2 (episodes of a show)
then drill into viewController 3 (episode details)
then viewController 4 ( with av media player view) presented modally from viewController 3

Requirement of orientation:

+--------+------------------+---------------------+-----------------------+-----------------------------+
| Device | Shows list (vc1) | Episodes list (vc2) | Episode details (vc3) | Player ViewController (vc4) |
+--------+------------------+---------------------+-----------------------+-----------------------------+
| iPad   | Landscape        | Landscape           | Landscape             | Landscape                   |
+--------+------------------+---------------------+-----------------------+-----------------------------+
| iPhone | Portrait         | Portrait            | Portrait              | Portrait & Landscape        |
+--------+------------------+---------------------+-----------------------+-----------------------------+

Method 1:

Implement this following code at app delegate level

func application(_ application: UIApplication,
                 supportedInterfaceOrientationsFor window: UIWindow?) -> UIInterfaceOrientationMask {

    guard let viewController = self.window?.rootViewController?.presentedViewController else {
        if UIDevice.current.userInterfaceIdiom == .pad {
            return .landscape
        }
        return [.portrait, .portraitUpsideDown]
    }

    if viewController is PlayerViewController && viewController.isBeingDismissed == false {
        return .all
    } else {
        if UIDevice.current.userInterfaceIdiom == .pad {
            return .landscape
        }
        return [.portrait, .portraitUpsideDown]
    }
}

Method 2:

Implement this following code at navigationViewController level

class MyNavigaitonViewController: UINavigationController {

override var shouldAutorotate: Bool {
    if self.traitCollection.userInterfaceIdiom == .pad {
        return true // we are supporting both landscapeLeft and landscapeRight on iPad.
    }
    return false }

override var supportedInterfaceOrientations: UIInterfaceOrientationMask {
    if self.traitCollection.userInterfaceIdiom == .pad {
        return .landscape // we are supporting both landscapeLeft and landscapeRight on iPad.
    }
    return .portrait }

}

Player view controller has this following code in both Method 1 and Method 2

extension PlayerViewController {
override var shouldAutorotate: Bool {
    if self.traitCollection.userInterfaceIdiom == .pad {
        return true // we are supporting both landscapeLeft and landscapeRight on iPad.
    }
    // TODO: workout a way to support landscapeleft & right in phone
    return true
}

override var supportedInterfaceOrientations: UIInterfaceOrientationMask {
    return UIInterfaceOrientationMask.all
}
}

What about implementing CoreViewController class which by default support
landscape for iPad and portrait for iPhone.
All your VCs inherit from it.
As a result VC1,VC2 and VC3 will have it by default and in VC4 override rotation methods to add support for landscape mode?


WPF Load boolean values in combobox I have a Combobox and I want to edit a boolean value.

        <dxe:ComboBoxEdit ItemsSource="{Binding EnumItemsSource}"
                          DisplayMember="Name"
                          ValueMember="Id"
                          IsTextEditable="False"
                          EditValue="{Binding TargetValue, UpdateSourceTrigger=PropertyChanged}"/>

My ViewModel:

    /// <summary>
    /// Contains the ItemsSource for Enums
    /// </summary>
    public List<EnumItemObject> EnumItemsSource
    {
        get { return _enumItemsSource; }
        set
        {
            _enumItemsSource = value;
            OnPropertyChanged();
        }
    }

public class EnumItemObject
{
   public int Id {get;set;}
   public string Name {get;set;}
}

And I prepare the data for the Combobox ItemsSource that:

    /// <summary>
    /// Sets the value to the properties for the BitTemplate view. (similar with EnumTemplate)
    /// </summary>
    /// <param name="propertyInfo">a boolean property</param>
    private void PrepareDataForBitTemplate(PropertyInfo propertyInfo)
    {
        TargetValue = (int)propertyInfo.GetValue(_firstSelectedItem);
        EnumItemsSource = new List<EnumItemObject>();
        EnumItemsSource.Add(new EnumItemObject() { Id = 0, Name = "Nein" });
        EnumItemsSource.Add(new EnumItemObject() { Id = 1, Name = "Ja" });
    }

Is it the approach correct? Is there any easier solution?

UPDATE:
Extract from my ViewModel code (you remember EnumItemsSource is ItemsSource for Combobox and TargetValue the Combobox selected item):

    private void LookUpViewData()
    {
        var propertyInfo = _firstSelectedItem.GetType().GetProperty(TargetFieldDescription.fdBigViewColumnName);
        if ((propertyInfo != null) && (propertyInfo.GetValue(_firstSelectedItem) != null))
        {
            if ((int) FieldDataType.ENum == TargetFieldDescription.fdDataType)
                PrepareDataForEnumTemplate(propertyInfo);
            if ((int) FieldDataType.Bit == TargetFieldDescription.fdDataType)
                PrepareDataForBitTemplate(propertyInfo); // like EnumTemplate
            else if ((int) FieldDataType.Time == TargetFieldDescription.fdDataType)
                PrepareDataForTimeTemplate(propertyInfo);
            else
                PrepareDataForDefaultTemplate(propertyInfo);
        }
        else
            TargetValue = String.Empty;
    }

    private void PrepareDataForEnumTemplate(PropertyInfo propertyInfo)
    {
        TargetValue = (int)propertyInfo.GetValue(_firstSelectedItem);
        if (_targetFieldDescription.fdEnumSource != null)
        {
            switch (_targetFieldDescription.fdEnumSource)
            {
                case "Station":
                    EnumItemsSource = ConvertListToEnumItemObjectList(_targetFieldDescription.fdEnumSource);
                    break;
                default:
                    EnumItemsSource = EnumUtil.ConvertEnumToEnumItemObjectList(_targetFieldDescription.fdEnumSource);
                    break;
            }

        }
    }

    private void PrepareDataForBitTemplate(PropertyInfo propertyInfo)
    {
        var value = propertyInfo.GetValue(_firstSelectedItem);
        bool bValue;
        if (value == null)
            TargetValue = 0;
        else if (bool.TryParse(value.ToString(), out bValue))
            TargetValue = (bValue) ? 1 : 0;
        else
            TargetValue = 0;
        //TargetValue = (int)propertyInfo.GetValue(_firstSelectedItem);
        var bitItems = new List<EnumItemObject>();
        bitItems.Add(new EnumItemObject { Id = 0, Name = "Nein" });
        bitItems.Add(new EnumItemObject { Id = 1, Name = "Ja" });
        EnumItemsSource = bitItems;
    }

So I want to load three possible kind of data in my Combobox. Enums, List of Objects (with name and id) and booleans (Yes/No List).

It's not clear where this PrepareDataForBitTemplate(PropertyInfo) method is located. Is it code-behind? Whatever it is, if the goal is to bind a ComboBox with some enum values, I would much rather keep it standard - meaning the DataContext of the ComboBox is the ViewModel of the containing Window or UserControl:

public IEnumerable<SomeEnumType> ViewModelEnumValues
{
    get { return Enum.GetValues(typeof(SomeEnumType)).Cast<SomeEnumType>(); }
}

private SomeEnumType _selectedEnumValue;
public SomeEnumType SelectedEnumValue
{
    get { return _selectedEnumValue; }
    set { _selectedEnumValue = value; NotifyPropertyChanged(() => SelectedEnumValue); }
}

Before I move on to the corresponding XAML markup, a few observations:

The ViewModel implementation derives from some ViewModelBase abstract class which implements INotifyPropertyChanged and allows its derivatives to specify a property name in a strongly-typed way. I see your setter calls some OnPropertyChanged() method, but not how that method is able to notify the View that a specific property was changed.
The name Enum refers to a specific language construct. Calling something "Enum" when that something is not an enum is rather confusing.
I don't see why you're not using an actual enum when the values you're trying to load are basically "Yes" and "No".
EnumItemObject is an awful name, for several reasons:

It's not an object, it's a class - an object is an instance of a class.
It's not an enum item, it's essentially a ViewModel that exposes two properties - an int and a string.
It's not immediately apparent how this class relates to any type of enum.

Now, given that your ViewModel exposes the above SelectedEnumValue and ViewModelEnumValues properties (both bad/stub names), with a ValueConverter the XAML for the ComboBox would be as simple as this:

<ComboBox ItemSource="{Binding ViewModelEnumValues}"
          SelectedItem="{Binding SelectedEnumValue, Mode=TwoWay}">
    <ComboBox.ItemTemplate>
        <DataTemplate>
            <Border Style="{StaticResource ListItemBorder}"> <!-- just some border -->
                <StackPanel>
                    <Image Source="{Binding Value, Converter={StaticResource EnumValueIconConverter}}" />
                    <Label Content="{Binding Value, Converter={StaticResource EnumValueNameConverter}}" />
                </StackPanel>
            </Border>
        </DataTemplate>
    </ComboBox.ItemTemplate>
</ComboBox>

And the converters would look like this:

using resx = Project.Properties.Resources;

public class EnumValueToStringConverter : ConverterBase
{
    public override object Convert(object value, Type targetType, object parameter, CultureInfo culture)
    {
        SomeEnumType result;
        return Enum.TryParse(value.ToString(), out result)
                   ? resx.ResourceManager.GetString(promoType + "Caption")
                   : string.Empty;
    }
}

public class EnumValueToIconConverter : ConverterBase
{
    public override object Convert(object value, Type targetType, object parameter, CultureInfo culture)
    {
        var result = string.Empty;
        var typedValue = value As SomeEnumType;
        if (typedValue != null)
        {
            switch(typedValue)
            {
                case SomeEnumType.EnumValue1:
                    result = "images/image-for-value1.png";
                    break;
                case SomeEnumType.EnumValue2:
                    result = "images/image-for-value2.png";
                    break;
            }
        }

        return new BitmapImage(new Uri("/project.namespace;component/" + result, UriKind.Relative));
    }
}

Of course this is overkill if all you want is the enum names in your dropdown list - but I tend to define a DataTemplate for just about everything (and I think I have a tendency to abuse converters, too).

If all you need is to display an enum value directly in the UI, then this will suffice:

public IEnumerable<string> ViewModelEnumNames
{
    get { return Enum.GetNames(typeof(SomeEnumType)).ToList(); }
}

private string _selectedEnumName;
public string SelectedEnumName
{
    get { return _selectedEnumName; }
    set { _selectedEnumName = value; NotifyPropertyChanged(() => SelectedEnumName); }
}

<ComboBox ItemSource="{Binding ViewModelEnumNames}"
          SelectedItem="{Binding SelectedEnumName, Mode=TwoWay}">


Fractional Knapsack in Java So I solved Fractional Knapsack problem:

  There are n items in a store. For i =1,2, . . . , n, item i has weight wi > 0 and worth vi > 0. Thief can carry a maximum weight of W pounds in a knapsack. In this version of a problem the items can be broken into smaller piece, so the thief may decide to carry only a fraction xi of object i, where 0 ≤ xi ≤ 1. Item i contributes xiwi to the total weight in the knapsack, and xivi to the value of the load.
  In Symbol, the fraction knapsack problem can be stated as follows.
  maximize nSi=1 xivi subject to constraint nSi=1 xiwi ≤ W
  It is clear that an optimal solution must fill the knapsack exactly, for otherwise we could add a fraction of one of the remaining objects and increase the value of the load. Thus in an optimal solution nSi=1 xiwi = W

import java.util.Scanner;

public class FractionalKnapsack{

 static int[] value;
 static int[] weight;
 static float[] ratio;
 static int knapSackWeight;

 static void getMaximumBenefit() {

     int currentWeight = 0;
     float benefit = 0;

     while(currentWeight < knapSackWeight) {

        int item = getMaxRatioItem();

        //No items left
        if(item == -1) {

            break;
       }

       for(int i=0;i<weight[item];i++) {

           if(currentWeight+ratio[item]<=knapSackWeight) {
               currentWeight++;
               benefit = benefit + ratio[item];
            }
       }

        //Removing the item from array
        ratio[item] = 0;
     }

    System.out.println("Weight: " + currentWeight + " Benefit: " + benefit);

 }

static int getMaxRatioItem() {

    float maxRatio = 0;
    int ratioIndex = -1;

    //Getting max ratio
    for(int i=0;i < ratio.length;i++) {

        System.out.println(ratio[i]);

        if(ratio[i] > maxRatio) {

            maxRatio = ratio[i];
            ratioIndex = i;
        }
    }

    return ratioIndex;
}

  public static void main(String[] args) {

    Scanner in = new Scanner(System.in);

    int n = in.nextInt();
    value = new int[n];
    weight = new int[n];
    ratio = new float[n];

    for (int i = 0; i < n; i++) {
        weight[i] = in.nextInt();
        value[i] = in.nextInt();
        ratio[i] = (float)value[i] / weight[i];
    }

   knapSackWeight = in.nextInt();

    getMaximumBenefit();

}
}

Is there any better solution? The running time will be n^2?

I am not going to review the actual algorithm. However, there are some improvements possible:

1

static int[] value;
static int[] weight;
static float[] ratio;
static int knapSackWeight;

You expose the state of your algorithm to entire package. Also, your implementation is not thread-safe: if two threads call the getMaximumBenefit more or less simultaneously, they might interfere.
One way to circumvent this issue is to make sure that getMaximumBenefit  constructs an object holding the state of computation and manipulate only that object throughout the computation.

2

getMaximumBenefit should be declared public. Otherwise no-one
will be able to call it outside of your (default) package.

3

Since getMaxRatioItem is a helper routine, it would be nice to declare it private.

4

int ratioIndex = -1;

What is the purpose of the value -1? It would be nice to declare a constant:

private static final int NO_ITEMS_LEFT = -1;

and then write

int ratioIndex = NO_ITEMS_LEFT;
...
if (item == NO_ITEMS_LEFT) {
    return benefit;
}

A nice point here is that if you need for some reason to change the value of that constant, you will have to edit its definition only in one place. The rule here is not to hardcode magic values, but make named constants out of them.

5

System.out.println(ratio[i]);

Printing within an algorithm is a bad taste (unless debugging). Think about how much output you get in production whenever computing a large problem instance.

6

getMaximumBenefit should return some simple data structure representing the result, not just print it on standard output.

Summa summarum

All in all, I had this in mind:

import java.util.Objects;
import java.util.Scanner;

public class FractionalKnapsackSolver {

    private static final int MINIMUM_CAPACITY = 1;
    private static final int NO_ITEMS_LEFT = -1;

    private final int[] values;
    private final int[] weights;
    private final float[] ratios;
    private final int knapsackCapacity;

    private FractionalKnapsackSolver(final int[] values,
                                     final int[] weights,
                                     final float[] ratios,
                                     final int knapsackCapacity) {
        this.knapsackCapacity = checkCapacity(knapsackCapacity);
        this.values = Objects.requireNonNull(values,
                                             "The value array is null.");
        this.weights = Objects.requireNonNull(weights,
                                              "The weight array is null.");
        this.ratios = Objects.requireNonNull(ratios,
                                             "The ratio array is null.");
        checkWeightArrayLength();
        checkRatioArrayLength();
    }

    private int checkCapacity(final int capacity) {
        if (capacity < MINIMUM_CAPACITY) {
            throw new IllegalArgumentException(
                    "Nonpositive capacity given: " + capacity + "." +
                    "Must be at least " + MINIMUM_CAPACITY + ".");
        }

        return capacity;
    }

    private void checkWeightArrayLength() {
        if (weights.length != values.length) {
            throw new IllegalArgumentException(
                    "The length of the weight array (" + weights.length +
                    ") does not match the length of the value array (" +
                    values.length + ").");
        }
    }

    private void checkRatioArrayLength() {
        if (ratios.length != values.length) {
            throw new IllegalArgumentException(
                    "The length of the ratio array (" + ratios.length +
                    ") does not match the length of the value array (" +
                    values.length + ").");
        }
    }

    public static float computeBenefit(final int[] values,
                                       final int[] weights,
                                       final float[] ratios,
                                       final int knapsackCapacity) {
        final FractionalKnapsackSolver solver =
                new FractionalKnapsackSolver(values,
                                             weights,
                                             ratios,
                                             knapsackCapacity);

        return solver.compute();
    }

    private int getMaximumRatioItem() {
        float maximumRatio = 0.0f;
        int maximumRatioIndex = NO_ITEMS_LEFT;

        for (int i = 0; i < ratios.length; ++i) {
            if (maximumRatio < ratios[i]) {
                maximumRatio = ratios[i];
                maximumRatioIndex = i;
            }
        }

        return maximumRatioIndex;
    }

    private float compute() {
        int currentWeight = 0;
        float benefit = 0.0f;

        while (currentWeight < knapsackCapacity) {
            final int item = getMaximumRatioItem();

            if (item == NO_ITEMS_LEFT) {
                return benefit;
            }

            for (int i = 0; i < weights[item]; ++i) {
                if (currentWeight + ratios[item] <= knapsackCapacity) {
                    currentWeight++;
                    benefit += ratios[item];
                }
            }

            ratios[item] = 0;
        }

        return benefit;
    }

    public static void main(String[] args) {
        Scanner in = new Scanner(System.in);

        final int n = in.nextInt();
        final int[] values = new int[n];
        final int[] weights = new int[n];
        final float[] ratios = new float[n];

        for (int i = 0; i < n; ++i) {
            weights[i] = in.nextInt();
            values[i] = in.nextInt();
            ratios[i] = (float)(values[i]) / weights[i];
        }

        final int knapsackCapacity = in.nextInt();

        System.out.println(computeBenefit(values,
                                          weights,
                                          ratios,
                                          knapsackCapacity));
    }
}

Hope that helps.


Find cells crossed by a line This method find cells that are crossed by line A-B and adds them to one vector.
I have got some duplicates here and some lines of code are very similiar but they differ in one variable. Should this duplicated be fixed or is it fine?

std::vector<Cell> Grid::findCells(int xA,int yA,  int xB,  int yB) {

vector<Cell> setOfCellsToUnmark;
//
int xI, yI, t;

if (xA < xB) {
    for (xI = coordinateToCellIndex(xA) * SIZE_CONST + MOVEMENT;
         xI < xB; xI += SIZE_CONST) {
        t = (xI - xA) / (xB - xA);
        yI = yA + t * (yB - yA);
        //find coordinates that X axis is crossed by line from xA to xB inclusive
        addToVector(xI, yI, setOfCellsToUnmark);
        //find coordinates that X axis is crossed by line from xA to xB exclusive
        addToVector(xI + MOVEMENT, yI, setOfCellsToUnmark);
    }
}
else {
    for (xI = coordinateToCellIndex(xA) * SIZE_CONST - MOVEMENT;
         xI > xB; xI -= SIZE_CONST) {
        t = (xI - xA) / (xB - xA);
        yI = yA + t * (yB - yA);
        addToVector(xI, yI, setOfCellsToUnmark);
        addToVector(xI + MOVEMENT, yI, setOfCellsToUnmark);
    }
}

if (yA < yB) {
    for (yI = coordinateToCellIndex(yA) * SIZE_CONST + MOVEMENT;
         yI < yB; yI += SIZE_CONST) {
        t = (yI - yA) / (yB - yA);
        xI = xA + t * (xB - xA);
        addToVector(xI, yI, setOfCellsToUnmark);
        addToVector(xI, yI + MOVEMENT, setOfCellsToUnmark);
    }
} else {
    for (yI = coordinateToCellIndex(yA) * SIZE_CONST - MOVEMENT;
         yI > yB; yI -= SIZE_CONST) {
        t = (yI - yA) / (yB - yA);
        xI = xA + t * (xB - xA);
        addToVector(xI, yI, setOfCellsToUnmark);
        addToVector(xI, yI + MOVEMENT, setOfCellsToUnmark);
    }
}
return setOfCellsToUnmark;
}

void Grid::addToVector(const int x, const int y, vector<Cell> & setOfCells) const {
    Cell c = coordinateToCellIndex(x, y);
    if (isCellInGrid(c)) {
        setOfCells.push_back(c);
    }
}

What you're implementing is called Bresenham's Line Algorithm.

You can get rid of the duplication by implementing the algorithm as described on wikipedia. Mirrored here for completeness:

 function line(x0, y0, x1, y1)
     real deltax := x1 - x0
     real deltay := y1 - y0
     real error := 0
     real deltaerr := abs(deltay / deltax)    // Assume deltax != 0 (line is not vertical),
           // note that this division needs to be done in a way that preserves the fractional part
     int y := y0
     for x from x0 to x1
         plot(x,y)
         error := error + deltaerr
         while error ≥ 0.5 then
             plot(x, y)
             y := y + sign(y1 - y0)
             error := error - 1.0

If you want to, you can also look at my implementation here.


JavaScript Sticky Footer I'm using this JS to sticky website footers at the bottom and I would like to know if is there anything to be optimised?

(function($, window, document){
    //Screen_height = Header_height + Main_Content_height + Footer_height + alltoplvlContainersMarginTop_Bottom + alltoplvlContainersPaddingTop_Bottom
    $.fn.sticky_footer = function(header,footer,content) {
        if(typeof header != undefined && typeof footer != undefined && typeof content != undefined) {
            var screen_height = $(window).height(); //Total height of the screen which is rendered within a browser window without having to scroll-down.

            var sum_of_top_bottom_props = function(id){ //Returns the sum of float values of padding-top,bottom margin-top,bottom, border-top,bottom-width.
                    if(typeof id != undefined){
                        var elem_id = id;
                        if(elem_id.length>0) { //If id exists in the page, fetch the sum of float values of the properties.
                           var prop_padding = parseFloat(elem_id.css('padding-top'))+parseFloat(elem_id.css('padding-bottom')),
                               prop_margin = parseFloat(elem_id.css('margin-top'))+parseFloat(elem_id.css('margin-bottom')),
                               prop_border = parseFloat(elem_id.css('border-top-width'))+parseFloat(elem_id.css('border-bottom-width'));
                           return prop_padding+prop_margin+prop_border;
                       } else{ //If id soesnot exist return 0;
                           console.error("The id: "+id+" doesn't exists! Pleas enter a valid id.");
                           return 0;
                       }
                   }};

            var header_height = (header.length>0) ? header.height():0, //Height of header
                footer_height = (footer.length>0) ? footer.height():0, //Height of footer
                header_props = sum_of_top_bottom_props(header), //Height of properties that contribute to header's height.
                footer_props = sum_of_top_bottom_props(footer), //Height of properties that contribute to footer's height.
                main_props = sum_of_top_bottom_props(content), //Height of properties that contribute to content's height.
                main_height = screen_height-(header_height + header_props + main_props + footer_props + footer_height);

            if(content!= undefined && content.length>0) { content.css('min-height',main_height); return true; }
            else return false;
        }
        else return false;
    }
})(jQuery, window, document);

GitHub: https://github.com/Elavarasanlee/sticky-footer.js

From a once over,

I strongly suggest lowerCamelCase: sticky_footer -> stickyFooter, screen_height -> screenHeight etc. etc.
You should compare to undefined with === or use a falsey comparison, I would suggest the falsey comparison:

if(header && footer && content) {

This: var elem_id = id; makes no sense to me, why not simply keep working with id ?
If you are going to return the sum of all 6 css values (return prop_padding+prop_margin+prop_border; ), then I would not use the temporary values but just return the actual calculation
Do not use console.log in production code
That gives something like this:

//Returns the sum of float values of padding-top/bottom,margin-top/bottom, border-top/bottom,
//if no element is provided, returns 0
var sum_of_top_bottom_props = function(id){
    if(id && id.length){
        return parseFloat(elem_id.css('padding-top')) +
               parseFloat(elem_id.css('padding-bottom')) +
               parseFloat(elem_id.css('margin-top')) +
               parseFloat(elem_id.css('margin-bottom')) +
               parseFloat(elem_id.css('border-top-width')) +
               parseFloat(elem_id.css('border-bottom-width'));
     }
     return 0;
}};

else return false is pointless, just do return false
Finally, you require the user of your script to take care of $(window).resize(function(){, your script should take care of that instead.


String manipulation with std::adjacent_find For the given strings (not containing numbers), print their shortened
  versions,  where each adjacent sequence of the same characters longer
  than 2, change to an expression consisting of a sign and a number of
  repetitions.

Sample input:

4
AAA
ABCDEF
CCCCCCDDDDDDD
ZZAACCCDDDDEEEEEE

Sample output:

A3
ABCDEF
C6D7
ZZAAC3D4E6

My code:

#include <algorithm>
#include <cstddef>
#include <functional>
#include <iostream>
#include <iterator>
#include <string>

std::string reduce(std::string const& word) {
    std::string result;
    for (auto it = word.cbegin(); true;) {
        auto curr = std::adjacent_find(it, word.cend(), std::not_equal_to<int>());
        auto dist = std::distance(it, curr) + (curr != word.cend());

        if (dist < 3) {
            result += std::string(dist, *it);
        } else {
            result += *it + std::to_string(dist);
        }

        if (curr == word.cend()) {
            break;
        }
        it = 1 + curr;
    }
    return result;
}

int main() {
    std::size_t tests;
    std::cin >> tests;
    while (tests--) {
        std::string word;
        std::cin >> word;
        std::cout << reduce(word) << "\n";
    }
}

How could I simplify or improve this code?

First impressions: nicely presented code; good use of the appropriate standard library functions and classes.

A minor suggestion would be to change the name, given that reduce is a well-known concept in functional programming (and is a function in <numeric>).  Perhaps call it compress?

I'd suggest extracting the constant 3 to give it a meaningful name.

Can we eliminate the break with some reorganisation of the loop?  Perhaps by using std::mismatch(it, it+1) instead of std::adjacent_find()?  (I haven't fully thought that through; it might not be better.)

We can avoid constructing a new string here:

        result += std::string(dist, *it);

by using the overload of append() that takes two iterators:

        result.append(dist, *it);