Caesar and Vigenère ciphers in Haskell, Take 2 (not so simple this time) I've posted my simple ciphers on Code Review several days ago (link: here). I refactored my code and tried to define a more general "cipher" function, according to Zeta's excellent advice. I've split the code to make it more readable.

General cipher

This function takes an encoding function, "zero", list of keys and list of things to encode. The "zero" here is the character that shouldn't be encoded - e.g. a whitespace.

import Data.Char (toLower, ord, chr, isUpper)

cipher :: (Eq a) => (k -> a -> a) -> a -> [k] -> [a] -> [a]
cipher f z k = helper (cycle k)
  where
    helper _ [] = []
    helper (k:ks) (x:xs)
        | x == z = z : helper (k : ks) xs
        | otherwise = f k x : helper ks xs

Caesar's cipher

Notice the support for uppercase letters. I'm not sure it is right to include it, but it was a fun exercise nonetheless.

advanceBy :: Int -> Char -> Char -> Char
advanceBy n b c = chr $ (ord c - base + n) `mod` 26 + base
  where
    base = ord b

caesar, uncaesar :: Int -> String -> String
caesar n = cipher helper ' ' [n]
  where
    helper k x =
        let base = if isUpper x then 'A' else 'a'
        in advanceBy k base x

uncaesar n = caesar (26 - n)

Vigenère's cipher

I noticed  the helper of vigenere is almost the same as that of caesar. I could somehow extract that one out, but it wouldn't really help the readability very much and also I'd need to hack a little in the definition of caesar.
Also, unVigenere can be defined in terms of vigenere (similarly to how unCaesar is defined), but it isn't really nice nor elegant, so I do it traditionally instead.

vigenere, unVigenere :: String -> String -> String
vigenere = cipher helper ' '
  where
    helper k x =
        let base = if isUpper x then 'A' else 'a'
        in advanceBy (ord k - ord base) base x

unVigenere = cipher helper ' '
  where
    helper k x =
        let base = if isUpper x then 'A' else 'a'
        in advanceBy (26 - (ord k - ord base)) base x

Some other weird cipher

Here I want to showcase a cipher that encodes a list of integers using two keys. Because I don't know any legit ciphers apart from Caesar's and Vigenère's (I'd like to, though - be sure to link some if you know them), I had to create a new throwaway cipher.

(By the way, is this right? I don't like the fromIntegral and floor parts. It doesn't matter much here,  but it'd be nice to know how to write this properly.)

fancypher, unfancypher :: Int -> String -> [Int] -> [Int]
fancypher n = cipher helper 0
  where
    helper k x = n * x * ord k

unfancypher n = cipher helper 0
  where
    helper k x = floor $ fromIntegral x / fromIntegral (n * ord k)

Bases

Vigenère has a "bug". That is due to your alphabet management. Strictly speaking, both Caesar and Vigenère advance characters in their respective alphabet. Therefore, your alphabet should be something like

ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz

and caesar 1 "Z" could lead to "a" without losing the original Caesar's meaning. It's just shifting in the alphabet.

You, on the other hand, have two alphabets:

ABCDEFGHIJKLMNOPQRSTUVWXYZ
abcdefghijklmnopqrstuvwxyz

You stay in the respective alphabet while shifting and therefore not lose the case. Up to that point, everything seems fine, right?

Now we come to the mentioned bug:

> vigenere ['A'..'Z'] $ ['a'..'z']
"uwyacegikmoqsuwyacegikmoqs"
> vigenere ['A'..'Z'] $ ['A'..'Z']
"ACEGIKMOQSUWYACEGIKMOQSUWY"

You check the case of the secret, but not of the key. Theoretically, you should check that too:

vigenere, unVigenere :: String -> String -> String
vigenere = cipher helper ' '
  where
    helper k x =
        let base  = if isUpper x then 'A' else 'a'
            kbase = if isUpper k then 'A' else 'a'
        in advanceBy (ord k - ord kbase) base x

Since you use that function so often, a baseOf might be handy:

baseOf :: Char -> Char
baseOf x = if isUpper x then 'A' else 'a'

Note that you don't notice the bug in checks, since you do the same in unVigenere

Caesar knows its key

In caesar, you can just ignore the key, since you already know it:

caesar n = cipher helper ' ' [n]
  where
    helper _ x =
        let base = if isUpper x then 'A' else 'a'
        in advanceBy n base x

More on that later.

Type signatures

While it is possible to write multiple type signatures in a single line, it's usually not done.  If I have a look at unVigenere, I will only see its definition if I miss vigenere line:

unVigenere = cipher helper ' '
  where
    …

Now I have to guess it's type. I have to check cipher, helper's type, and so on. Compare that to

unVigenere :: String -> String -> String
unVigenere = cipher helper ' '
  where
    …

which immediately tells you what type unVigenere has. I don't need to scan the rest of the code anymore.

Remove unused imports

You import toLower, but you don't use it.

Even more general cipher

While your cipher is now rather general, there are still some small things off. First of all, your cipher's only work for ASCII letters, yet the only ignored character is a single space.

We can fix this if we use a predicate instead:

cipher :: (k -> a -> a) -> (a -> Bool) -> [k] -> [a] -> [a]
cipher f p k = helper (cycle k)
  where
    helper _ [] = []
    helper (k:ks) (x:xs)
        | not (p x) = x : helper (k : ks) xs
        | otherwise = f k x : helper ks xs

Together

isAsciiLetter x = isAscii x && isLetter x -- both from Data.Char

we can now say which characters we want to change. But we can make cipher even more generic. At the moment, we need a list of keys before we can actually call cipher. But that prevents certain kinds of ciphers, namely those that work with the value of the previous character. We need some kind of state for that.

Here's where that hint from my last comment comes in:

cipher :: (k -> a -> (k, a)) -> (a -> Bool) -> k -> [a] -> [a]
cipher f p = go
  where
    go _ []     = []
    go k (x:xs) =
      | not (p x) = x : go k  xs
      | otherwise = y : go k' xs
      where
        (k',y) = f k x

What does f now do? Well, given a key and something to encrypt, it gives us the encrypted version and a new key. Here is how you would use it in caesar:

caesar :: Int -> String -> String
caesar n = cipher helper isAsciiLetter n
  where
    helper k x =
        let base = if isUpper x then 'A' else 'a'
        in (k, advanceBy k base x)

Here's a variant that shifts the first character by n, the next by n + 1 and so on:

caesar' :: Int -> String -> String
caesar' n = cipher helper isAsciiLetter n
  where
    helper k x =
        let base = if isUpper x then 'A' else 'a'
        in (k + 1, advanceBy k base x)

We can even do something silly like only cipher every second character:

caesar'' :: Int -> String -> String
caesar'' n = cipher helper isAsciiLetter 0
  where
    helper k x =
      | even k = (k + 1, x)
      | otherwise = let base = if isUpper x then 'A' else 'a'
                    in (k + 1, advanceBy n base x)

However, since our stateful function is now so powerful, we don't actually need isAsciiLetter anymore. If we want to ignore a character, we can just do so in our f:

cipher :: (k -> a -> (k, b)) -> k -> [a] -> [b]
cipher f = go
  where
    go _ []     = []
    go k (x:xs) = y : go k' xs
      where
        (k',y) = f k x

Here's caesar again with the new function:

caesar n = cipher (onlyAscii go) n
  where
    go k x = let base = if isUpper x then 'A' else 'a'
             in advanceBy k base x

The new function onlyAscii is a modifier of f. In this case, it will apply f only on ASCII letters and keep the key and the character otherwise:

onlyAscii :: (k -> a -> (k, a)) -> (k -> a -> (k, a))
onlyAscii f k x
  | isAsciiLetter x = f k x
  | otherwise       = (k, x)

But that's just a demonstration of what you could do with that function. Back to your code. How would we capture vigenere?

vigenere k = cipher (onlyAscii go) (cycle k)
  where
    go (k:ks) x = (ks, advanceBy (ord k - ord (baseOf k)) (baseOf x) x)

where baseOf is left for an exercise. Let us check that it does the same as your previous version:

the key is the cycled String, so it stayed the same
due to onlyAscii, we don't change any non-ASCII letter
if we evaluate go, we have an ASCII letter
we use the first element from the key to cipher the current x, and return the rest from the key for the next application.

Therefore, this vigenere behaves just as yours.

Atbash

This variant of cipher can be used to implement map. Don't believe me? Well, there's a cipher called Atbash. It doesn't need a key, and we could simply implement it like this:

atbashSingle :: Char -> Char
atbashSingle k = chr $ 26 - ord k + 2 * ord (baseOf k)

atbash :: String -> String
atbash = map atbashSingle

But of course, we can use cipher again:

atbash = cipher (ignoreKey atbashSingle) ()

ignoreKey :: (a -> b) -> (k -> a -> (k, b))
ignoreKey f k x = (k, f x)

Which shows that map f == cipher (ignoreKey f) ().

Exercises

That cipher is now harder to use than before. Add some convenience functions like onlyAscii and ignoreKey to make it easier to work with it.
Create a type synonym to make (k -> a -> (k, b)) more user friendly.
There's a Monad that captures this kind of computation. Which is it? Can you write cipher with the help of that Monad?


Finding a common prefix The problem I am trying to solve:

Given a string, split the string into two substrings at every possible point. The rightmost substring is a suffix. The beginning of the string is the prefix. Determine the lengths of the common prefix between each suffix and the original string. Sum and return the lengths of the common prefixes. Return an array where each element 'i' is the sum for the string 'i'. Example:

My Solution:
def commonPrefix(l):

    size=len(l)  #Finding number of strings
    sumlist=[]  #Initializing list that'll store sums
    for i in range(size):
        total=0  #Initializing total common prefixes for every string
        string=l[i]  #Storing each string in string
        strsize=len(string)  #Finding length of each string
        for j in range(strsize):
            suff=string[j:]   #Calculating every suffix for a string
            suffsize=len(suff)  #Finding length of each suffix
            common=0   #Size of common prefix for each length
            for k in range(suffsize):
                if(string[k]==suff[k]):
                    common=common+1
                else:
                    break   #If characters at equal positions are not equal, break
            total=total+common   #Update total for every suffix
        sumlist.append(total)   #Add each total in list
    return sumlist   #Return list

My solution takes up a lot of time, I need help with optimizing it.

Second for loop
Your second loop will only iterate once, because string is only 1 character long:
string = l[i]

I think you meant to slice here.
Incrementing an int
You can use the += operator to increment an integer:
x = 0

for i in range(3):
    x += 1

str.startswith
For the most part, you can use str.startswith to check if the suffix starts with the prefix (other than the case where prefix is ''):
prefix, suffix = 'a', 'abc'

suffix.startswith(prefix)
True

This is a bit more idiomatic than slicing from position i or j to compare with a prefix
Creating your prefix and suffix
When generating your prefix and suffix, slice once, and you can keep your code down to one loop:
for i in range(len(l)):
    # Generate your prefix and suffix here
    prefix, suffix = l[:i], l[i:]

    # rest of code

Instead of a counter, use an if statement
For actually tabulating your results, you can use an if statement, paired with the str.startswith logic to cut down on your second loop.
Furthermore, it appears from the example that if the prefix is an empty string, you append the length of the suffix. This can go in the if str.startswith bit because 'anystr'.startswith('') will return True:
results = []

# Start your loop
for i in range(len(mystring)):
    # Create your prefix and suffix based on slice point
    prefix, suffix = mystring[:i], mystring[i:]

    # check str.startswith here
    if suffix.startswith(prefix):
        # You can use a ternary if block in the append to cover the
        # empty string case
        results.append(len(prefix) if prefix else len(suffix))
    else:
        results.append(0)

The ternary operator will result in either the left side if the predicate is truthy, otherwise the right side
x = 'a' if True else 'b'

x
'a'

x = 'a' if False else 'b'
x
'b'

x = 'a' if '' else 'b'

x
'b'

Empty strings are falsey.
Style
Naming Conventions
Method names should be snake_case:
def some_method_name():
    # do things

Spacing
There should be a space between a name, the equals sign, and a value:
# instead of this
some_value=3

# do this
some_value = 3

The exception to this is with keyword arguments in a function
def f(key='value'):
    return None

# No extra spaces here
f(key=1)

Be careful about name shadowing
string is the name of a module, so watch out for cases where you might acidentally name a variable something that you may import (or users of your code might import). Not a huge deal in this case, but certainly something to watch out for. Note that this accounts mostly for common modules and functions, both built-in and 3rd party like string, pandas, math, sum, etc.


Simple implementation of LRU caching in PHP This is my implementation of LRU caching in PHP.

It works fine when I tested as. What are the things that I miss or need attention?

<?php

class Cache{
    private $cache;
    private $size;

    public function __construct($size)
    {
        $this->cache = [];
        $this->size = $size;
    }

    public function put($item)
    {
        if($this->isFull())
        {
            if(in_array($item, $this->cache))
            {
                $this->cache = array_diff($this->cache, [$item]);
            }
            else
            {
                array_pop($this->cache);
            }
        }

        array_unshift($this->cache,$item);
    }

    public function display()
    {
        var_dump($this->cache);
    }

    private function isFull()
    {
        if(count($this->cache) >= $this->size)
        {
            return true;
        }
    }
}

$cache_size = 4;
$entries = [3,4,4,5,6,7,4,4,3,5,2,3,1,1,4];

$cache = new Cache($cache_size);
$cache->display();

foreach($entries as $entry)
{
    $cache->put($entry);
}

$cache->display();

Output for above provided data

//blank cache
array(0) {
}
//insertion of all entries
array(4) {
  [0]=>
  int(4)
  [1]=>
  int(1)
  [2]=>
  int(3)
  [3]=>
  int(2)
}

This implementation is quite simple. I compared it with some others in PHP as well as other languages like C++, Java and JavaScript. Some use a node class to store a key and value separately (e.g. this PHP implementation) but for a simple implementation your approach seems fine.

I noticed that the method isFull() only explicitly returns a value when true is returned. While it works because the conditional that calls that method would this implicitly evaluate the empty return to a false-y  value, some would argue that methods that return a value should do so in every case, for the sake of readability. That could be achieved by simply returning the conditional:

private function isFull()
{
    return count($this->cache) >= $this->size);
}

And one other simplification would be to set the property/instance variable $cache to an empty array when it is declared, since it can be evaluated at compile time and does not depend on any run-time information to be evaluated, instead of inside the constructor:

private $cache = [];


Raspberry Pi GPIO safe clean up We are always told to call GPIO.cleanup() before we exit our Pi programs. I've seen people using try ... catch ... finally to achieve this. But hey, we are doing python here, an elegant programming language. Do you guys think this is a more elegant solution?

# SafeGPIO.py

from RPi import GPIO

class SafeGPIO(object):
    def __enter__(self):
        return GPIO

    def __exit__(self, *args, **kwargs):
        GPIO.cleanup()

Use like this:

from SafeGPIO import SafeGPIO
import time

with SafeGPIO() as GPIO:
    GPIO.setmode(GPIO.BOARD)
    GPIO.setup(7, GPIO.OUT)
    GPIO.output(7, True)
    GPIO.setup(8, GPIO.OUT)
    GPIO.output(8, True)

    val = 0
    for i in xrange(10):
        val = (val + 1) % 2
        active_pin = 7 + val
        inactive_pin = 7 + (val + 1) % 2
        GPIO.output(active_pin,True)
        GPIO.output(inactive_pin,False)
        time.sleep(2)

Intuitively I think that I don't like to encapsulate all of my program in a with SafeGPIO() as GPIO call, so I searched the library and found at_exit. Using this you should be able to have something like the following in SafeGPIO.py:

import at_exit
from RPi import GPIO as RPi_GPIO

# Something useful to re-export GPIO?! Possibly the following?
GPIO = RPi_GPIO

@atexit.register
def cleanup():
    RPI_GPIO.cleanup()

Not sure on how to re-export the GPIO, but if this construct works you only need to do from SafeGPIO import GPIO, and then when Python exits it will call the original GPIO.cleanup().

Now the real question is why don't the RPi module automatically do this if this is a best practice when using RPi? Sadly, I don't know the answer to that, but it could be a reason for it, which might affect whether this is actually a good solution or not.

PS! I've addressed some more variants related to how to do cleanup in this answer, where I also focus a little more on choosing different options related to use cases.


Updating HTML table (Userlist) with Node.js I currently have an HTML
 with all the socket.io-rooms. Client-side, the table gets fetched if you visit the page. If some room gets created or deleted, it get updated with the following code. There's also a button that gets updated to show if the room is full or not.

Now I want to know if there's any "better" way. The argument is room_list (array with objects for each room). Should I split it into two functions (fetchRoomList and updateRoomList) or use "pure" JavaScript instead of jQuery? Any other ideas?

socket.on( 'updateRoomsList', function ( room_list ) {
    var i;
    for (i = room_list.length - 1; i >= 0; i -= 1) {
        var r = room_list[ i ];
        var el = $( '.room-' + r.id ).length;
        var parts = [];

        // Create Row
        if( !el ) {
            parts.push( '
' );
            parts.push( '' + r.id + '	' );
            parts.push( '' + r.name + '	' );
            parts.push( '' + r.players.length + ' / ' +r.slots + '	' );
            parts.push( 'JOIN	' );
            $( '.lobby-container .room-list tbody' ).prepend( parts.join() );
        }
        else // Update Row
        {
            $( '.lobby-container .room-list tbody .room-' + r.id + ' .room-players' ).text( r.players.length + ' / ' +r.slots );
            if( r.players.length >= r.slots ) { // Full Room
                $( '.lobby-container .room-list tbody .room-' + r.id + ' .room-button .btn' )
                    .text( 'FULL' )
                    .addClass( 'btn-warning' )
                    .removeClass( 'btn-success' )
                    .addClass( 'disabled' );
            } else { // Not full
                $( '.lobby-container .room-list tbody .room-' + r.id + ' .room-button .btn' )
                    .text( 'JOIN' )
                    .addClass( 'btn-success' )
                    .removeClass( 'btn-warning' )
                    .removeClass( 'disabled' );
            }
        }
    }
} );

Your code looks quite clear, and I see only some possible improvements.
Regarding your questions:

I don't think it'd better to split into fetchRoomList and updateRoomList separate functions: there are a few common vars that should be computed twice; additionally it'd prevent increasing performance like I suggest below (by traversing HTML tree less times).
using pure JS instead of jQuery would obviously increase performance a bit, but IMO it doesn't worth the negative impact on readability.

Then I can suggest what follows:

caching jQuery traversal:

currently you're looking:

for .room-X once for each room
for .lobby-container .room-list tbody once for each room to add a
 (when creating row)
for .lobby-container .room-list tbody .XXX, where XXX is .room-players (once), then .room-btn .btn (twice).

So to create C rows and update U rows you spend 2xC + 4xU "big" traversals for each room.
Instead you can:

first get var $rooms = $('.lobby-container .room-list tbody'); once for all
then look for $room = $(#${id}, $rooms); once for each room
and use $rooms.prepend() when creating row or $('XXX', $room) when updating room (these are only "little" traversals, since inside of $rooms).

this way you spend only 1 "big" traversal, plus 0xC + 3xU "little" traversals.
Note that room-X is unique so to be more strictly significant you might change the class into id.

less important: you also can cache r.id (used several times)
factorizing operations when creating room:

depending on r.players.length >= r.slots you execute different but analogue operations on the room, using twice the same jQuery traversal
instead you can get the condition var isFull = r.players.length >= r.slots;, then use it in each operation, all operations being attached to the same unique traversal
this way you actually spend only 1 "big" traversal, plus 0xC + 2xU "little" traversal

Last point, I suggest using tagged strings for row creation, which at the same time reduces code and improves readability. You'll notice I also choosed to omit the closing tags
 and 	 (optional in HTML5).

Here is the whole refactored function:

socket.on( 'updateRoomsList', function (room_list) {
  var $rooms = $('.lobby-container .room-list tbody');
  for (var i = room_list.length - 1; i >= 0; i -= 1) {
    var r = room_list[i],
        id = `room-${r.id}`,
        $room = $(`#${id}`, $rooms);
    if (!$rooms.length) {
      // Create row
      $rooms.prepend(`

  ${r.id}
  ${r.name}
  ${r.players.length}/${r.slots}
  <tdclass="room-button">

      JOIN

        `);
    } else {
      // Update row
      $('.room-players', $room).text(`${r.players.length}/${r.slots}`);
      var isFull = r.players.length >= r.slots;
      $('.room-button .btn', $room)
        .text(isFull ? 'FULL' : 'JOIN')
        .toggleClass('btn-warning disabled', isFull)
        .toggleClass('btn-success', !isFull);
    }
  }
}


How to reduce this RadioGroup Code? I have 18 RadioGroup. I am declaring the RadioGroup like this way

private RadioGroup kn1RadioGroup, kn2RadioGroup, kn4aRadioGroup,
kn4bRadioGroup, kn4cRadioGroup, kn4dRadioGroup, kn4eRadioGroup,
kn4fRadioGroup, kn4gRadioGroup, kn4hRadioGroup, kn4iRadioGroup,kn4jRadioGroup,ap2aRadioGroup,ap2bRadioGroup,
ap2cRadioGroup,ap2dRadioGroup,ap2eRadioGroup,pr1RadioGroup;

I have created getter and setter for these RadioGroup. And I am setting the RadioGroup id like this way

fragment.setKn1RadioGroup((RadioGroup) findViewById(R.id.kn1_radio_group));
fragment.setKn2RadioGroup((RadioGroup) findViewById(R.id.kn2_radio_group));
fragment.setPr1RadioGroup((RadioGroup) findViewById(R.id.pr1_radio_group));
fragment.setKn4aRadioGroup((RadioGroup) findViewById(R.id.kn4a_radio_group));
fragment.setKn4bRadioGroup((RadioGroup) findViewById(R.id.kn4b_radio_group));
fragment.setKn4cRadioGroup((RadioGroup) findViewById(R.id.kn4c_radio_group));
fragment.setKn4dRadioGroup((RadioGroup) findViewById(R.id.kn4d_radio_group));
fragment.setKn4eRadioGroup((RadioGroup) findViewById(R.id.kn4e_radio_group));
fragment.setKn4fRadioGroup((RadioGroup) findViewById(R.id.kn4f_radio_group));
fragment.setKn4gRadioGroup((RadioGroup) findViewById(R.id.kn4g_radio_group));
fragment.setKn4hRadioGroup((RadioGroup) findViewById(R.id.kn4h_radio_group));
fragment.setKn4iRadioGroup((RadioGroup) findViewById(R.id.kn4i_radio_group));
fragment.setKn4jRadioGroup((RadioGroup) findViewById(R.id.kn4j_radio_group));
fragment.setAp2aRadioGroup((RadioGroup) findViewById(R.id.ap2a_radio_group));
fragment.setAp2bRadioGroup((RadioGroup) findViewById(R.id.ap2b_radio_group));
fragment.setAp2cRadioGroup((RadioGroup) findViewById(R.id.ap2c_radio_group));
fragment.setAp2dRadioGroup((RadioGroup) findViewById(R.id.ap2d_radio_group));
fragment.setAp2eRadioGroup((RadioGroup) findViewById(R.id.ap2e_radio_group));

Is there any easy way to do it?

The easiest way would be to define a constant integer array holding the ids:

private static final int[] ids = { R.id.kn1_radio_group, R.id.kn2_radio_group, R.id.pr1_radio_group, /* and so on... */};

And then using the same method as in my answer to your previous question.

However, your naming of these RadioGroups tells me that there might be another structure here. And I also suspect that there is more to your application than what we see here. So what you really should do is to think about the structure of your variables; Should a kn be a class? Should kn4 be an array on it's own, or should perhaps kn be a two-dimensional array (one dimension for the number and one for the letter. These are questions that none of us currently can answer considering the lack of context we have about your application. If you would explain a bit more about What your application is for and what a kn / ap RadioGroup is, then we might help you more. You probably have some reasons for naming the radio groups the way that you do, in which case a different structure than an array might be more useful.

Also, I would like to add something: Why not let the fragment take care of it's own initialization of the RadioGroups? Is perhaps the RadioGroups a part of the fragment's view already? If so, then there is no need to use all these setter methods for it.


Generating Calendar month information using JavaScript I am using the below function to set month information on context passed. It is working fine, but please review it and let me know if there are any potential flaws or any improvements in this code.

/* Expects month to be in 1-12 index based. */
var monthInformation = function (year, month) {
    /* Create a date. Usually month in JS is 0-11 index based but here is a hack that can be used to calculate total days in a month */
    var date = new Date(year, month, 0);
    /* Get the total number of days in a month */
    this.totalDays = date.getDate();
    /* End day of month. Like Saturday is end of month etc. 0 means Sunday and 6 means Saturday */
    this.endDay = date.getDay();
    date.setDate(1);
    /* Start day of month. Like Saturday is start of month etc. 0 means Sunday and 6 means Saturday */
    this.startDay = date.getDay();
    /* Here we generate days for 42 cells of a Month */
    this.days = [];
    /* Here we calculate previous month dates for placeholders if starting day is not Sunday */
    var prevMonthDays = 0;
    if (this.startDay !== 0) {
        prevMonthDays = new Date(year, month - 1, 0).getDate() - this.startDay;
    }
    /* This is placeholder for next month. If month does not end on Saturday, placeholders for next days to fill other cells */
    var count = 0;
    // 42 = 7 columns * 6 rows. This is the standard number. Verify it with any standard Calendar
    for (var i = 0; i < 42; i++) {
        var day = {};
        /* So start day is not Sunday, so we can display previous month dates. For that below we identify previous month dates */
        if (i < this.startDay) {
            day.date = (prevMonthDays = prevMonthDays + 1);
            /* belong to next month dates. So, month does not end on Saturday. So here we get next month dates as placeholders */
        } else if (i > this.totalDays + (this.startDay - 1)) {
            day.date = (count = count + 1);
            /* belong to current month dates. */
        } else {
            day.date = (i - this.startDay) + 1;
        }
        this.days[this.days.length] = day.date;
    }
};

/* Usage below */
monthInformation.call(context, 4, 2015);

This could use some breathing space.

this.endDay = date.getDay();
date.setDate(1);
this.startDay = date.getDay();
this.days = [];

Compare it to this.

this.endDay = date.getDay();

date.setDate(1);

this.startDay = date.getDay();
this.days = [];

See how the extra vertical space calls out the change in state?

Magic Number Alert

This needs an extra variable, a comment, or maybe even both.

for(var i = 0; i < 42; i++) {

I've read over this loop at least 20 times now. I still have no idea why the number 42 is important here. Why is 42 the upper bound? The logic isn't obvious. You should find a way to make it obvious.


A little scheme programming challenge I am learning scheme to get something new from a programming language and this code below is the solution to Project Euler question 21 but the code runs 10x slower than the listed Python code when I use Chibi-Scheme. Can any Scheme craftsman refactor the code into the Scheme idiom for efficiency.

Scheme Code:

(define (sum-of-amicable-pairs n)
  (let ((sums (make-vector n)))
        (for-each
          (lambda (i)
            (vector-set! sums i
                         (reduce + 0
                                 (filter (lambda (j) (= (remainder i j) 0))
                                         (iota (+ 1 (quotient i 2)) 1 1)))))
            (iota n 0 1))

        (let loop ((res (make-vector n 0))
                   (i 0))
          (cond
            ((= i n) (reduce + 0 (vector->list res)))
            ((and (< (vector-ref sums i) n) (not (= (vector-ref sums i) i))
                  (= (vector-ref sums (vector-ref sums i)) i))
             (begin
               (vector-set! res i i)
               (vector-set! res (vector-ref sums i) (vector-ref sums i))
               (loop res (+ 1 i))))
            (else
              (loop res (+ i 1)))))))

(display (sum-of-amicable-pairs 10000))
(newline)

Python code:

def amicable_pairs(n):
    """returns sum of all amicable pairs under n. See project euler for
    definition of an amicable pair"""
    div_sum = [None]*n
    amicable_pairs_set = set()
    for i in range(n):
        div_sum[i] = sum([j for j in range(1, i/2 + 1) if i%j == 0])
    for j in range(n):
        if div_sum[j] < n and div_sum[div_sum[j]] == j and div_sum[j] != j:
            amicable_pairs_set.add(j)
            amicable_pairs_set.add(div_sum[j])
    #print sum(amicable_pairs_set)
    return sum(list(amicable_pairs_set))

Well, for one thing, the python code is using a set for its amicable_pair_set, where-as you're using a large vector and setting the n'th element to n when you need to add to the set.  It's a reasonable way to imitate a set, if your scheme doesn't have a set library; however, this situation doesn't need true set semantics.  You can use a simple list instead, so your named let becomes:

(let loop ((res '())
           (i 0))
      (cond
       ((= i n) (reduce + 0 res))
       ((and (< (vector-ref sums i) n) (not (= (vector-ref sums i) i))
         (= (vector-ref sums (vector-ref sums i)) i))
    (loop (cons i res) (+ 1 i)))
       (else
    (loop res (+ i 1)))))

This keeps it close to the python code, but you could go a step further and have res be an integer accumulator, adding to it instead of consing to it, getting rid of the need to add the list at the end.  Of course, the same optimization could be made to the python version as well.

Unfortunately, I've optimized the part of the code that runs quick :-).  The real work is being done above, when the sums vector is being populated.  Since that is a pretty direct translation, I would chalk it up to chibi's implementation of scheme versus whatever implementation of python you're using (PyPy, for instance, is probably going to be faster).  I use racket-scheme which is jit compiled.  Calling your code returns an answer in ~680ms for me.  Calling it with the default python 2.7.3 compiler gives me a result in ~1400ms.

Here's a version which only calculates the sum of its proper divisors for the numbers that are needed, and then stores them for future use.  Basically memoizing the results.  The first run is slightly faster for me ~30ms, and then subsequent runs return in less than 1ms.

(define d
  (let ((cache (make-vector 10000 #f)))
    (lambda (n)
      (or (vector-ref cache n)
      (let ((val (reduce + 0 (filter (lambda (d) (= 0 (remainder n d)))
                     (iota (quotient n 2) 1)))))
        (vector-set! cache n val)
        val)))))

(define (sum-of-amicable-pairs n)
  (let loop ((a 0)
         (sum 0))
    (if (< a n)
      (let ((b (d a)))
        (loop (+ a 1)
          (if (and (< a b n) (= (d b) a))
              (+ sum a b)
              sum)))
      sum)))

(time (sum-of-amicable-pairs 10000))


User details logging I have written this logic for logging, but is getting replication for all fields (last name, DOB,city, state, ZIP, etc). Here only validation logic is going to differ, whereas logging functionality will remain the same.
public boolean validFirstName(UserInputDetails record) {
        if (CommonUtil.isNullOrEmpty(record.getFirstName())) {
            record.setErrorCode(ErrorCodes.FIRSTNAME_NULL.getErrorCode());
            if(record.isDetailedLoggingEnabled()){
                Map<String,String> dtlLogMap = new HashMap<>(5);
                dtlLogMap.put("transId",String.valueOf(record.getTransactionId()));
                dtlLogMap.put("firstName",record.getFirstName());
                dtlLogMap.put("validationName","VALID_FIRSTNAME_CHECK");
                dtlLogMap.put("validationStatus","Errored");
                dtlLogMap.put("errorCode",record.getErrorCode());
                DetailedLogger.log(dtlLogMap);
            }
            return false;
        }

        if ( record.getFirstName().length() >=50) {
            record.setErrorCode(ErrorCodes.FIRSTNAME_INVALID_LENGTH.getErrorCode());
            if(record.isDetailedLoggingEnabled()){
                Map<String,String> dtlLogMap = new HashMap<>(6);
                dtlLogMap.put("transId",String.valueOf(record.getTransactionId()));
                dtlLogMap.put("firstName",record.getFirstName());
                dtlLogMap.put("firstNameLength",returnStringifNotNull(record.getFirstName().length()));
                dtlLogMap.put("validationName","VALID_FIRSTNAME_LENGTH_CHECK");
                dtlLogMap.put("validationStatus","Errored");
                dtlLogMap.put("errorCode",record.getErrorCode());
                DetailedLogger.log(dtlLogMap);
            }
            return false;
        }
        if(record.isDetailedLoggingEnabled()){
            Map<String,String> dtlLogMap = new HashMap<>(4);
            dtlLogMap.put("transId",String.valueOf(record.getTransactionId()));
            dtlLogMap.put("firstName",record.getFirstName());
            dtlLogMap.put("validationName","VALID_FIRSTNAME_CHECK");
            dtlLogMap.put("validationStatus","passed");
            DetailedLogger.log(dtlLogMap);
        }
        return true;
    }

as mentioned in the comments it might be helpful to extract methods to remove redundant code - you might want to read a more detailed explanation on "methods in java" to gain basic knowledge on methods.
tl;dr
"A Java method is a collection of statements that are grouped together to perform an operation"
let's collect your your statements that all checks have in common
Map<String,String> dtlLogMap = new HashMap<>(4);
dtlLogMap.put("transId",String.valueOf(record.getTransactionId()));
dtlLogMap.put("firstName",record.getFirstName());
dtlLogMap.put("validationName","VALID_FIRSTNAME_CHECK");
dtlLogMap.put("validationStatus","passed");
DetailedLogger.log(dtlLogMap);

we can extract a lot of stuff here into a method named Map<String,String> createLog(Record record, String checkResult) - noteworthy is, that we only group things together, that belong together - we do not (yet) extract the logging action - to maintain the Single Responsibility-principle. That makes it also a lot easier to find proper method names (instead of creating an obfucasting method name createAndPrintLog())
private Map<String,String> createLog(Record record, String checkName, String checkResult){
    Map<String,String> logs= new HashMap<>(4);
    logs.put("transId",String.valueOf(record.getTransactionId()));
    logs.put("firstName",record.getFirstName());
    logs.put("validationName",checkName);
    logs.put("validationStatus",checkResult);
    return logs;
}

note that a method has parameters that let you control the output of the method: the parameters should be self-explaining (Tell - dont Ask)
we can now apply this method in your checks and remove redundant code
...
if(record.isDetailedLoggingEnabled()){
    //here: method call instead of redundancy
    Map<String,String> dtlLogMap = createLogs(record,"VALID_FIRSTNAME_CHECK",  "Errored");
    dtlLogMap.put("errorCode",record.getErrorCode());
    DetailedLogger.log(dtlLogMap);
}
...

use more methods to remove more reduncancy!


Finding neighbors and returning average value from neighbors I have a list of sales records sorted by sales date. I need to fill in the gaps (days without a sales record) in this list with 'inferred sales' records. The method in question  calculates an average price for the inferred sales records from its closest valid neighbors that have the same SalesType. If there's only one neighbor then no need to average it just returns the sales price of the neighbor. I'd like to improve readability of this method.

public class SalesRecord
{
    public DateTime Date { get; set; }
    public bool IsValid { get; set; }
    public int SalesType { get; set; }
    public double UnitPrice { get; set; }
}
public class SalesDataContainter
{
    private SortedList<DateTime, SalesRecord> salesData;
    private double GetInferredPrice(SalesRecord inferredSalesRecord)
    {
        SalesRecord closestBefore = null;
        SalesRecord closestAfter = null;
        foreach (var salesRecord in salesData.Values)
        {
            //neigbor needs to be valid
            if (salesRecord.IsValid)
            {
                continue;
            }
            //has to have the same sales type
            if (salesRecord.SalesType != inferredSalesRecord.SalesType)
            {
                continue;
            }
            //listed is sorted get the last found for before neighbor
            if (salesRecord.Date < inferredSalesRecord.Date)
            {
                closestBefore = salesRecord;
            }
            //listed is sorted get the first found for after neighbor
            if (closestAfter == null && salesRecord.Date > inferredSalesRecord.Date)
            {
                closestAfter = salesRecord;
            }
        }
        // we have neighbors on both sides
        if (closestBefore != null && closestAfter != null)
        {
            return (closestAfter.UnitPrice + closestBefore.UnitPrice) / 2;
        }
        // there's only before neighbor
        if (closestBefore != null)
        {
            return closestBefore.UnitPrice;
        }
        // there's only after neighbor
        if (closestAfter != null)
        {
            return closestAfter.UnitPrice;
        }
        return 0;
    }
}

Just a few things I want to point out before tackling this:

Based on the comment, your first condition is flipped.  You're skipping valid entries instead of invalid entries.  It should be:

if (!salesRecord.IsValid)

Your last condition in the loop could just break at that point.  Since it is sorted, every valid item will be a match but you're only interested in the first.
You should consider making UnitPrice of type decimal.  You're dealing with currency here so you should not be using double.

Using a little bit of LINQ here could make this more readable IMHO.  It's easier to think of it as "looping through the list of candidate records" rather than "looping through all records and skipping some of them."

Personally, I would write the last set of conditions as I have shown using the conditional operator.  You may or may not like it this way or this structure so your choice with that part.

public class SalesRecord
{
    public DateTime Date { get; set; }
    public bool IsValid { get; set; }
    public int SalesType { get; set; }
    public decimal UnitPrice { get; set; }
}
public class SalesDataContainter
{
    private SortedList<DateTime, SalesRecord> salesData;
    private decimal GetInferredPrice(SalesRecord inferredSalesRecord)
    {
        Func<SalesRecord, bool> isValidRecord = record =>
            record.IsValid                                          //neigbor needs to be valid
            && record.SalesType == inferredSalesRecord.SalesType;   //has to have the same sales type
        var candidateRecords = salesData.Values
            .SkipWhile(record => !isValidRecord(record))
            .Where(isValidRecord);

        SalesRecord closestBefore = null;
        SalesRecord closestAfter = null;
        foreach (var salesRecord in candidateRecords)
        {
            //get the last found for before neighbor
            if (salesRecord.Date < inferredSalesRecord.Date)
            {
                closestBefore = salesRecord;
            }
            //get the first found for after neighbor
            else if (salesRecord.Date > inferredSalesRecord.Date)
            {
                closestAfter = salesRecord;
                break;
            }
        }

        // there's a neighbor before
        if (closestBefore != null)
        {
            return (closestAfter != null)
                // we have neighbors on both sides
              ? (closestBefore.UnitPrice + closestAfter.UnitPrice) / 2M
              : closestBefore.UnitPrice;
        }
        else
        {
            return (closestAfter != null)
                // there's only a neighbor after
              ? closestAfter.UnitPrice
              : 0M;
        }
    }
}

An alternative would be to let LINQ do all the work:

public decimal GetInferredPrice(SalesRecord inferredSalesRecord)
{
    Func<SalesRecord, bool> isValidRecord = record =>
        record.IsValid                                          //neigbor needs to be valid
        && record.SalesType == inferredSalesRecord.SalesType;   //has to have the same sales type

    return salesData.Values
        .SkipWhile(record => !isValidRecord(record))
        .Where(isValidRecord)
        .GroupBy(
            record => record.Date.CompareTo(inferredSalesRecord.Date),
            record => record.UnitPrice as decimal?,
            (key, g) => (key < 0) // assumes no records will have equal dates
                ? g.LastOrDefault()
                : g.FirstOrDefault()
        )
        .Average() ?? 0M;
}


Rock Paper Scissors in Java (I just want to know whatever feedback you have :) Thank you!) Any feedback on this code would be greatly appreciated! I am just trying to improve so I can move on to bigger projects. Thank you so much!

    import java.util.*;
   import java.util.Random;

   public class RockPaperScissors {
     public static void main(String [] args)
     {

    Random rd = new Random();
    Scanner kb = new Scanner(System.in);
    System.out.println("Welcome to Rock Paper Scisscors!");

    while(true)
    {
    int rock = 1;
    int paper = 2;
    int scissors = 3;

    int conditions [] = {rock, paper, scissors};
    int random = (conditions[new Random().nextInt(conditions.length)]);

    System.out.print("Please Enter: \n1 for Rock, \n2 for Paper, \n3 for Scissors: ");
    int player1 = kb.nextInt();

    if(player1 == rock && random == rock)
    {
        System.out.print("You have tied, computer rock, please try again\n");
    }
    else if(player1 == paper && random == paper)
    {
        System.out.print("You have tied, computer chose paper, please try again");
    }
    else if(player1 == scissors && random == scissors)
    {
        System.out.print("You have tied, computer chose scissors, please try again");
    }

    if(player1 == rock && random == paper)
    {
        System.out.print("Computer chose paper, computer has won");
        break;
    }
    else if(player1 == paper && random == rock)
    {
        System.out.print("You win!, computer chose rock");
        break;
    }
    else if(player1 == paper && random == scissors)
    {
        System.out.print("Computer chose scissors, computer has won");
        break;
    }
    else if(player1 == scissors && random == paper)
    {
        System.out.print("You win, computer chose paper");
        break;
    }
    else if(player1 == rock && random == scissors)
    {
        System.out.print("You win!, computer chose scissors");
        break;
    }
    else if(player1 == scissors && random == rock)
    {
        System.out.print("Computer chose rock, computer has won");
        break;
    }

    if(player1 != rock && player1 != paper && player1 != scissors )
    {
        System.out.print("Invalid response");
        break;
    }
    }

     }

   }

I have some suggestions for you.

Next time, before submitting code, try to format it :)

Suggestions

Unused variable

The variable rd is not used, you can remove it.

Put variables that don’t change out of the loop

In the while loop, the rock, paper and scissors get their value reassigned with the same value each time the loop restart, move them outside of the loop.

I suggest that you put those as class constants, so you will be able to reuse the in other methods.

public class RockPaperScissors {
    public static final int ROCK = 1;
    public static final int PAPER = 2;
    public static final int SCISSORS = 3;
}

Validate the invalid choice first

In your code, you check if the chosen option is valid at the end of the loop; it will be more efficient if you check the invalid options first. This will prevent to check every other conditions and quit the loop faster.

System.out.print("Please Enter: \n1 for Rock, \n2 for Paper, \n3 for Scissors: ");
int player1 = kb.nextInt();

if (player1 != rock && player1 != paper && player1 != scissors) {
   System.out.print("Invalid response");
   break;
}

Extract some of the logic into methods

In your code, you can move some of the logic into method to allow the code to be shorter and easier to read.

I suggest make methods to check if the answer is a rock, paper or scissors; this will allow the conditions to be shorter.

public static void main(String[] args) {
   while (true) {
      //[...]
      if (isRock(player1) && isRock(random)) {
         //[...]
      }
      //[...]
   }
}

private static boolean isRock(int answer) {
   return answer == ROCK;
}

private static boolean isPaper(int answer) {
   return answer == PAPER;
}

private static boolean isScissor(int answer) {
   return answer == SCISSORS;
}

I suggest that you extract the logic that read the value in a method.

public static void main(String[] args) {
   while (true) {
      //[...]
      int player1 = getPlayer1Answer(kb);
   }
}

private static int getPlayer1Answer(Scanner kb) {
   System.out.print("Please Enter: \n1 for Rock, \n2 for Paper, \n3 for Scissors: ");
   return kb.nextInt();
}

Bonus

While you are in the method, I suggest to make another loop to verify the validity of the answer; this will make sure the provided choice is valid and ask again if not.

private static int getPlayer1Answer(Scanner scanner) {
   int value;

   while (true) {
      System.out.print("Please Enter: \n1 for Rock, \n2 for Paper, \n3 for Scissors: ");
      value = scanner.nextInt();

      if (isRock(value) || isPaper(value) || isScissor(value)) {
         break;
      } else {
         System.out.println("Invalid response");
      }
   }

   return value;
}

I suggest that you extract the logic that generates the computer answer in a method.

    private static int getComputerAnswer() {
        int[] conditions = {ROCK, PAPER, SCISSORS};
        return conditions[new Random().nextInt(conditions.length)];
    }

Refactored code

public class RockPaperScissors {
    public static final int ROCK = 1;
    public static final int PAPER = 2;
    public static final int SCISSORS = 3;

    public static void main(String[] args) {
        Scanner kb = new Scanner(System.in);
        System.out.println("Welcome to Rock Paper Scissors!");

        while (true) {
            int random = getComputerAnswer();
            int player1 = getPlayer1Answer(kb);

            if (isRock(player1) && isRock(random)) {
                System.out.print("You have tied, computer rock, please try again\n");
            } else if (isPaper(player1) && isPaper(random)) {
                System.out.print("You have tied, computer chose paper, please try again");
            } else if (isScissor(player1) && isScissor(random)) {
                System.out.print("You have tied, computer chose scissors, please try again");
            }

            if (isRock(player1) && isPaper(random)) {
                System.out.print("Computer chose paper, computer has won");
                break;
            } else if (isPaper(player1) && isRock(random)) {
                System.out.print("You win!, computer chose rock");
                break;
            } else if (isPaper(player1) && isScissor(random)) {
                System.out.print("Computer chose scissors, computer has won");
                break;
            } else if (isScissor(player1) && isPaper(random)) {
                System.out.print("You win, computer chose paper");
                break;
            } else if (isRock(player1) && isScissor(random)) {
                System.out.print("You win!, computer chose scissors");
                break;
            } else if (isScissor(player1) && isRock(random)) {
                System.out.print("Computer chose rock, computer has won");
                break;
            }
        }
    }

    private static boolean isRock(int answer) {
        return answer == ROCK;
    }

    private static boolean isPaper(int answer) {
        return answer == PAPER;
    }

    private static boolean isScissor(int answer) {
        return answer == SCISSORS;
    }

    private static int getComputerAnswer() {
        int[] conditions = {ROCK, PAPER, SCISSORS};
        return conditions[new Random().nextInt(conditions.length)];
    }

    private static int getPlayer1Answer(Scanner scanner) {
        int value;

        while(true) {
            System.out.print("Please Enter: \n1 for Rock, \n2 for Paper, \n3 for Scissors: ");
            value = scanner.nextInt();

            if(isRock(value) || isPaper(value) || isScissor(value)) {
                break;
            } else {
                System.out.println("Invalid response");
            }
        }

        return value;
    }
}
```


Optimal BST challenge I was coding the problem here. I think I have solved it, but I ran into time limit exceeded issues. I am new in Competitive Programming, so I was looking for a better, time efficient logic for solving the same.

Time limit: 1 secs

My time: 3.01 secs

The problem in short:

  Given the numbers  [1 .. N ]  to be put in a binary search tree and a sequence of  m  searches  (\$s_1\$ , \$s_2\$ , . . . , \$s_m\$ ), solve the optimal BST problem
  with the cost as the sum of the distances between every two
  consecutive searches \$s_i\$ and \$s_{i+1}\$ on the BST.

  Cost(T,S) = Depth(\$s_1\$) + Distance(\$s_1\$, \$s_2\$) + Distance(\$s_2\$, \$s_3\$) + ... +
  Distance(\$s_{m-1}\$, \$s_m\$).

import sys

def getLevel(node, data):
    if (node.key == data) :
        return 1
    downlevel= 1+getLevel(node.left, data)
    if (downlevel != 0) :
        return downlevel
    downlevel = 1+getLevel(node.right,data)
    return downlevel

def pathToNode(node, path, k):
    path.append(node.key)
    if node.key == k :
        return True
    if ((node.left != None and pathToNode(node.left, path, k)) or
            (node.right!= None and pathToNode(node.right, path, k))):
        return True
    path.pop()
    return False

def distance(node, x, y):
    path1 = []
    pathToNode(node, path1, x)
    path2 = []
    pathToNode(node, path2, y)
    i=0
    while i<len(path1) and i<len(path2):
        if path1[i] != path2[i]:
            break
        i = i+1
    return (len(path1)+len(path2)-2*i)

class newNode:
    def __init__(self, item):
        self.key=item
        self.left = None
        self.right = None

def constructTrees(start, end):
    list = []
    if (start > end) :
        list.append(None)
        return list
    for i in range(start, end + 1):
        leftSubtree = constructTrees(start, i - 1)
        rightSubtree = constructTrees(i + 1, end)
        for j in range(len(leftSubtree)) :
            left = leftSubtree[j]
            for k in range(len(rightSubtree)):
                right = rightSubtree[k]
                node=newNode(i)
                node.left = left
                node.right = right
                list.append(node)
    return list

if __name__ == '__main__':
    b1,c1=input().split(' ')
    b1=int(b1)
    c1=int(c1)
    ar = input()  # 1 3 2 1 2
    ar = ar.split(' ')
    ar = [int(x) for x in ar]
    min= sys.maxsize
    totalTreesFrom1toN = constructTrees(1, b1)
    for i in range(len(totalTreesFrom1toN)):
        dist = getLevel(totalTreesFrom1toN[i],1)-1
        for j in range(len(ar)-1):
            dist=dist+distance(totalTreesFrom1toN[i], ar[j], ar[j+1])
        if(dist<min):
            min=dist
    print(min)

Can anyone tell me where I can possibly refine the code?

Use the properties of a BST

In a BST, if the value being search for is less than the key of the current node, then it is in the left subtree and if the value is greater than the key of the current node, then it is in the right subtree.  Your code always searches the left subtree first and then searches the right subtree if needed.  This code only searches one subtree:

def pathToNode(node, path, k):
    path.append(node.key)
    if node.key == k :
        return True

    elif k < node.key:
        if node.left and pathToNode(node.left, path, k):
           return True

    else:
        if node.right and pathToNode(node.right, path, k):
           return True

    path.pop()
    return False

The same applies to getlevel().

distance() determines the path from the root to each node. Then determines the common prefix of each path. This can be simplified as observing that if both nodes are in the same subtree, then the root of the current tree isn't on the path between the nodes.

def distance(node, x, y):
    if x < node.key and y < node.key:
        return distance(node.left, x, y)

    elif node.key < x and node.key < y:
        return distance(node.right, x, y)

    else:
        path1 = []
        pathToNode(node, path1, x)
        path2 = []
        pathToNode(node, path2, y)

        return len(path1) + len(path2) - 2

Note: you don't actually need the path of a node, just it's depth.


Image filtering application I'm building an image filtering application, and I'm new to the Java frameworks that I need to do so. Right now, I can apply a simple 3x3 kernel blur to my thing by overwriting the RGB values of the destination picture pixel by pixel using the RGB averages from the original photo. I'm trying to increase the speed at which I can perform these filter calculations so there's less lag in my app.

I've done some surface level optimization, but if there's anything you see that could better utilize the cache or an entirely different framework that's faster than BufferedImage.getRGB(), I'd love to hear!

public void boxBlur(ImageView iv) {
       for (int height = 0; height < sourceImage.getHeight(); height++) {
           for (int width = 0; width < sourceImage.getWidth(); width++) {
               int[][] kernel = new int[][]{ // Contruct RGB array to manipulate. Edges are accounted for in construction
                   {(height == 0 || width == 0) ? sourceImage.getRGB(width, height) : sourceImage.getRGB(width - 1, height - 1), (height == 0) ? sourceImage.getRGB(width, height) : sourceImage.getRGB(width, height - 1), (height == 0 || width >= sourceImage.getWidth() - 1) ? sourceImage.getRGB(width, height) : sourceImage.getRGB(width + 1, height - 1)},
                   {(width == 0) ? sourceImage.getRGB(width, height) : sourceImage.getRGB(width - 1, height), sourceImage.getRGB(width, height), (width == sourceImage.getWidth() - 1) ? sourceImage.getRGB(width, height) : sourceImage.getRGB(width + 1, height)},
                   {(height >= sourceImage.getHeight() - 1 || width == 0) ? sourceImage.getRGB(width, height) : sourceImage.getRGB(width - 1, height + 1), (height >= sourceImage.getHeight() - 1) ? sourceImage.getRGB(width, height) : sourceImage.getRGB(width, height + 1), (height >= sourceImage.getHeight() - 1 || width >= sourceImage.getWidth() - 1) ? sourceImage.getRGB(width, height) : sourceImage.getRGB(width + 1, height + 1)}
               };
               int redAvg = 0, blueAvg = 0, greenAvg = 0;
               for (int i[] : kernel) {
                   for (int j : i) {
                       redAvg += getRed(j); // Get bitwise value from RGB int
                       greenAvg += getGreen(j);
                       blueAvg += getBlue(j);
                   }
               }
               redAvg /= 9; // 9 is size of kernel
               greenAvg /= 9;
               blueAvg /= 9;
               destinationImage.setRGB(width, height, 65536 * redAvg + 256 * greenAvg + blueAvg); // setRGB() takes the integer value of an rgb color
           }
       }
       iv.setImage(updateDisplay());
   }

Throwaway memory

I haven't done any analysis on the code but based on my experience the worst part is the allocation of throwaway memory inside the innermost loop. Instead of allocating a 3x3 array 9 million times and throwing it to the garbage colletor immediately, allocate it once at the start of the method and reuse it in the loop.

int[][] kernel = new int[][] { ... }

Repeated calculations

Second obvious thing is the repeating of same mathematical operations. You calculate the right and bottom border several times inside the innermost loop even though the result never changes during the image processing. Calculate these at the start of the method and store the results to local variables:

sourceImage.getHeight() - 1
sourceImage.getWidth() - 1

Sacrifice readability for performance

After solving the obvious parts, you can start breaking good coding conventions. The only reason you have those mile long unreadable ternary operations is to handle the edge cases that affect 0.1% of your data. Take out the easy code inside the edges that covers 99.9% of your running time, that doesn't need any bounds checking, and make it as fast as possible. Then make a separate loop that handle only the edges.

Bad variable naming

The choice of loop variables are bad. Width and height are constants of the image that refer to the right and bottom border. You are using these names to refer to single pixels inside the image. The correct names here would be x and y. This makes following your code extremely difficult.

Refer to the good old StackOverflow-side of this site for more info about accessing the color values directly:

https://stackoverflow.com/questions/37779515/how-can-i-convert-an-imageview-to-byte-array-in-android-studio


HSTS Recommendations in .htaccess Please see my previous post in the below hyperlink
I've updated my .htaccess file to account for an HSTS, along with many of the recommended changes. See the snippet below. I'd like to stress that implementing an HSTS is not to be taken lightly for anybody else new to it. With that said, I'm looking for advice on what can be done differently from those who have knowledge on .htaccess.
#IMPLEMENT HSTS
<IfModule mod_headers.c>
Header set Strict-Transport-Security "max-age=63072000; includeSubDomains; preload"
</IfModule>

#CUSTOM ERROR PAGES
ErrorDocument 400 /allerror.php
ErrorDocument 401 /allerror.php
ErrorDocument 403 /allerror.php
ErrorDocument 404 /allerror.php
ErrorDocument 405 /allerror.php
ErrorDocument 408 /allerror.php
ErrorDocument 500 /allerror.php
ErrorDocument 502 /allerror.php
ErrorDocument 504 /allerror.php

RewriteEngine On

#REDIRECT TO SECURE HTTPS CONNECTION
RewriteCond %{HTTPS} off
RewriteRule ^(.*)$ https://%{HTTP_HOST}%{REQUEST_URI} [L,R=301]

#FORCE WWW TO NON-WWW
RewriteCond %{HTTP_HOST} ^www.example.com [NC]
RewriteRule ^(.*)$ https://example.com/$1 [L,R=301]

#URL EXTENSION REMOVAL
RewriteCond %{THE_REQUEST} /([^.]+)\.html [NC]
RewriteRule ^ /%1 [NC,L,R]
RewriteCond %{REQUEST_FILENAME}.html -f
RewriteRule ^ %{REQUEST_URI}.html [NC,L]

#HOTLINKING PROTECTION
RewriteCond %{HTTP_REFERER} !^https://(www\.)?example\.com(/.*)*$ [NC]
RewriteCond %{HTTP_REFERER} !^$
RewriteRule \.(css|flv|gif|ico|jpe|jpeg|jpg|js|mp3|mp4|php|png|pdf|swf|txt)$ - [F]

#CONTENT SECURITY POLICY
<FilesMatch "\.(html|php)$">
    Header set Content-Security-Policy "default-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self' 'unsafe-inline' 'unsafe-eval'; img-src 'self' data: 'unsafe-inline'; media-src 'self' data: 'unsafe-inline'; connect-src 'self';"
</FilesMatch>

#REDIRECT FOR DATE PAGE
RewriteRule ^date$ /storage/date-202010 [R=301,L]

#REDIRECT FOR HOME PAGE
RewriteRule ^home$ / [R=301,L]

#PREVENT DIRECTORY BROWSING
Options All -Indexes

#FILE CACHING
    #cache html and htm files for one day
<FilesMatch "\.(html|htm)$">
Header set Cache-Control "max-age=43200"
</FilesMatch>
    #cache css, javascript and text files for one week
<FilesMatch "\.(js|css|txt)$">
Header set Cache-Control "max-age=604800"
</FilesMatch>
    #cache flash and images for one month
<FilesMatch "\.(flv|swf|ico|gif|jpg|jpeg|mp4|png)$">
Header set Cache-Control "max-age=2592000"
</FilesMatch>
    #disable cache for script files
<FilesMatch "\.(pl|php|cgi|spl|scgi|fcgi)$">
Header unset Cache-Control
</FilesMatch>

#BLOCKS FILE TYPES FOR USERS
<FilesMatch "\.(ht[ap]|ini|log|sh|inc|bak)$">
Require all denied
</FilesMatch>

HSTS Implementation
To note a few things that I've learned while researching HSTS:

An SSL certificate is required
If your sites are available via HTTP, redirect all requests to HTTPS with a 301 Permanent Redirect.
Include the following in your .htaccess file: Strict-Transport-Security: max-age=31536000; includeSubDomains; preload. Max-age must be at least 10886400 seconds or 18 Weeks. Go for the two years value.
Add your domain to a preload list using the first link below.

I encourage everyone to read each source below for more information.

HSTS Preload List Submission
What is HSTS and Why Should I use it? | Acunetix
What is HSTS and should I implement it? | Network Dynamics
Why websites should be using HSTS to improve security and SEO

There are two aspects to HTTP Strict Transport Security (HSTS):

Implementing HSTS on your site.
Submitting your already HSTS enabled site to the HSTS preload list. This is where the "list of HSTS sites" are compiled directly into the browser, which avoids the first request ever being over HTTP (where the request is potentially vulnerable to MITM attacks).

You seem to be going straight for #2. This is not necessarily recommended. Think of the "preload list" as a one-way trip. Technically it's possible to be removed from the preload list; realistically that's not something you want to even be thinking about. (It's hard - slow - enough stepping back from HSTS-only.)
The preload list submission page itself does not recommend going straight to "preload list submission". The recommendation is to increase the max-age parameter over a period of time (months), before taking the final step to submitting to the preload list. Test test test in the meantime to ensure SSL certs are reliably renewed, no mixed content warnings etc. etc.
I would also be wary about HSTS preload list submission (or even HSTS itself to some degree) on a shared server, where you don't have full control over the SSL configuration. You don't actually state whether you are on a shared server or not, but since you are doing all this config in .htaccess, I assume that you are. If you have your own server and access to the server config then most of this should be configured in the server/virtualhost config (and it is arguably easier and more reliable to do so).
Remember, that once you have gone the HSTS route (and users have accessed the HTTPS site or you are on the "preload list") then your site can only be accessed over HTTPS. This doesn't just apply to your site, but also to any 3rd party services you might be using (mixed content browser warnings etc.).

#IMPLEMENT HSTS
<IfModule mod_headers.c>
Header set Strict-Transport-Security "max-age=63072000; includeSubDomains; preload"
</IfModule>

This sets the required HSTS HTTP response header on "most"*1 responses (but note the preload parameter, which should probably be omitted initially).
*1 However, this directive does not necessarily set the required header on all responses. A requirement of HSTS is that you also set the header on "redirect" responses (eg. www to non-www on HTTPS). Currently the above does not do this. You should be using the always condition on the Header directive to set the header on responses other than 200 OK. For example:
# Use "always" condition to set on "redirects" as well.
Header always set Strict-Transport-Security "max-age=63072000; includeSubDomains"

(I've also removed the preload parameter for now.)
You do not need the <IfModule> wrapper and should be removed. mod_headers should be enabled by default. This is your server, you know whether mod_headers is enabled or not. mod_headers must be enabled for this to work. You don't want this to silently fail should mod_headers not be available - you need notification as soon as this fails with an error in your logs.
Many articles state that you should only set the Strict-Transport-Security header on secure (HTTPS) responses. And the "preload list submission" does indeed issue a "warning" (not strictly an "error" I believe) if you send the header over HTTP as well. However, whilst it only needs to be set on the HTTPS response, compliant browsers ignore this header when sent over an unencrypted HTTP connection (to prevent MITM attacks) so it should not matter whether the header is "unnecessarily" sent over HTTP as well. This would be easier to manage in the appropriate <VirtualHost> containers in the main server config. Sending this header only on HTTPS responses in .htaccess is more complex (and hence more prone to error). You would need to employ an additional environment variable that you can use to set the HSTS response header conditionally.
For example:
# Set environment var "HSTS" if accessed over HTTPS connection
RewriteCond %{HTTPS} on
RewriteRule ^ - [E=HSTS:1]

# Conditionally set only on HTTPS connections (ie. when "HSTS" env var is set)
Header always set Strict-Transport-Security "max-age=63072000; includeSubDomains" env=HSTS

Otherwise, I believe your remaining directives are OK with regards to HSTS. But as always, test test test.

Resolve multiple (unnecessary) redirects

#FORCE WWW TO NON-WWW
RewriteCond %{HTTP_HOST} ^www.example.com [NC]
RewriteRule ^(.*)$ https://example.com/$1 [L,R=301]

#URL EXTENSION REMOVAL
RewriteCond %{THE_REQUEST} /([^.]+)\.html [NC]
RewriteRule ^ /%1 [NC,L,R]

A potential issue here (with the 2nd and 3rd canonical redirects above, after the HTTP to HTTPS redirect) is that this potentially results in two additional redirects if www + .html is requested. This could be resolved by simply reversing the two redirects and including the canonical hostname in the "URL EXTENSION REMOVAL" redirect (as mentioned in my answer to your earlier question).
For example:
#URL EXTENSION REMOVAL
RewriteCond %{THE_REQUEST} /([^.]+)\.html [NC]
RewriteRule ^ https://example.com/%1 [R=301,L]

#FORCE WWW TO NON-WWW
RewriteCond %{HTTP_HOST} ^www\.example\.com [NC]
RewriteRule (.*) https://example.com/$1 [R=301,L]

See my previous answer for an alternative approach to the "URL EXTENSION REMOVAL" redirect that addresses some potential issues.
If you are not using the www subdomain on other hostnames (eg. www.subdomain.example.com) then you could simplify the CondPattern on the www to non-www redirect to simply ^www\., ie. any requested hostname that simply starts www., rather than checking the entire hostname.

Additional Fixes
URL Extension Removal / Rewrite Loop (500 Internal Server Error)
There are a couple of other issues that I did not get round to addressing in your earlier question, not relating to HSTS, which I'll cover below...

#URL EXTENSION REMOVAL
:
RewriteCond %{REQUEST_FILENAME}.html -f
RewriteRule ^ %{REQUEST_URI}.html [NC,L]

This (and similar) is one of those code snippets that is "blindly" copy/pasted everywhere (and I mean everywhere) as the "standard" way to append (URL-rewrite) the file-extension when using extensionless URLs. However, whilst it probably "works" for your valid-URLs, it has a serious flaw when requesting invalid-URLs...
If /about.html is a valid file that you wish to serve when requesting the extensionless URL /about then it works OK. However, if I (maliciously) requested /about/ or /about/<anything> it will send your server into a spiralling rewrite-loop, resulting in 500 Internal Server Error response. The end-user should not be able to invoke such a response (potentially more vulnerable to DDOS attacks and other hostile behaviour).
This is because REQUEST_FILENAME (the mapped filesystem path) does not necessarily refer to the same public URL-path as the REQUEST_URI (requested URL-path) variable.
To resolve this, use REQUEST_URI throughout. For example:
RewriteCond %{DOCUMENT_ROOT}%{REQUEST_URI}.html -f
RewriteRule ^ %{REQUEST_URI}.html [L]

(The NC flag is superfluous on the RewriteRule directive here.)
See my answer to the following ServerFault question for more detail on this.
Hotlink Protection - Simplify Condition

#HOTLINKING PROTECTION
RewriteCond %{HTTP_REFERER} !^https://(www\.)?example\.com(/.*)*$ [NC]

This is relatively minor. The regex in the above condition can be simplified. At this point in the .htaccess file, the hostname has already been canonicalised to remove the www the subdomain, so the (www\.)? subpattern in the above is superfluous. As is the trailing (/.*)*$ subpattern, which simply matches everything else. You don't need to actually match anything here, you just need to assert that the Referer header starts with the appropriate (scheme+)hostname.
For example:
RewriteCond %{HTTP_REFERER} !^https://example\.com

The NC flag is also superfluous here. Forcing a case-insensitive match when it's not required just creates (a tiny bit) more work for your server and in some cases can open you up to vulnerabilities ("duplicate content" being a common one - although that is not an issue here).
Enabling unnecessary Options

#PREVENT DIRECTORY BROWSING
Options All -Indexes

This doesn't only prevent "directory browsing". The All argument enables a bunch of other stuff that you probably don't need, such as server-side includes (Includes) and the ability to execute CGI scripts (ExecCGI). (Incidentally, this is the only time when you can mix arguments with a + or - with those without.) To only prevent directory browsing (ie. the auto generating of directory indexes by mod_autoindex) then remove the All argument.
However, you probably only need FollowSymLinks (which might already be set in the server config), so you could set the following instead:
Options FollowSymLinks

Note the absence of + or -. This only sets FollowSymLinks, so disabling Indexes ("directory browsing") if it was already set.


Checking if a computer is part of a domain I am currently using the following code to see if the current machine has joined a domain. I was wondering if there is a more efficient way of doing this. I am currently using the JNA to receive the Secur32 instance variable.

public static String CurrentDomainName() {
        char[] userNameBuf = new char[10000];
        IntByReference size = new IntByReference(userNameBuf.length);
        boolean result = NativeMethods.Secur32.INSTANCE.GetUserNameEx
                (Secur32.EXTENDED_NAME_FORMAT.NameSamCompatible, userNameBuf, size);

        if (!result)
            throw new IllegalStateException("Cannot retrieve name of the currently logged-in user");

        String[] username = new String(userNameBuf, 0, size.getValue()).split("\\\\");
        return username[0];
    }

public static String CurrentMachineName() {
        try {
            java.net.InetAddress localMachine = java.net.InetAddress.getLocalHost();
            return localMachine.getHostName();
        } catch(UnknownHostException e) {
            e.printStackTrace();
        }
        return "Error";
    }

This is the final function that returns the result:

public static Boolean IsDomainJoined() {
        return !Windows.UserInfo.CurrentMachineName().equals(CurrentDomainName());
    }

Different approaches to determine the domain membership of a computer

Usage of command line tools

If you use command line tools you have the burden to parse the output of the process.

A naive implementation to execute command line tools in windows:

public class CmdExecutor {

    private String cmdString;

    public CmdExecutor(String cmdString) {
        this.cmdString = cmdString;
    }

    public void execute() throws IOException, InterruptedException {

        final Process process = Runtime.getRuntime().exec("cmd /C " + cmdString);

        final BufferedReader br = new BufferedReader(new InputStreamReader(process.getInputStream()));

        String line = br.readLine();

        while (line != null) {

            System.out.println(line);

            line = br.readLine();

        }

        process.waitFor();

    }

}

wmic

    public static void main(String[] args) throws IOException, InterruptedException {
        new CmdExecutor("wmic computersystem get domain").execute();
    }

systeminfo

    public static void main(String[] args) throws IOException, InterruptedException {
        new CmdExecutor("systeminfo | findstr /B /C:\"Domain\"").execute();
    }

LDAP access

If you have access to the LDAP interface of your Active Directory you can query LDAP for a specific computer account:

(&(objectClass=computer)(name=MyPC))

Comparing the approches

Using JNA and the command line approaches on the local machine can only return the local state. The only reliable information you can get from LDAP. This especially relates to disabled or deleted computer accounts.

Querying LDAP and evaluating the result (including server not reachable) may be difficult. But as the ldap schema does not change frequently you can rely on the query parameters and the result fields to remain stable.

Command line tools and Windows system libraries tend to change in usage from time to time. That is a lot more probable than in LDAP. So you have to expect to adapt the result parsing or the parameter structure.

This you can say for the command line tools as well, but there is a higher abstraction. You can deal with standard command line tools very well and easily provide alternate implementations. This is even possible with JNA but you a dealing with system libraries and not with command line tools. (My personal view on this is that command line tools are easier to handle).


Graph implementation for solving graph problems (java) I've currently started solving some competitive programming Graph questions. But Started using adjacency list as HashMap<Integer,ArrayList<Integer>> list . But in some problems, this doesn't work. so I decided to implement class Graph other than this HashMap.
I would like a review of efficiency and general code quality. Please let me know if this follow standard adjacency list implementation and best and worst-case scenarios.
Node class
class Node
{
    public int id;
    public boolean visited;
    public List<Node> adjecent;
    Node(int id)
    {
        this.id =id;
        adjecent = new ArrayList<>();
    }
}

Graph
class Graph
{
   private List<Node> nodes;
    Graph()
    {
       nodes = new ArrayList<>();
    }
    boolean check(int id)
    {
        boolean flag = false;
        for(Node temp:nodes)
        {
            if(temp.id == id)
                flag =true;
        }

        return flag;
    }
    Node getNode(int id)
    {
        for(Node temp:nodes)
        {
            if(temp.id == id)
                return temp;
        }
        return null;
    }
    void addEdge(int src,int dest)
    {

        Node s = check(src) ? getNode(src):new Node(src);
        Node d = check(dest) ? getNode(dest):new Node(dest);
        s.adjecent.add(d);
        d.adjecent.add(s);

        if(!check(src)) nodes.add(s);
        if(!check(dest)) nodes.add(d);
    }

    void print()
    {
        for(Node temp : nodes)
        {
            System.out.print(temp.id+" -> ");
             temp.adjecent.forEach(e->System.out.print(e.id+" "));
            System.out.println();
        }
    }
    void dfs(Node root)
    {
        if(root == null) return;
        System.out.print(root.id+" ");
        root.visited = true;
        for(Node t : root.adjecent)
        {
            if(!t.visited)
                dfs(t);
        }
    }
    void refresh()
    {
        for(Node temp : nodes)
        {
            temp.visited =false;
        }
    }
    void bfs(Node root)
    {
        Queue<Node> q =new LinkedList<>();
        root.visited =true;
        q.add(root);
        while(!q.isEmpty())
        {
            Node temp = q.poll();
            System.out.print(temp.id+" ");
            for(Node n:temp.adjecent)
            {
                if(!n.visited)
                    {
                        n.visited=true;
                        q.add(n);
                    }
            }
        }
    }

    public static void main(String[] args) {
        Graph g = new Graph();
        /*
         *                0
         *             /  |  \
         *            1   2   3
         *           / \   \   \
         *          4  5    6   7
         */
        g.addEdge(0, 1);
        g.addEdge(0, 2);
        g.addEdge(0, 3);
        g.addEdge(1, 4);
        g.addEdge(1, 5);
        g.addEdge(2, 6);
        g.addEdge(3, 7);
        g.print();
        System.out.println(g.nodes.size());
        g.dfs(g.getNode(0));
        System.out.println();
        g.refresh();
        g.bfs(g.getNode(0));

    }
}

Any suggestions to make this code leverage to better efficiency is appreciated.

Don't store method flow flags as state in classes. This is a breach in object-oriented design.

class Node
{
    public boolean visited;
    // .. other
}

Search methods like dfs should use a map of some sort to store which nodes are visited.

By storing this flag incorrectly as state, you'll get in trouble when multiple threads search the graph concurrently.


Haskell CodeEval Beautiful Strings Problem Definition :
When John was a little kid he didn't have much to do. There was no internet, no Facebook, and no programs to hack on. So he did the only thing he could... he evaluated the beauty of strings in a quest to discover the most beautiful string in the world.
Given a string s, little Johnny defined the beauty of the string as the sum of the beauty of the letters in it. The beauty of each letter is an integer between 1 and 26, inclusive, and no two letters have the same beauty. Johnny doesn't care about whether letters are uppercase or lowercase, so that doesn't affect the beauty of a letter. (Uppercase 'F' is exactly as beautiful as lowercase 'f', for example.)
You're a student writing a report on the youth of this famous hacker. You found the string that Johnny considered most beautiful. What is the maximum possible beauty of this string?
Your program should accept as its first argument a path to a filename. Each line in this file has a sentence.
Print out the maximum beauty for the string.

Background :
I'm still very new to Haskell, trying to wrap my head around things like monads and such, so some of the constructs might seem overly simplistic. I would like any sort of criticism from efficiency to idiomatic usage of Haskell. I harbor suspicions that my countSort function is inefficient as I would solve this using a dictionary in an imperative language but that sort of mutable solution seemed out of place in my understanding of Haskell and I couldn't decide if lazy evaluation would make up for it. Either way the speed was actually a bit faster than what I've implemented in imperative languages.
I thought about generalizing the beauty function by moving the toUpper/IsAlpha out of the method to change it's signature to something like [a] -> Int, but it felt outside of the point of the problem, and I honestly couldn't think of a use for this function outside of this problem.
Code :
import Data.List
import Data.Char
import Data.Ord
import System.Environment

countSort :: Ord a => [a] -> [[a]]
countSort x =  sortBy (flip $ comparing length) (group $ sort x)

maximumStringBeauty :: String -> Int
maximumStringBeauty x = maximumBeautyInner (countSort . map toUpper $ filter isAlpha x) 26
    where maximumBeautyInner [] _ = 0
          maximumBeautyInner (x : xs) maxVal = (length x) * maxVal + maximumBeautyInner xs (maxVal - 1)

main :: IO ()
main = do
    args <- getArgs
    let path = args !! 0
    file <- readFile path
    putStrLn . unlines . map (show . maximumStringBeauty) . lines $ file

This looks like nice Haskell to me! I'm not an expert, but I could make a few remarks:

args !! 0 could be simplified to head args. It's just more idiomatic.
maximumBeautyInner could be renamed to maximumStringBeauty'. That's what I see most of the time, anyway, in this pattern (having an outer "starter" function and an inner "real recursive" function).
I would write:

countSort = reverse . sortOn length . group . sort

because it reads nicer, but I'm not sure how expensive the reverse will be afterwards.
maximumBeautyInner can be written much more concisely when using the builtins sum, zipWith and map. When writing recursive functions, try to see if you can use these and maybe fold or another of their family.

  where maximumBeautyInner = sum . zipWith (*) [26, 25..1] . map length

Step by step:

The map length returns only the lengths of the groups (from countSort).
The zipWith combines that list with the list [26, 25..1] to form pairs, and applies * to each pair of values. So the first (highest) length gets multiplied by 26, the next one by 25, etcetera.
sum, finally, sums them all up (you'd probably guessed).

You would only pass the result of the countSort to this function, as it is now [[Char]] -> Int.

By using these list functions, you can reduce the need for your own recursive functions, which increases readability.

All in all, well-factored code, and a good way to solve this problem!

EDIT: instead of using countSort = reverse . sortOn length . group . sort, use countSort = sortOn (negate . length) . group . sort. That removes the need for reverseing the list.


Find indices of two numbers such that they add up to a target The task

...is taken from leetcode

  Given an array of integers, return indices of the two numbers such
  that they add up to a specific target.

  You may assume that each input would have exactly one solution, and
  you may not use the same element twice.

  Example:

  Given nums = [2, 7, 11, 15], target = 9,

  Because nums[0] + nums1 = 2 + 7 = 9, return [0, 1].

const lst = [11, 7, 15, 2]
const target = 9;

My imperative solution

function findIndexOfSumPair(lst, trgt) {
  const len = lst.length;
  for (let i = 0; i < len; i++) {
    const found = lst.findIndex(x => trgt - lst[i] === x);
    if (found !== i && found !== -1) {
      return [found, i];
    }
  }
}

console.log(findIndexOfSumPair(lst, target));

My functional solution

const findIndexOfSumPair2 = (lst, trgt) => {
  return lst.reduce((res, x, i) => {
    const j = lst.findIndex(y => target - x === y);
    return j !== -1 ? [i, j] : res;
  });
};

console.log(findIndexOfSumPair2(lst, target));

The disadvantage with my functional solution is that reduce iterates till the end of the array - even though it may have found a solution already. I tried to come up with a functional that stops iterating once it found a solution. I could come up with this. However it is too verbose and relies on side-effects (which doesn't make it functional anymore):

const findIndexOfSumPair3 = (lst, trgt) => {
  let res;

  lst.some((x, i) => -1 !== lst.findIndex((y, j) => {
    if (i !== j && trgt - y === x) {
      res = [j, i];
      return true;
    }
  }));
  return res;
};

console.log(findIndexOfSumPair3(lst, target));

Do you know an elegant and efficient functional solution?

Both your solutions work, however they are inefficient. A warning sign is when you need to iterate the array you are iterating. Even if the inner iteration can exit early, always consider the worst possible situation.

return lst.reduce((res, x, i) => {  // << first iteration
  const j = lst.findIndex(y => target - x === y); // << second iteration

The result is \$O(n^2)\$ time complexity

Improved search

When ever you repeat a search over the same data many times you should consider using a Map or a Set

In this case we can take advantage of the fact that the values we have already looked at provide information about the values we want.

If we have the arguments ([1,2,3,5], 7) as we step over each item we can build map of the items we want. For 1, we need a 6, 2 needs a 5, 3 needs a 4, and when you get to 5 you know you need that value and thus have a result.

Thus we get a solution that is \$O(n)\$  time, exchanging CPU cycles for memory (\$O(n)\$ storage)

Example 1

function find2SumTo(arr, k){
    const lookFor = new Map();
    var i = arr.length;
    while (i--) {
        if (lookFor.has(arr[i])) { return [i, lookFor.get(arr[i])]  }
        lookFor.set(k - arr[i], i);
    }
} // Note no final return as data is know to always have a result

The functional approach.

You don't have to use the built in array functions, you can add to the Array.prototype a more useful function.

// Finds information related to an item
Array.prototype.findInfo = function(cb) { // Must be function() to bind array to this
    var i = 0;
    while(i < this.length) {
        const result = cb(this[i], i++, this);
        if (result) { return result }
    }
}

With the new array function you can solve the problem with

const find2SumTo = (arr, k) => arr.findInfo((x, i) =>
      arr.findInfo((y, j) => {
           if (i !== j && k - y === x) { return [j, i] }
      });

However it is still an \$O(n^2)\$ time solution.

Imperative functional

The better functional approach is the first solution (Example 1). Functional does not mean you have to use lots of functions. It means that your solution must conform to the functional rules, no side effect, be pure. The function can be as complex as you need to avoid breaking the rules.


String cut by a separator with the separator in the respective list element in python Python 3.6.5

Is there any better solution than this one? Particularly the last line. I don't like it.

import re
s = "my_separator first thing my_separator second thing"
data = re.split("(my_separator )", s)[1:]
data = [even+odd for i, odd in enumerate(data) for j, even in enumerate(data) if i%2==1 and j%2==0 and i==j+1]

You can exploit zip and iterators to allow you to pair things together:

data = [a + b for a, b in zip(*[iter(data)]*2)]

You could use just re, and change the separator with a look ahead assertion.

data = re.split(" (?=my_separator)", s)

You can use str.split, and just add the separator back:

sep = 'my_separator '
data = s.split(sep)[1:]
data = [sep + i for i in data]

data = [sep + i for i in s.split(sep)]


Refactoring a TGA image parser I have followed the TGA specs described here (the info at the end of the page is what this method is based on) and have written a method that takes a TGA file's pixel data section and creates a bitmap out of it.

It works, but the code looks sloppy. Particularly, when I'm actually reading the bytes. You'll notice that I do some sort of odd foreach loop in order to repeatly add the same bytes to my array.

Here's the method that reads the pixels. It returns an array of bytes representing the pixels.

Is there a way for me to re-write the inner loop in the 128 case to NOT use a foreach in order to set the same sequence of bytes to an array? I imagine I will find myself putting bytes into an array often and it would be nice to know other ways to do it.

private byte[] getImageType10()
{
    // header stores total bits per pixel
    int bytesPerPixel = header.bpp / 8;
    int total = header.width * header.height * bytesPerPixel;

    // number of bytes we've read so far
    int count = 0;
    int repeat;
    byte packetHdr;
    int packetType;

    // temp storage
    byte[] bytes;
    byte[] pixData = new byte[total];

    while (count < total)
    {
        packetHdr = inFile.ReadByte();
        packetType = packetHdr & (1 << 7);

        // RLE packet
        if (packetType == 128)
        {
            // packet stores number of times following pixel is repeated
            repeat = (packetHdr & ~(1 << 7)) + 1;
            bytes = inFile.ReadBytes(bytesPerPixel);
            for (int j = 0; j < repeat; j++)
            {
                foreach (byte b in bytes)
                {
                    pixData[count] = b;
                    count += 1;
                }
            }
        }

        // raw packet
        else if (packetType == 0)
        {
            // packet stores number of pixels that follow
            repeat = ((packetHdr & ~(1 << 7)) + 1) * bytesPerPixel;
            for (int j = 0; j < repeat; j++)
            {
                pixData[count] = inFile.ReadByte();
                count += 1;
            }
        }
    }
    return pixData;
}

The 128 case can be optimized as:

    if (packetType == 128)
    {
        // packet stores number of times following pixel is repeated
        repeat = (packetHdr & ~(1 << 7)) + 1;
        bytes = inFile.ReadBytes(bytesPerPixel);
        for (int j = 0; j < repeat; j++)
        {
            bytes.CopyTo(pixData, count);
            count += bytes.Length;
        }
    }

The 0 case can be optimized as:

    if (packetType == 0)
    {
        // packet stores number of pixels that follow
        repeat = ((packetHdr & ~(1 << 7)) + 1) * bytesPerPixel;
        bytes = inFile.ReadBytes(repeat);
        bytes.CopyTo(pixData, count);
        count += bytes.Length;
    }


Multiply two numbers I am trying to implement multiplication of two numbers without using the * operator, as a practice for programming interviews. I have written two functions.

1) Using Long Multiplication

    static int MultiplyTwoNumbers(int num1,int num2)
    {
        List<int> leftnumber = new List<int>();
        do
        {
            leftnumber.Add(num1% 10);
            num1/= 10;
        } while (num1!=0);

        List<int> rightnumber = new List<int>();

        do
        {
            rightnumber.Add(num2% 10);
            num2/= 10;
        } while (num2!= 0);

        int result = 0;
        int counter = 1;
        for (int i = 0; i <leftnumber.Count; i++)
        {
            int tempresult = 0;
            int carry = 0;
            int innercounter = counter;
            for (int j = 0; j<rightnumber.Count; j++)
            {
                int tempVal = leftnumber[i] * rightnumber[j];
                tempVal += carry;
                if (tempVal / 10 != 0)
                {
                    carry = tempVal / 10;
                    tempVal = tempVal % 10;
                }
                tempresult = tempresult + (innercounter * tempVal);
                innercounter = innercounter * 10;
            }

            result +=tempresult;
            counter = counter * 10;
        }

        return result;

    }

2) using Addition

  static int multiplicationUsingAdd(int num1,int num2)
    {
        int result = 0;
        for (int i = 0; i < num2; i++)
        {
            int temp=result+ num1;
            result = temp;
        }
        return result;
    }

Which one is better and how can I improve it ?

You seem to be very much over-complicating this. Here is an efficient solution:

Assuming a and b are positive numbers and the result is a valid 32-bit integer value

public int Multiply(int a, int b)
{
    if (a < 0 || b < 0)
        throw new ArgumentException("Numbers must be positive");

    var min = Math.Min(a, b);
    var max = Math.Max(a, b);

    if (min == 0 || max == 0)
        return 0;
    else if (min == 1)
        return max;

    int s = min >> 1;
    int halfProduct = Multiply(s, max);

    if (min % 2 == 0)
        return halfProduct + halfProduct;
    return halfProduct + halfProduct + max;
}

So what is happening here? First we need to sort these by the smaller and larger numbers, this reduces the number of iterations required. If either of the numbers is zero, then we return zero (since x*0 == 0). If the minimum number is 1, then we return max (since x*1 == x).

Now we can apply the shift operator since shifting one to the right divides the number by 2. By recursion we keep doing this until Multiply returns 0 or max. We can then determine the value if min is even by adding the two half-products or if min is odd by adding the two half products and max.

Again, even in your original solution this will break for negative numbers. You can code around that by switching the sign (Math.Abs) and swapping the sign at the end (0 - return value, unless both are negative then you leave it positive). I'll leave that as an exercise for you.