Algorithm For Longest Palindrome I made an brute force algorithm to find the longest palindrome within the string. This palindrome is a substring of the input. Below is the code.

def generate_substrings(string):
    """
    avg time: O(len(string) * len(string))
    avg space: O(1)

    amortized time: O(len(string) * len(string))
    amortized space: O(1)
    """
    col = 0
    for row in range(len(string)):
        col = row + 1
        while col < len(string) + 1:
            yield string[row:col]
            col = col + 1

def test_generate_substrings():
    assert list(generate_substrings('')) == []
    assert list(generate_substrings('a')) == ['a']
    assert list(generate_substrings('ab')) == ['a', 'ab', 'b']

def is_palindrome(string):
    """
    avg time: O(len(string))
    avg space: O(len(string))

    amortized time: O(len(string))
    amortized space: O(len(string))
    """
    return string[::-1] == string

def test_is_palindrome():
    assert is_palindrome('') == True
    assert is_palindrome('ab') == False
    assert is_palindrome('bb') == True
    assert is_palindrome('abc') == False
    assert is_palindrome('ddd') == True

def find_palindrome(string):
    """
    avg time: O(len(string) * len(string))
    avg space: O(len(string))

    amortized time: O(len(string) * len(string))
    amortized space: O(len(string))
    """
    answer = ''
    vec = []

    for string in generate_substrings(string):
        if is_palindrome(string):
            vec.append(string)

    for string in vec:
        if len(answer) <= len(string):
            answer = string

    return answer

def test_find_palindrome():
    assert find_palindrome('ab') == 'b'
    assert find_palindrome('aa') == 'aa'

def main():
"""
entry point for test code
"""
    test_generate_substrings()
    test_is_palindrome()
    test_find_palindrome()

if __name__ == '__main__':
    main()

```

It's good to see code that comes with tests - well done!
I would add some more tests in places.  Specifically, we ought to test is_palindrome() with a single-letter input, as that's something that could easily get broken, and find_palindrome() needs testing with some strings where the longest palindrome is at beginning, middle or end.
I'm concerned at

assert find_palindrome('ab') == 'b'

There's nothing in the problem statement that says we couldn't return 'a' instead, so this is actually over-testing the code.  There's something similar going in in test_generate_substrings() where we require a specific order (we'd be better testing the result converted to set rather than list, to make the test more reasonable).

In terms of the algorithm, I see an obvious candidate for improvement:

for string in generate_substrings(string):
    if is_palindrome(string):
        vec.append(string)

for string in vec:
    if len(answer) <= len(string):
        answer = string

We store all of the palindromes in vec, even though we only want the longest one.  So we don't really need vec:
for string in generate_substrings(string):
    if is_palindrome(string) and len(answer) <= len(string):
        answer = string

We could do even better if we arrange for generate_substrings() to return all the substrings longest first.  Then we wouldn't need to generate or use all the shorter substrings:
for string in generate_substrings_longest_first(string):
    if is_palindrome(string):
        return string


Linked List Delete Node Function This code is supposed to delete every node from a linked list with a given value.

Are there any logical errors with this implementation? Did I do anything wrong or is there an area of this implementation that needs significant improvement. Any suggestions?

public boolean delete(int d) {
    Node tmpNode = head;
    Node prevNode = null;
    boolean deletedANode = false;

    if (head == null) {
        return deletedANode;
    }

    while (tmpNode != null) {
        if (tmpNode.data == d) {
            if (tmpNode == head) {
                head = head.next;
            }
            else {
                prevNode.next = tmpNode.next;
            }
         deletedANode = true;
         }

         prevNode = tmpNode;
         tmpNode = tmpNode.next;
    }

    return deletedANode;
}

Your code is buggy, if you have two Nodes with the same value in succession it will corrupt the list....

prevNode will be set to the deleted node tempNode, and if the next value also matches the input value you will be working with the wrong node as prevNode.

Also, why is d a good name for the input search value?

To avoid future bugs it is convenient to mark constant input values as final. It also can potentially help with performance

You need to change some code:

// added final, change input to `searchValue`
public boolean delete(final int searchValue) {
    Node tmpNode = head;
    Node prevNode = null;
    boolean deletedANode = false;

    /*
    This code is redundant, the while-loop does the same effective thing.
    if (head == null) {
        return deletedANode;
    }
    */

    while (tmpNode != null) {
        if (tmpNode.data == searchValue) {
            if (tmpNode == head) {
                head = head.next;
            } else { // fixed indenting/newline
                prevNode.next = tmpNode.next;
            }
            // fixed indenting
            deletedANode = true;
         } else {
             // only advance the prevNode when there's no match.
             prevNode = tmpNode;
         }
         tmpNode = tmpNode.next;
    }

    return deletedANode;
}


mIRC scripting like string token manipulation in Lua I'm trying to reproduce in Lua the  mIRC scripting manipulating tokens function:

local function tokenize(C, text)
    local char = string.format("%c", C)
    local t = {}
    for w in string.gmatch(tostring(text), "[^"..char.."]+") do
    w = tonumber(w) or tostring(w)
    table.insert(t,w)
    end
    return t
end

local function gettok(strng, position, separator, range)
  local char = string.format("%c", separator)
  local tokens = tokenize(separator, strng)
  local result, n, r, start, stop
  if (position ~= 0) then
    if (position > 0) then n = position else n = #tokens + position + 1 end
    if (range) and (position ~= range) then
      if (range > 0) then r = range

      elseif (range == 0) or ((n + range) > #tokens) then r = #tokens
      else r = n + (range + 1)
      end
      if (n == r) then
        result = tokens[i]
      else
        start = (r >= n) and n or r
        stop = (r <= n) and n or r
        for i = start, stop do
          result = (not result) and tokens[i] or tostring(result..char..tokens[i])
        end
      end
    else
      for i = 1, #tokens do
        if (i == n) then result = tokens[i] end
      end
    end
  else result = strng
  end
  return result
end

And this is the way it should work:

gettok(strng, position, separator, range)

Where

strng = string to manipulate
position = position of the token inside the string. If lesser than 0, it will be considered the position from the last token to the first. If equal to 0, returns the whole string.
separator = ASCII code of the token separator
range = optional: if specified, returns the token from position to range. If equal to 0, return all tokens from position to the end of the string.

local text = "apple.banana.cherry.grape.orange"

  apple

gettok(text,1,46)

  grape

gettok(text,-2,46)

  banana.cherry.grape

gettok(text,2,46,4)

  cherry.grape.orange

gettok(text,-1,46,-3)

Could you give me some advice on improving the code?

You don't need to use tostring inside string.match or other methods provided by string meta-table.
I'd suggest allowing users to pass the character as well as ASCII-code for the separator. This is of course, you own choice and nothing needs to be done in the code to enforce this.
The following loop:

for i = 1, #tokens do
    if (i == n) then result = tokens[i] end
end

is entirely useless and can be replaced with result = tokens[n].
I do not understand your reasons behind the following conversion:

w = tonumber(w) or tostring(w)

w will be type string by default.
You can use table.concat effectively; instead of creating string buffers.
Instead of writing long winded if-else blocks, put the smaller blocks forward and return results as soon as you arrive at one.
In lua, you do not necessarily need to put the if-conditions inside parentheses.
What does a negative range imply? I tested your function with gettok(text, -3, 46, -2) expecting cherry.grape as output, instead; I received banana.cherry.
For your start and stop values, use math.max and math.min.

The rewritten code follows. Please note that I've used my own convention of variable naming.

local function Tokenize( sChar, sInput )
    local tReturn = {}
    for sWord in string.gmatch( sInput, "[^"..sChar.."]+" ) do
        table.insert( tReturn, tonumber(sWord) or sWord )
    end
    return tReturn
end

local function GetTok( sInput, iPosition, Separator, iRange )
    local Separator = string.format( '%c', Separator )
    local tTokens = Tokenize( Separator, sInput )
    if iPosition == 0 then
        return sInput
    end
    local iStart, iStop = ( iPosition > 0 ) and iPosition or ( #tTokens + iPosition + 1 )
    if not iRange or iPosition == iRange then
        return tTokens[ iStart ]
    end
    if iRange > 0 then
        iStop = iRange
    elseif iRange == 0 or ( iStart + iRange ) > #tTokens then
        iStop = #tTokens
    else
        iStop = iStart + iRange + 1
    end
    if iStart == iStop then
        return tTokens[ iStart ]
    end
    if iStart > iStop then
        iStart, iStop = iStop, iStart
    end
    return table.concat( tTokens, Separator, iStart, iStop )
end


Deitel Java exercise 6.30: Number-guessing game I missed points for putting implementation in the driver class, rather than having the driver class only make call methods in other classes. I'm not sure how to move the implementation outside of the main class. Suggestions are welcome!

/*
*Deitel Chapter 6 Exercise 6.30
*/
package guess;

import java.util.Scanner;

/**
*Driver class for number guessing game
*/
public class GuessDriver {

/**
 *
 * @param args
 */
public static void main(String[] args) {

    Scanner input = new Scanner(System.in);
    /*
     *instantiate guess class
     */
    Guess Guess = new Guess();

    /*
    *loops program until user guesses correct number
    */
    while (Guess.getGuess() != Guess.secretNumber){
    /*
    *takes input from user
    n*/
    System.out.println("Enter a number from 1 to 1000: ");
    int setGuess = input.nextInt();
    Guess.setGuess(setGuess);

   /*
    *nested if-else statements compares guess against secret number
    */
    if(Guess.secretNumber < Guess.guess){
        System.out.println("too high");
    } else if (Guess.secretNumber > Guess.guess){
        System.out.println("too low");
     } else {
        System.out.println("Congratulations, you win!");
     }
    }
}
}

/*
*Deitel Chapter 6 Exercise 6.30
*/
package guess;
import java.util.Random;

/**
* Takes in and stores an integer, sets value for random number
*/
public class Guess {

/**
 *instance variable
 */
public int guess;

/**
 *instance variable
 */
public int secretNumber = (int) (Math.random() * 999 + 1);

/**
 *return secret number
 * @return
 */
public int getSecretNumber() {
    return secretNumber;
}

/**
 *return guess
 * @return
 */
public int getGuess() {
    return guess;
}

/**
 *set guess
 * @param guess
 */
public void setGuess(int guess) {
    this.guess = guess;
}

}

Guess Guess = new Guess();

Java's naming convention for variables are in camelCase, so I will suggest writing that as Guess guess = new Guess();.

With that said, the fact that Java being an OOP language doesn't mean that everything must be done object-style. For this relatively trivial exercise, all you need is just a random number:

int randomNumber = getRandomNumber();

And everything can be done within the main() method itself:

public static void main(String[] args) {
    int randomNumber = getRandomNumber();
    try (Scanner scanner = new Scanner(System.in)) {
        for (int guess = prompt(scanner); guess != randomNumber; guess = prompt(scanner)) {
            System.out.println("too " + (guess < randomNumber ? "low" : "high"));
        }
        System.out.println("Congratulations, you win!");
    }
}

I will suggest using try-with-resources on the Scanner instance to handle its underlying I/O resources efficiently. Instead of a 'plain' while loop, I opted to showcase how a similar loop can be done using the for-loop construct: it is somewhat more expressive, and the slight benefit is that it nicely scopes the guess variable within itself.

The implementation for getRandomNumber() and prompt(Scanner) shall be left for the reader... What I can further advise for the latter is that it should safely handle invalid inputs internally and only returns an int value for comparison.

Oh, and one more thing... you current implementation for the random number does not generate it in the range \$[1 ... 1000]\$, assuming that you really intend the range to be bound-inclusive:

(int) (Math.random() * 999 + 1)

Tip: Math.random() will not give you exactly 1.0. In addition the cast to int truncates the precision, so a value like 999.999 is rounded down to 999. You'll need an alternative way of getting 1000. :)


Von Neumann extractor to remove bias from pseudorandom array Von Neumann extraction is a technique to remove bias from a source of entropy. You extract an unbiased array of bits from a biased one by comparing pairs of consecutive bits in the array and returning the first one when they are different, or nothing when they are the same. The extracted array will be smaller than the source array, but will be more balanced in terms of overall 0s and 1s.

To accomplish this in JavaScript I ended up writing two extraction methods: one for extracting a bit (given an array and an index) and one for extracting an entire array (given an array). The code appears to work as desired, but I'd love to hear any insights or suggestions related to best practices, naming conventions, performance concerns, or problematic edge cases.

Here's my bit-extraction method:

function extractBitFromArray(arr, i) {
    var a = arr[i];
    var b = arr[i + 1];
    return (a ^ b ? a : null);
}

And here's my array-extraction method:

function extractUnbiasedArray(randomArray){
    var extractedArray = [];
    for (var i = 0; i + 1 < randomArray.length; i += 2) {
        var extracted = extractBitFromArray(randomArray, i);
        if (extracted !== null) {
            extractedArray.push(+extracted);
        }
    }
    return extractedArray;
}

For the sake of completion, here are some analogous dummy functions used to generate a biased but still pseudorandom array:

function getBiasedArray(length){
    var randomArray = [];
    for (var i = 0; i < length; i++) {
        randomArray.push(getBiasedBit());
    }
    return randomArray;
}
function getBiasedBit() {
    return +(Math.pow(Math.random(), Math.random()) > Math.random());
}

Example of how the functions would be used:

var randomArray = getBiasedArray(5000);
var extractedArray = extractedUnbiasedArray(randomArray);

I have a fiddle demonstrating how the technique works, but it's polluted with logging and metrics, and doesn't have all the logic encapsulated into functions; I'd rather any critiques be directed to the above code rather than the code in the fiddle: http://jsfiddle.net/thriggle/Ljojyn6p/

Your code looks quite clean and I have just a few observations, mainly regarding readability.

First it lacks comments, that is especially important in your case because the matter is not that common. In your question you separately gave all needed information to understand what and how it is processed, but these explanations should be included in the code, even if in a less detailed fashion.

Naming conventions respect standards but I'd suggest a different choice about what you named "bit". Technically speaking you're not managing bits but numbers, even if they only take values of 0 or 1: so at a first view it's a bit (!) confusing and you'd better using something like "digit" or "val", or somewhat else not too much related to a (here falsy) javascript type.

Yet about this fact that data are not really bits I was surprised you're using a bitwise operator in return (a ^ b ? a : null);. Maybe I'm missing some subtility but for what I can understand this XOR can be simply expressed as return (a != b ? a : null);.
Though I'm not pretty sure, it might be less detrimental to performance.

The only real fault against best practices is in your for (var i = 0; i + 1 < randomArray.length; i += 2): this way randomArray.length is evaluated for each step. The right way is for (var i = 0, l = randomArray.length; i + 1 < l; i += 2), so evaluating randomArray.length only once.

Last observation, you may save some variables and reduce the code length each time a variable is used only once, like here:

var a = arr[i];
var b = arr[i + 1];
return (a ^ b ? a : null);

that can be written:

var a = arr[i];
return (a ^ arr[i + 1] ? a : null);

(note that I only dropped the b var: also dropping a would cause arr[i] to be evaluated twice)


How to reduce responsibility of the main function in strategy pattern (python) I am trying to make an example of Strategy design pattern in Python 3.x. Below is my code. I feel like this can be improved but I don't know how. The run methods seem to take more responsibilities for testing and executing. So does __init__ function. The class takes an external function to create an object but also:

checks if input arguments for the function process are None
loads an object if exists to avoid re-calculations
checks if user wants to create an object regardless conditions
tests if an object to return is None
returns an object.

I am wondering how this can be improved. This is the code:

class Strategy(object):
    def __init__(self, filename, forcecreate=False):
        if (filename is None) or (not isinstance(filename, str)) or (filename == ""):
            raise ValueError("File name is not set")
        self.filename = filename
        self.forcecreate = forcecreate
        self.t = None
        self.output = None

    def run(self, process, *inputs):
        if not self.forcecreate:
            self.output = self.__pre()
        if not self.output is None:
            return self.output
        # Check if objects provided are defined
        if len(inputs) > 0:
            if any([i is None for i in inputs]):
                raise ValueError("One of the objects is not set")
        self.output = process(*inputs)
        self.__post()
        return self.output

    def __pre(self):
        if (Path(self.filename).is_file()):
            print("File exists")
            with open(self.filename, 'rb') as f_handler:
                return pickle.load(f_handler)
        return None

    def __post(self):
        with open(self.filename, 'wb') as f_handler:
            pickle.dump(self.output, f_handler)

I have a set of functions which process data and return objects which contain some structured data. I also save objects in order to avoid re-calculations. If a file exists, I can just load an object and return it. The number of functions is getting higher and I end up with copy-pasting. I thought that would slightly improve my code.

The strategy design pattern is useful mostly when you want to parametrize an algorithm with different behaviours. That implies you would have more than one strategy, which is not the case here. As such, calling this a strategy might be confusing.

Instead, it might be better to view your class as a decorator that can cache the result of a computation in a file. However, that would raise two questions:

Why is the cached data independent from the process that can produce the data, and why is the cached data independent of the input? For example, this behaviour might not be expected:

def multiply_by_2(*xs):
  return [2 * x for x in xs]

def add_3(*xs):
  return [x + 3 for x in xs]

cache.run(multiply_by_2, 1, 2, 3)  #=> [2, 4, 6]
cache.run(multiply_by_2, 3, 4, 5)  #=> [2, 4, 6]
cache.run(add_3,         1, 2, 3)  #=> [2, 4, 6]

That the function may be changed can be prevented by providing the process to the constructor, i.e. associating a cache file with a function before your cache is queried. A different function would then get a different cache.

That the data may vary between invocations is difficult to prevent unless you also store the input data in the file. You may be interested in looking at the functools.lru_cache() decorator that can cache the last n calls in memory, and looks up the correct cache entry by argument values.
If the cache object decorates an existing function (and effectively just offers a single public method run(), then it may be convenient to let the cache also offer a function-like interface: either by implementing a __call__() method, or by actually just using nested functions. With the latter approach, you might implement a decorator like:

def cache(filename, forcecreate=False):
  def inner(process):
    def cached(*input):
      ...  # basically your run() method
    return cached
  return inner

Used like:

@cache("foo.data")
def cache_me(a, b, c):
  ...

result = cache_me(1, 2, 3)

or:

cached_multiply_by_2 = cache("multiply by 2.data")(multiply_by_2)
result = cached_multiply_by_2(1, 2, 3)

It is also possible to do this with an object with a __call__() method. Such an object is actually preferable for debugging since you can inspect the properties of an object.

Other notes:

The self.t field is never read. It can be safely removed.
While __post() reads from self.output, the corresponding __pre() method does not write to that variable, returning a result instead. It would be better if either both methods directly access the self.output state or both methods are "pure" so that __post() would take an parameter.
The names of pre and post are slightly confusing. I would associate them with asserting preconditions and postconditions. Instead, they load() and store() cached data.
Your cache format is unable to cache a None value. When a None is loaded, the process is re-run again. Instead, load/store the data within a container, e.g. a single-element list. Then an empty list indicates the absence of a value, whereas a single-element list (even if that element is None) indicates the presence of a value.
if not self.output is None: would usually be written as if self.output is not None:.
The outer parentheses in if (Path(self.filename).is_file()): are unnecessary.
Output like print("File exists") deep inside your code is not a good idea. If you want to do logging, use a dedicated logging function that is switched off by default. If you output logging, prefer writing to sys.stderr rather than sys.stdout.
any([i is None for i in inputs]) creates a list unnecessarily. A generator expression is sufficient: any(i is None for i in inputs).
It is weird that your class checks the arguments for None-ness. If the wrapped process() cannot handle nones it should check that itself. Or you could define a separate decorator that you could apply independently:

def all_not_none(function):
  @functools.wraps(function)
  def inner(*args):
    if any(x is None for x in args):
      raise ValueError(...)
    return function(*args)
  return inner

(Using the functools.wraps decorator fixes docstrings and names of the returned function object to imitate the original function more closely.)


Pivotting SQL Table Response to JSON in JavaScript Issue

Our API returns two formats JSON and this SQL table format. I'm not sure what it is truly called so I'll give a few examples below. Like all API responses, it can be a list or a single response but it is always in the same format. I'm creating a utility function for this SQL table format to be converted into a JSON object so the front-end only needs to be concerned with a single format, JSON.

Problem

Convert SQL Table Response to JSON

Examples of the SQL table format

Here is the SQL table format as a success message.

{
  "COLUMNS":["SUCCESS","RETMESSAGE"],
  "DATA":[[true,"Order Approved"]]
}

Here is an example of a list

{
  "COLUMNS":["PRODUCTID","PRODUCTNAME"],
  "DATA":[[1001,"Product1"],[1002,"Product2"],[1003,"Product3"]]
}

Current Solution

The current solution is the following. It works well but I keep coming back to it and thinking there is a more elegant way of writing this.

const request = {
        "COLUMNS":["SUCCESS","RETMESSAGE"],
        "DATA":[[true,"Order Approved"]]
      };

const desiredFormat = [{
        "SUCCESS":true,
        "RETMESSAGE":"Order Approved"
      }];

function tableToJSON (dataset) {
    const data = dataset.DATA;
    const columns = dataset.COLUMNS;
    const jsonData = [];
    data.forEach( (row) => {
      const json = {};
      row.forEach( (item, index) => {
        json[columns[index]] = item;
      });
      jsonData.push(json);
    });
    return jsonData;
  }

const formattedResponse = tableToJSON(request);

console.log(JSON.stringify(formattedResponse) === JSON.stringify(desiredFormat))
// Outputs: True

Whenever you have code that sets up an array, then has a loop to push items into that array, Array.map() could be used to condense that code. For example, these lines:

const jsonData = [];
data.forEach( (row) => {
  const json = {};
  row.forEach( (item, index) => {
    json[columns[index]] = item;
  });
  jsonData.push(json);
});
return jsonData;

Could be simplified to this:

return data.map( (row) => {
  const json = {};
  row.forEach( (item, index) => {
    json[columns[index]] = item;
  });
  return json;
});

You could optionally simplify the inner loop using Array.reduce()

return data.map( (row) => {
  return row.reduce( (json, item, index) => {
    json[columns[index]] = item;
    return json;
  }, {});
});

There are some good exercises in Functional Programming in Javascript where you practice implementing some of those methods.


Should i throw exceptions in the service layer or repository, and catch it on the controller? I read some answers here on stack exchange, but none really "hits the nail on the head" about where to throw exceptions, where I shouldn't and where to catch them.
the idea that an exception should be thrown in exceptional cases is very vague, for example, if I can't find a user in my data source (repository) I can't continue the flow of my code, can this be considered exceptional?
I also understand that there are caveats about flow control with exceptions, for me it does not seem right to use try/catch within the service, to catch all the exceptions that a repository/adapter/whatever may throw to control what to do next.
based on this, a code example with a "service" and a controller calling the service and catching all of the service exceptions:
Service:
class CheckUserUltraSecretInformation
{

    public function __construct(
        private CachedUserDataRepository $cachedDataRepository,
    )
    {
    }

    public function handle(string $document)
    {
        $cachedUserData = $this->cachedDataRepository->findByDocument($document);

        if (empty($cachedUserData)) {
            throw new CachedUserDataNotFoundException('User Cached information not found, cannot proceed');
        }

        if ($cachedUserData->hasExpired()) {
            throw new CachedUserDataHasExpiredException('Cached user data is expired');
        }
        // return ultra secret information
    }
}

Controller:
class UserSecretInformationController
{

    public function __construct(
        private CheckUserUltraSecretInformation $checkUserUltraSecretInformation)
    {
    }

    public function checkUserInformation(Request $request)
    {
        $document = $request->input('document');

        try {
            $userUltraSecretInformation = $this->checkUserUltraSecretInformation->handle($document);
        } catch (CachedUserDataNotFoundException $exception) {

            // return json response with http code 400

        } catch (CachedUserDataExpiredException $exception) {
            //returns json response with http code 500
        }
    }
}

Is it "correct" that my repository does not throw exceptions, and only returns null/false when it cannot return an entity? if not, what is the problem?

if my repository throws exceptions, should my service capture them and re-launch them as service exceptions? is this considered flow control?

Are the outer layers the "most correct" to capture and handle exceptions? in my example the controller is used, can It know the name of the services/use-cases exceptions?

the idea that an exception should be thrown in exceptional cases is very vague, for example, if I can't find a user in my data source (repository) I can't continue the flow of my code, can this be considered exceptional?

My personal approach is I raise an exception if encounter a scenario where during code execution, I make a truthy assumption but that assumption turns out to be falsey. So in your example, if you can't continue the expected code execution path without a valid user object returned from the repository, throw an exception at the point where your having a user assumption turns out to be false.
Where to initially throw and exception and where to catch depends on the use case. The rule of thumb I tend to follow is thrown an exception as soon as I can't continue or recover the desired code execution path, then allow that exception to propagate back up the stack to a place that understands how to handle that exception. Don't catch an exception if all you're going to do is re-throw.

Is it "correct" that my repository does not throw exceptions, and only returns null/false when it cannot return an entity? if not, what is the problem?

I would say this is acceptable. There might be occasions where a falsey response from your repository is not exceptional and is perfectly acceptable. I would also argue it's not the responsibility of the repository to decide if a falsey response is acceptable or not (for example when checking the existence of a record).

if my repository throws exceptions, should my service capture them and re-launch them as service exceptions? is this considered flow control?

Catch an exception where you intend to handle that exception. Sometimes it does make sense to catch and re-throw an exception, such as in cases where you want to add information to the exception or cast it to a more meaningful exception for the caller above.

Are the outer layers the "most correct" to capture and handle exceptions? in my example the controller is used, can It know the name of the services/use-cases exceptions?

Catch and handle where it makes sense. In your case it makes sense as the controller is the last caller before returning a response to the client and it's not the responsibility of your service to format an error response for the caller.


Counting the number of click on the entire page In an interview I was giving the following problem:

  Count the number of click on on the entire page.

Can you think of a better way to do this?

let counter = 0;
document.addEventListener('click', clickCounter);

function clickCounter() {
  counter++;
  console.log(counter);
}

From my experience, this is a good first cut solution for a less experienced candidate as opposed to some experienced candidate. Also, a lot of other info is missing in question such as was there any further discussion on how to improve the solution or about adapting some good practices or how to make this code bug free etc.

I will try to list down possible issues which can to addressed:

The counter variable is a global variable here which is not recommended
The function is not reusable. Suppose, I want to track clicks in 2 different sections in same document now, the approach will fail
Any user can easily modify counter value. Anytime in window, user can do counter = 0 and the value will reset to 0 losing data
What if user refreshes the pages? In that case, do we need to store the data or losing it would be fine
If some additional functionality needs to be added (send total counter to backend server etc), so how easily it can be added without modifying the existing code

A lot of things depends on how the discussion goes after you present the solution. You can use closures, class based approach etc. I will try to list them below.

1. Use closure

(Not the best approach but better than OP's)

const clickCounter = clicker()
document.addEventListener('click', clickCounter);

function clicker() {
  let counter = 0
  return function() {
    ++counter
    console.log(counter)
    return counter
  }
}

2. Use class like approach

This is the one people generally expect and works best; covering most of the cases
You can also do the same by using ES6 classes
Adding new functionality is lot more easier here

function Counter() {
  let clicks = 0
  this.registerClick = function() {
    ++clicks
    console.log(clicks)
    return clicks
  }
}

const clicker = new Counter()

document.addEventListener("click", function() {
  clicker.registerClick()
})

Hope it helps. Revert for any doubts.


Buttons that expand or collapse all the  within the document I have <button>s that when pressed expand or collapse the contents of all the  within the document. The code works, but as a novice in JavaScript, I am not very confident in its performance and/or efficiency. Could you please review it.

expandDetBtn = document.getElementById("showBtn");
collapseDetBtn = document.getElementById("hideBtn");

expandDetBtn.addEventListener("click", function expandDetails(x) {
  var x = document.getElementsByTagName("details");
  var i;
  var len = x.length;
  for (i = 0; i < len; i++) {
    if (x[i].open == false) {
      x[i].setAttribute("open", "true");
    }
  }
});

collapseDetBtn.addEventListener("click", function expandDetails(x) {
  var x = document.getElementsByTagName("details");
  var i;
  var len = x.length;
  for (i = 0; i < len; i++) {
    if (x[i].open) {
      x[i].removeAttribute("open");
    }
  }
});
<button id="showBtn">Show details</button>
<button id="hideBtn">Hide details</button>

  Section 1

Text 1

  Section 2

Text 2

  Section 3

Text 3

Performance-wise the code is fine. There is not really anything to improve. I have a few other comments about the code.

You are accessing DOM elements at the start, without checking if they are loaded. Usually they are not, which would cause an error here.

You are naming your handler function, but never refer to them. You can just use an anonymous function here. You also don't use the x parameter.

Don't use var, use let and const instead.

x is not a very descriptive name. It also overwrites the exiting parameter.

There is no need to save the length in a variable. You also don't need to declare i before the loop, since it's not used outside.

There is no need to check if they are already opened or closed. Opening one which is already open doesn't do anything, so just open them all.

Don't use "true" here .setAttribute("open", "true");. According to this.

  Boolean attributes are considered to be true if they're present on the
  element at all, regardless of their actual value; as a rule, you
  should specify the empty string ("") in value

The outcome is the same, but it might lead people to believe that setting the value to "false" will remove it, which is not the case.

With all these changes, the opening look like this

expandDetBtn.addEventListener("click", function() {
  const elements = document.getElementsByTagName("details");
  for (let i = 0; i < elements.length; i++) {
    elements[i].setAttribute("open", "");
  }
});

If you want to get more advanced you could use a single function, since they are very similar.

function toggleDetails(open) {
  for(element of document.getElementsByTagName("details")) {
    element.open = open;
  }
}

expandDetBtn.addEventListener("click", toggleDetails.bind(null, true));
collapseDetBtn.addEventListener("click", toggleDetails.bind(null, false));


Optimising Lucky Number Program In this, we are given an array of numbers, say 1 2 3 4.

We start from a given position, let's say the 1st position.
We cancel that number and move forward that many non-cancelled numbers. The number on which we stop again has the same procedure.This is repeated until only one number is left. That is the lucky number.

1 2 3 4

2 3 4

3 4

3

My code:

import java.util.*;

class ln
{
    public static void main(String[] ar)throws Exception
    {
        Scanner sc = new Scanner(System.in);
        System.out.print("How many nos. will you enter: ");
        int n = sc.nextInt();
        int[] a = new int[n];
        System.out.print("Enter the nos.: ");
        for(int i = 0; i < a.length; i++)
            a[i] = sc.nextInt();
        System.out.print("From which position do you want to start: ");
        int st = sc.nextInt();
        for(int i = 0; i < a.length - 1; i++)
        {
            int j = a[st - 1];
            a[st - 1] = 0;
            int s = (int)Math.signum(j);
            while(j != 0)
            {
                if(a[st-1] != 0)
                    j -= s;
                st += s;
                if(st > a.length)
                    st = 1;
                else if(st < 1)
                    st = a.length;
            }
        }
        for(int i = 0; i < a.length; i++)
        {
            if(a[i] != 0)
            {
                System.out.println("Final no.: " + a[i]);
                break;
            }
        }

    }
}

Please tell if there is a more elegant way of doing this .

Yes, there is a more elegant way of doing this. (That's all you wanted to know, isn't it?)

First of all, here's some general comments about your code:

Your Scanner should be closed when you are done with it. sc.close();
You use in total six different names for variables. All of these have a variable name of one or two single characters. It is hard to understand their purpose because of the short variable names you have given them. Possible better names could be currentIndex, sign, currentNumber...
Use methods. A method should do one thing and it should do that one thing well. Your main method currently does:

Receive user input and put it into an array
Determine the "final number" according to your rules for this array
Display result to user

At least make a separate method for determining the final number for an array. The method could be something like this: public static int finalNumber(int[] inputArray)

Use an if-statement to check if you are currently processing the last number. This will get rid of the last for-loop in your current code, which only loops through the array and shows the non-zero number it encounters.

There is some things that can be simplified within your algorithm as well.

loop x number of times, where x equals array size
get the value at the current index
get the sign of the current value
modify the current index by + current value
while the value at the current index is zero, modify the current index by the value of sign

When modifying the index, you can use the modulo % operator to make sure that it is within the bounds of the array.

Here is what I would do:

public static int luckyNumber(int[] a, int index) {
    List<Integer> ints = new ArrayList<Integer>(a.length);
    for (int i = 0; i < a.length; i++) {
        // Add the values to a list so that it is easy to remove them later
        ints.add(a[i]);
    }
    int lastValue = 0;
    while (!ints.isEmpty()) {
        // Save the index that should be removed later
        int oldIndex = index;
        int currentNumber = ints.get(index);
        index = index + currentNumber;

        // Make sure that the index is within a valid range
        if (index < 0)
            index += ints.size() * Math.abs(index);
        index = index % ints.size();

        // Remove the old index and adjust the index value if the index removed was before the current index.
        lastValue = ints.remove(oldIndex);
        if (oldIndex <= index) index--;
    }
    // Once the list is empty, return the last value that was removed
    return lastValue;
}

All these tests will print the lovely number 42:

System.out.println(luckyNumber(new int[]{ 1, 2, 42, 4 }, 0));
System.out.println(luckyNumber(new int[]{ 1, -1, 42, 4 }, 0));
System.out.println(luckyNumber(new int[]{ 1, -1, 2, 42, -4 }, 1));


FizzBuzz in JavaScript using Ramda Just getting up to speed in Ramda, I have implemented this:

var R = require('ramda');
var divisibleBy = (b) => R.compose(R.equals(0), R.flip(R.modulo)(b))
var fizzbuzz =  R.map(R.cond([
  [R.both(divisibleBy(3), divisibleBy(5)), R.always('FizzBuzz')],
  [divisibleBy(3), R.always('Fizz')],
  [divisibleBy(5), R.always('Buzz')],
  [R.T, R.identity]
]));

console.log(fizzbuzz(R.range(1,101)))

Can I improve this, or is there a more canonical way of doing this in Ramda?

This got some discussion in the Ramda Gitter Room.  Although several people had different suggestions, my feeling -- and I believe the consensus -- was that this is very well done, and there's little to improve.

If you want it slightly more points-free, you could write divisibleBy like this:

var divisibleBy = R.curry(R.pipe(R.flip(R.modulo), R.equals(0)));

But, no, I don't think there is any more canonical way of doing this in Ramda.

BTW, I'm one of the original authors of Ramda, and as far as I've seen, yours is the very first Ramda fizzbuzz in existence!


Avoiding null in variable assignment I want to get empty string or the string value of the object

Which code you will use and why?

value = object.to_s

or this

value = object.nil? ? "" : object

If object is either nil or a string, you can just do value = object || "".

If it can be anything and you want to get a string, your second solution doesn't actually do what you want, since it won't turn the object into a string if it's not nil. To fix that your second solution would become value = object.nil? ? object.to_s : "". Of course since now you're calling to_s in both solutions there is no reason to prefer the second over the first, so I'd go with the first.


First Python game of Tic-Tac-Toe I'm just getting started with Python and was hoping for some feedback on a simple Tic-Tac-Toe game I wrote. Specifically, is there a simpler way to check the win conditions for the game, and to break the loops at the end without repeating the code twice, and just generally how to shorten this to achieve a similar effect?

print("Player 1 is 'X' and Player 2 is 'O'.\nEnter a number (0-8) to choose a \
space on the board.\nTake turns entering values until someone wins.\n\n[0, 1, 2] \
\n[3, 4, 5]\n[6, 7, 8]")

class Player:
    def __init__(self,num,XO):
        self.num = num
        self.XO = XO

p1 = Player(1,'X') #odd
p2 = Player(2,'O') #even

plist = [p1,p2]

b = [['0','1','2'], ['3','4','5'], ['6','7','8']]
i = 0

while True:
    for plyr in plist:
        while True:
            try:
                p = int(input(f'Player {plyr.num}, enter a number: '))
                row = int(p/3)
                cel = p%3
                if b[row][cel] is not 'X' and b[row][cel] is not 'O':
                    b[row][cel] = plyr.XO
                    break
                else:
                    print(f"Space already taken by {b[row][cel]}")
            except ValueError:
                print("That's not a valid number. Try again and choose 0-8.")

        bf = f"{b[0]}\n{b[1]}\n{b[2]}"
        print(bf)

        i+=1
        def CheckWin(b):
            if b[0][0]==b[0][1]==b[0][2]==plyr.XO or b[0][0]==b[1][0]==b[2][0]==plyr.XO or \
            b[0][0]==b[1][1]==b[2][2]==plyr.XO or b[0][1]==b[1][1]==b[2][1]==plyr.XO or \
            b[1][0]==b[1][1]==b[1][2]==plyr.XO or b[2][0]==b[2][1]==b[2][2]==plyr.XO or \
            b[0][2]==b[1][2]==b[2][2]==plyr.XO:
                print(f"Player {plyr.num} ('{plyr.XO}') wins!")
                return 1
            else:
                pass

        win = CheckWin(b)
        if not win and i<9:
            pass
        elif not win and i==9:
            print('The match is a tie.')
            break
        else:
            break
    if not win and i<9:
        pass
    elif not win and i==9:
        break
    else:
        break

First off, I see no reason to define p1 and p2, then put them into the playerlist, it would be more concise to simply define them inside plist's definition.

eg.

plist = [Player(1,'X'), Player(2, 'O')]

As for the repetition of the if statements, you could set a global variable "gameOver" that gets set true upon a game ending event.

eg.

#Before loops
gameOver = False

...

    #In for loop
        if not win and i<9:
            pass
        elif not win and i==9:
            gameOver = True
            print('The match is a tie.')
            break
        else:
            gameOver = True
            break
    #In first while loop
    if gameOver:
        break
    else:
        pass

This last part is just personal opinion, but I feel like asking the user for row and column would be more intuitive than cell number.

Edit: I just realized you could just do while not gameOver, no need for the if else.


Single player Hangman in Python This program is yet to become user-friendly and is still in development, but I wanted to improve the current code.

from random import choice
from os import system

clear = lambda: system('cls') # FOR WINDOWS ONLY

words = [i.strip() for i in open('words.txt').readlines()]

# Possible stages
STAGES = [
"""

    -------
""",

"""
    _______
    |   |
    |
    |
    |
    |
    -------
""",

"""
    _______
    |   |
    |   O
    |
    |
    |
    -------
""",

"""
    _______
    |   |
    |   O
    |   |
    |
    |
    -------
""",

"""
    _______
    |   |
    |   O
    |  /|
    |
    |
    -------
""",

"""
    _______
    |   |
    |   O
    |  /|\\
    |
    |
    -------
""",

"""
    _______
    |   |
    |   O
    |  /|\\
    |  /
    |
    -------
""",

"""
    _______
    |   |
    |   O
    |  /|\\
    |  / \\
    |
    -------
"""]

class HangMan:
    def __init__(self):
        self.current_stage = 0                              # The stage in which the hangman is
        self.letters = [chr(i) for i in range(97, 97 + 26)] # Possible characters
        self.word = choice(words)                           # Random word
        self.current_word = ['_'] * len(self.word)          # The letters which the player has unlocked till now

    def get_input(self) -> int:
        """ Takes the player's input and processes it """

        print(*self.letters)
        print()

        char = input('Please enter a letter from the above set: ')

        while char.lower() not in self.letters:
            print(f'Invalid letter: {char}\n')
            char = input('Please enter another letter: ')

        print()
        return self.process_char(char)

    def process_char(self, given_char: str) -> int:
        """

        Processes the input and returns 0, 1, or 2

        0 -> Win
        1 -> Neither a win nor a loss
        2 -> Lose

        """

        self.letters.remove(given_char.lower())
        unlocked = 0

        for index, char in enumerate(self.word):
            if char.lower() == given_char.lower():
                self.current_word[index] = char
                unlocked += 1

        if '_' not in self.current_word:
            print('You\'ve won!')
            return 0

        if not unlocked:
            print('Whoops! Wrong letter!')

            if not self.next_stage():
                clear()
                print(STAGES[self.current_stage])
                print('You\'ve lost!')
                return 2
        else:
            print(f'Yay! You\'ve unlocked {unlocked} letter{"" if unlocked == 1 else "s"}!')

        return 1

    def next_stage(self) -> bool:
        """ Returns if this stage is the last """

        self.current_stage += 1

        if self.current_stage < len(STAGES) - 1:
            return True

        return False

    def game(self) -> None:
        """ Starts the game """

        while True:
            clear() # Comment if you are not using a command console

            print('Currently unlocked letters:', ''.join(self.current_word))
            print()
            print(STAGES[self.current_stage])

            x = self.get_input()

            if x == 0 or x == 2:
                print(f'The word was {self.word}!')

            print()
            input('Press enter to continue...')

            if x == 0 or x == 2:
                return

man = HangMan()

man.game()

How do I improve this? (Make this look better, make it faster, etc.)

Your code is good but it may be overly complex. The more concise the code the easier it is to read (for me anyway). In this example below I have just used some example words, this code of course is just a quick example for demonstration. There is not anything in place if you get the word or want to guess the word etc. I couldn't really find a reason to use a class for simpler games because although the functions are 'closely related' in your functions, comprehension can save a lot of time and code.

The while loop is useful because it gives a consistent input option which can be as user friendly as you make it. I think its essential for the users experience when making something like a text RPG.

import random

words = ['cheese', 'biscuits', 'hammer', 'evidence']
word_choice = [i.lower() for i in random.choice(words)]
lives = 3
cor_guess = []
while True:
  print('Letters found: {}\nWord length: {}'.format(cor_guess, len(word_choice)))
  if lives == 0:
      print('Your dead homie!\nThe word was: ', word_choice)
      break
  guess = str(input('Guess a letter: '))
  if guess in word_choice:
      print(guess, 'is in the word!\n')
      cor_guess.append(guess)
  else:
      print('Your dying slowly!\n')
      lives -= 1

Example of the output:

Letters found: []
Word length: 8
Guess a letter: v
v is in the word!

Letters found: ['v']
Word length: 8
Guess a letter: i
i is in the word!

Letters found: ['v', 'i']
Word length: 8
Guess a letter: r
Your dying slowly!

Letters found: ['v', 'i']
Word length: 8
Guess a letter: b
Your dying slowly!

Letters found: ['v', 'i']
Word length: 8
Guess a letter: o
Your dying slowly!

Letters found: ['v', 'i']
Word length: 8
Your dead homie!
The word was:  ['e', 'v', 'i', 'd', 'e', 'n', 'c', 'e']


A Person class containing a list of Persons What do you guys think about this? I myself think it is pretty usefull, But how does it look from an OOP perspective?

Here's an example class

public class Person
{
    private string firstName, lastName;
    private int id;
    private static int statId;
    private static List<Person> persons = new List<Person>();
    public Person(string firstName,string lastName)
    {
        this.firstName = firstName;
        this.lastName = lastName;

        //Like this, everytime a new object of Person is created, the ID will be the id of the last created object +1
        id = statId;
        statId++;

        //Everytime you create a new instance of this class, it will be added to the list
        persons.Add(this);

    }
    public static Person GetPersonById(int id)
    {
        foreach(Person per in persons)
        {
            if (per.ID == id)
                return per;
        }
        return null;
    }
    public int ID
    {
        get { return id; }
    }

    public static List<Person> GetPersons()
    {
        return persons;
    }
    public string FirstName
    {
        get { return firstName; }
        set { firstName = value; }
    }
    public string LastName
    {
        get { return lastName; }
        set { lastName = value; }
    }
}

Now, aside from being able to use this class to store persons, you can also use some methods like GetPersonById and GetPersons / the PersonList property without instantiating it, and whenever you do instantiate it to make a new person, it automatically adds it to the list

But how does it look from an OOP perspective?

Not very good because it combines multiple responsibilities in a single class.

the one of a data object as the Person is clear, I guess. The other two are
the repository represented by

private static List<Person> persons = new List<Person>();

public static Person GetPersonById(int id)
{
    foreach(Person per in persons)
    {
        if (per.ID == id)
            return per;
    }
    return null;
}

public static List<Person> GetPersons()
{
    return persons;
}

and the factory represented by

private static int statId;
statId++;

This means that the Person should actually be three classes.

On top of all this you cannot test it because the nested factory is a static one and if you try to write a test validating the statId++ you couldn't because you cannot verfiy that it'll do +1 to the last id because if any other test executes at the same time the result might actually be +2 or even more. You also cannot verify that it correctly adds the new instance to the list because agian, if any tests execute at the same time you could have more then one object added to the list.

id = statId;
statId++;

I find this is not how the id increment should be implemented. statId should not be the next index but the last one. What if you decide to add a property like LastId? You'd have to calculate it first with statId - 1 - ugly. Instead use the prefix ++ like id = ++statId and start with int statId = -1; that is usually considered as na invalid index in many APIs. Then the LastId property could just be LastId => statId;.


Implementation of Luhn Check Algorithm I am beginner a 1st year cs student. I am taking CS50 and I completed the first week problem of implementing  Luhn Check algorithm. Though my solution  works I feel it's very sloppy and not efficient. Can someone please suggest how can I improve my solution? (It's a first-week problem so you are not intended to use array or functions.)

Here is the code:

#include<stdlib.h>
#include<cs50.h>
int main()
{
    long long x,j=0;
    int len,i,len_flag,flag=0,flag_val=0,sp_len=0,check_flag=0; //basic integers related  to loop etc
    int res,sum,fin_sum=0,h=0,d2_sum=0,d1_sum=0,ff_num=0; // variables  related to extracting numbers
    int sum_len=0,in_sum=0,in_fsum=0,len_sum=0,m=0; // extraing numbers from more than single  sum result
    int sum_f=0,sum_final=0;

    do{
        x = get_long("enter a valid card number \n");
        j=x;
        len_flag = 0;
        len = 0;
        while(len_flag!=1)
        {
          x = x / 10;
          if(x==0)
          {
            len_flag = 1;   //finding the lenght of the number
          }
          len++;
        }
        if(len==15||len==16||len==13)
        {
          flag = 1;
          sp_len =1;
        }
        else
        {
            flag=1;
        }

    }while(flag!=1);

    for(i=0;i<len;i++)
    {
       res = j % 10;
       j = j / 10;
       if(i%2!=0)
       {

        sum = res * 2;
        //printf("multi_res : %d \n",sum);
        len_flag = 0;
        sum_len = 0;
        len_sum = sum;
        while(len_flag!=1)
        {
            len_sum = len_sum / 10;
            if(len_sum==0)
            {
               len_flag=1;       // checking if the sum is more than single  digit
            }
            sum_len++;
            //printf("trig\n");
        }
        if(sum_len>1)
        {   x=0;
            while(x<sum_len)
            {
             in_sum = sum % 10;
             sum = sum/10;
             in_fsum = in_fsum + in_sum;
             //printf("double sum : %d \n",in_fsum);
             x++;

            }
        }

        fin_sum = fin_sum + sum;

       }
       if(i==len-1)
       {

         for(h=0;h < 1;h++)
         {
            fin_sum = fin_sum + in_fsum;
         }

         d1_sum = res;
       }

       else if(i%2==0)
       {
        sum_f = sum_f + res; // adding up the number that where not  x2
       }

       if(i==len-2)
       {
         d2_sum = res;     //5555555555554444
       }

    }

    sum_final = sum_f + fin_sum;

    //printf("sum_final : %d\n",sum_final);
    //printf("sum_f_final : %d\n",sum_f);
    //printf("sum_in_final : %d\n",fin_sum);

    if(sum_final%10==0)
    {
        flag_val=1; // checking if the number is valid
    }
    if(ff_num == 0)
    {
        ff_num = (d1_sum*10) + d2_sum;     //flip
    }

    do
    {
       if(sp_len==0)
       {
        printf("INVALID\n");
        check_flag=1;
        break;
       }
       if((flag==1&&flag_val==1&&ff_num==34)||ff_num==37)
       {
        printf("AMEX\n");
        check_flag=1;
        break;
       }
       else if(flag==1&&flag_val==1&&ff_num>50&&ff_num<56)
       {
        printf("MASTERCARD\n");
        check_flag=1;
        break;
       }
       else if(flag==1&&flag_val==1&&d1_sum==4)
       {
        printf("VISA\n");
        check_flag=1;
        break;
       }
       else
       {
        printf("INVALID\n");
        check_flag=1;
        break;
       }
    }while(check_flag!=1);

    return 0;

}

I felt like I was fixing a leak while writing the code. I would try to correct one thing and another thing would go wrong and this is the final result.

Thank you for your help. If the question was not asked properly please forgive me; it's my first time here.

Code will look much better once OP can use own functions.

  how can I improve my solution?

Meaningful names

Why is the credit card number called j.  How about cc_num?

Many others names need like-wise improvements.

Use bool

For flags, consider bool.

#include <stdbool.h>

bool flag;

Auto format

Use a development environment that supports auto formatting.  Do not format manually - just not efficient.

#include<stdlib.h>
#include<cs50.h>

int main() {
  long long x, j = 0;
  int len, i, len_flag, flag = 0, flag_val = 0, sp_len = 0, check_flag = 0; //basic integers related  to loop etc
  int res, sum, fin_sum = 0, h = 0, d2_sum = 0, d1_sum = 0, ff_num = 0; // variables  related to extracting numbers
  int sum_len = 0, in_sum = 0, in_fsum = 0, len_sum = 0, m = 0; // extraing numbers from more than single  sum result
  int sum_f = 0, sum_final = 0;

  do {
    x = get_long("enter a valid card number \n");
    j = x;
    len_flag = 0;
    len = 0;
    while (len_flag != 1) {
      x = x / 10;
      if (x == 0) {
        len_flag = 1;   //finding the lenght of the number
      }
      len++;
    }
    if (len == 15 || len == 16 || len == 13) {
      flag = 1;
      sp_len = 1;
    } else {
      flag = 1;
    }

  } while (flag != 1);
  ...

Define variables where needed

Rather than a sea of objects up front, defined them where needed.

Example: Refactor x.  Other variables can like-wise get defined closer to their usage.

  // long long x, j = 0;
  long long j = 0;
  ...

  do {
    // x = get_long("enter a valid card number \n");
    long long x = get_long("enter a valid card number \n");
    j = x;
    ...

    if (sum_len > 1) {
      // x = 0;
      int x = 0;

Spelling

//finding the lenght of the number
  finding the length of the number

Clean-up

For review purposes, no need for dead debug code.
Eliminate //printf("multi_res : %d \n",sum), etc.

Eliminate unused variables like m.

Comment code

Each chunk of code deserves to be a function, yet I understand that is prohibited for now.

I'd comment the chunks of code to help aide understanding.

  // Read input CC number
  // Set j as the CC#, len_flag as the CC# length, flag to 1, ...
  do {
    long long x = get_long("enter a valid card number \n");

Simplify

flag only used as 1.

  do {
    ...
      flag = 1;
  } while (flag != 1);

Replaceable with

  {
    ...
  }

Same for later do {  ... } while (check_flag != 1);.

Documentation

Within code I'd add an algorithm link:

// https://en.wikipedia.org/wiki/Luhn_algorithm

Why space before '\n' ?

// "enter a valid card number \n"
"enter a valid card number\n"

Chunks of code seem verbose

len_flag = 0;
len = 0;
while (len_flag != 1) {
  x = x / 10;
  if (x == 0) {
    len_flag = 1;   //finding the lenght of the number
  }
  len++;
}

versus

// decimal digit length of x
len = 0;
do {
  len++;
} while (x /= 10);


Finding all possible distinct palindromes of a string There was some online test where I was asked about finding all possible distinct palindromes of a string.

Here I had to give the count of all possible distinct palindromes of a given string (continuous substring). Here, a single character word is considered a palindrome.

Below is what I did. Can you please tell me if it is good OR there is a scope of improvement?

public class Solution {
/*
* Complete the function below.
*/

static int palindrome(String str) {
    String[] strArray = str.split("");
    List<String> list = Arrays.asList(strArray);
    list = list.subList(1, list.size());
    //Set does'nt allow duplicates.
    //Sublist is required because split method gives an extra space.
    Set<String> palindromeSet = new HashSet<>(list);
    String palindromeStr = null;
    for(int i = 0;i<list.size();i++){
        palindromeStr = list.get(i);
        for(int j = i+1;j<list.size();j++){
            palindromeStr = palindromeStr+list.get(j);
            if(isPalindrome(palindromeStr)){
                palindromeSet.add(palindromeStr);
            }
        }
    }
    return palindromeSet.size();
}

static boolean isPalindrome(String str){
    char[] chars = str.toCharArray();
    for(int i =0;i<(chars.length/2);i++){
        if(chars[i] != chars[chars.length-1-i]){
            return false;
        }
    }
    return true;
}

public static void main(String[] args) throws IOException{
    System.out.println(palindrome("arewenotdrawnonwardtonewera"));
}
}

/*
* Complete the function below.
*/

Do you still need it?

String[] strArray = str.split("");
List<String> list = Arrays.asList(strArray);
list = list.subList(1, list.size());

That's ugly. You could work with the original String or use str.toCharArray() if you really needed an array. But you don't.

//Set does'nt allow duplicates.

True, but rather well-known. And a typo.

//Sublist is required because split method gives an extra space.

True, but misplaced by a few lines. Full line comments belong before the block they describe.

String palindromeStr = null;
for(int i = 0;i<list.size();i++){
    palindromeStr = list.get(i);

This should be

for(int i = 0; i < list.size(); i++){
    String palindromeStr = list.get(i);

Note the spacing.

    for(int j = i+1;j<list.size();j++){
        palindromeStr = palindromeStr+list.get(j);

This is a pretty slow way of creating what

   String palindromeStr = str.substring(i, j);

could give you. By creating your string incrementally you gain nothing: Because of strings being immutable, their whole content gets copied on every step.

This way the complexity is O(n**3) and could be reduced(*) to O(n**2) by simply defining a method working on a substring like

 static boolean isPalindrome(String str, int start, int end) ...

You should improve both spacing (just press Ctrl-Shift-F in Eclipse) and naming, maybe as follows:

str -> input
strArray -> nothing, just inline it
palindromeSet -> palindromes as it's clearly a set
palindromeStr -> substring or candidate as it's not always a palindrome

Your naming is not really bad, but you concentrate on the type too much and I can imagine to get lost in a bunch of names like intListSetArray  and strDoubleMap without any clue what's the variable good for.

I'd bet there's a faster algorithm, but I haven't figured it out yet.

(*) I'm assuming that isPalindrome is O(1) on the average, which is true for normal strings, but not for e.g. "aaaa....a".


Protecting Vue page based on the API response If the user/visitor has access to the API route, then the status code will 200, else 401.
I know that I can user Vue navigation guards, but I will need to use Vuex.
This way is much simpler and faster, what do you think, is it a right way to protect Vue page from unauthenticated users?
    <template>
    <v-container>

loading

Protected Profile Page

    </v-container>
</template>

In theory what you do here works, but there are a few caveats:

You declared your beforeCreate lifecycle hook as async, but you do not await any statement. It serves no particular purpose.
Be very careful with declaring lifecycle hooks as asynchronous, because when you await something that rejects the promise, you cannot catch the resulting error. You will end up with a component that cannot render, which makes for a bad user experience.
Vue router has global navigation guards, but you can actually also just declare a route guard on the component that is being mounted. They are called in-component guards and one that would be useful to you is beforeRouteEnter. You can redirect someone to the login page before this component even renders.

beforeRouteEnter (to, from, next) {
  axios.get('api/user', {})
    .then(_ => {
      next();
    }).catch(_ => {
      next('/login');
    });
}

While you could rely on the endpoint returning an error of some sort, there are more reasons why your endpoint might return an error. Some of those reasons might include rate limiting of the api, a bad connection or an overloaded server. Having someone redirect to the login page everytime that happens will get old fast. If you want to protect multiple components this way, you are spamming your server with unnecessary api calls. Also keep in mind that frontend security is essentially no security at all. You only "protect" stuff in the frontend to help the user experience, but your api should never return data or save data from a user that has no permissions/is not logged in. You are likely better off catching 403 errors and only redirecting them, while showing a generic error message on anything else.

You have already mentioned it, but you could use Vuex for state management. Arguably, something like "isAuthenticated" is not something every singular component should manage. Setting up a vuex store is usually less than 5 minutes work and I would still recommend managing something like that there.


Extending java.util.Random.nextInt(int) into nextLong(long) In java.util.Random the Oracle implementation of nextInt(int) is as follows:

public int nextInt(int n) {
    if (n <= 0)
        throw new IllegalArgumentException("n must be positive");

    if ((n & -n) == n)  // i.e., n is a power of 2
        return (int)((n * (long)next(31)) >> 31);

    int bits, val;
    do {
        bits = next(31);
        val = bits % n;
    } while (bits - val + (n-1) < 0);
    return val;
}

I have a need to do the same thing for longs, but this is not included as part of the class signature. So I extended the class to add this behavior. Here's my solution, and even though I'm pretty sure I have it right, bit-twiddling can subtly fluster even the best of devs!

import java.util.Random;

public class LongRandom extends Random {

    public long nextLong(long n) {
        if (n <= 0)
            throw new IllegalArgumentException("n must be positive");

        if ((n & -n) == n)  // i.e., n is a power of 2
            return nextLong() & (n - 1); // only take the bottom bits

        long bits, val;
        do {
            bits = nextLong() & 0x7FFFFFFFL; // make nextLong non-negative
            val = bits % n;
        } while (bits - val + (n-1) < 0);
        return val;

    }
}

Have I introduced a subtle bug? Are there improvements to make? What might I need to watch out for?

Try it with n>2^32 and your code will fail. 0x7FFFFFFFL reduces your code to int, breaking your extension to long. You need to use the maximal value of long not int in your mask. i.e. 7FFFFFFFFFFFFFFFL.


Correct usage of MVVM and object for method I'm writing my first C# Windows Store app and am learning C# from scratch. I'm trying to implement MVVM as I understand it and object orientated patterns, obviously though I'm expecting to be doing it wrong so looking for pointers as to what I should improve.

Below is a method that gets called when a user either clicks a 'Search' XAML control, or hits return in the search textbox control.

The comments should explain the code. I feel like I probably have too much code in for a event method so please advise how I would restructure it.

  private void SubmitSearch(object sender, RoutedEventArgs e)
    {
        //New Validation object
        ViewModels.Validation validation = new ViewModels.Validation();

        // Capture user entered term and check is valid
        // submit appropriate message.
        var searchValue = searchTerm.Text.ToString();
        dynamic validateInput = validation.inputNullCheck(searchValue);

        int? errorCode = validateInput.ErrorCode;

        if (errorCode == 0)
        {
            SubmitAction(searchValue);
        }
        else
        {
            DisplayTerms(validateInput);
        }

    }

My SubmitAction method:

public async Task<object> SubmitAction(string searchValue)
    {

        // Query webservice/database through Model and return response
        dynamic response = await new ViewModels.Search().QueryRequest(searchValue);
        DisplayTerms(response);

        //This is here only because I needed the method to be async, which in turn requires a
        // value to be returned. Ideally it would be async and end after DisplayTerms(response);
        return response;
    }

DisplayTerms method:

private void DisplayTerms(object value)
    {

        ListView termsList = termsListContainer;
        dynamic searchResponse = value;

        int count = searchResponse.Count;

        // This will eventually be a loop through the returned object
        // it is hardcoded at the moment because I am debugging something.
        termsList.Items.Add(searchResponse[0].TermName);
    }

Validation class:

class Validation
{

    // Temporarily 0 = success, 1 = error
    public int? ErrorCode { get; set; }
    public string ErrorName { get; set; }
    public string ErrorMessage { get; set; }

    //Constructor
    public Validation ()
    {

    }

    // Check if user input was empty
    public object inputNullCheck(string input)
    {
        if (input != "")
        {
            this.ErrorCode = 0;
            this.ErrorName = "Success";
            this.ErrorMessage = "Valid input received";
            return this;
        }
        else
        {
            this.ErrorCode = 1;
            this.ErrorName = "Input Empty";
            this.ErrorMessage = "You have not input anything.";
            return this;
        }
    }
}

Minor note:

Here in your validation class there's this comment.

// Temporarily 0 = success, 1 = error

Which is fine. You don't want to use a boolean because you'll be adding more status codes in the future. Cool! But you shouldn't be hard coding the error code numbers. This is what enums are for.

enum ValidationStatus {Success,GenericError}

// Check if user input was empty
public object inputNullCheck(string input)
{
    if (input != "")
    {
        this.ErrorCode = ValidationStatus.Success;

At this point, it would be better to remove this code

       this.ErrorName = "Success";
       this.ErrorMessage = "Valid input received";

And create a class that manages what Message to return based off of the value of the ValidationStatus passed into its constructor.

Reducing the above code to

    if (input != "")
    {
        return new ValidationResult(ValidationStatus.Success);


Finding the coordinates on an edge of a square I wrote a calculation that finds coordinates on an edge of a square, but it came out as a really long nested tangle of conditions. After some reduction and finding common pieces I arrived at this example:

  // targetX, targetY, sourceX, sourceY are provided as parameters
  // and are markers for the centers of the two locations connecting
  let tx;
  let ty;
  const dx = targetX - sourceX;
  const dy = targetY - sourceY;
  const adjY = (radius * Math.tan(Math.atan2(dy, dx)));
  const adjX = (radius / Math.tan(Math.atan2(dy, dx)));
  if (Math.abs(adjY) <= radius) { // A
    if (targetX <= sourceX) { // B
      tx2 = targetX + (Math.abs(adjX) <= radius ? adjX : radius);
      ty2 = targetY + (Math.abs(adjY) <= radius ? adjY : radius);
    } else {
      tx2 = targetX - (Math.abs(adjX) <= radius ? adjX : radius);
      ty2 = targetY - (Math.abs(adjY) <= radius ? adjY : radius);
    }
  } else if (targetY <= sourceY) { // C
    tx2 = targetX + (Math.abs(adjX) <= radius ? adjX : radius);
    ty2 = targetY + (Math.abs(adjY) <= radius ? adjY : radius);
  } else {
    tx2 = targetX - (Math.abs(adjX) <= radius ? adjX : radius);
    ty2 = targetY - (Math.abs(adjY) <= radius ? adjY : radius);
  }

Moving several of the conditions into the lines themselves was possible using ternary operators. But now I'm stuck, so how do I reduce the conditions even further?

I thought of using boolean logic gate reduction techniques, like Karnaugh maps, but fail to find a way to map that process to my problem.

Other solutions involving different math are welcome as well.

The problem is finding the new target X and Y so that they meet the edge of the target rectangle instead of its center.

I think the solution is a little simplier

const dx = targetX - sourceX;
const dy = targetY - sourceY;
const scale = Math.hypot(dx, dy) / Math.hypot(radius,radius * (Math.abs(dy / dx) < 1 ? dy / dx : dx / dy));
const tx = targetX - dx / scale;
const ty = targetY - dy / scale;

If it is just the boundary of the box you are after. The only comparison is to find out which part of the box the line ends on, top/bottom or left/right the rest works by preserving dx and dy's sign. It also works if dx or dy is zero.

The only thing I am not sure of is if the offset is from the target, if so I would change to

const dx = sourceX - targetX;
const dy = sourceY - targetY;
const scale = Math.hypot(dx, dy) / Math.hypot(radius,radius * (Math.abs(dy / dx) < 1 ? dy / dx : dx / dy));
const tx = targetX + dx / scale;
const ty = targetY + dy / scale;


Find a substring with minimum distance Problem statement for finding given substring:

  Given a random string S and another string T with unique elements,
  find the minimum consecutive sub-string of S such that it contains all
  the elements in T.

  Example:

S='adobecodebanc'
T='abc'
answer='banc'

How could I have improved the code from construction of code? Should I have created one more compare function? Did I use too many variables? Could I have optimized the code further and much more?

#include <iostream>
#include <stdio.h>

int min(int a, int b, int c);
int max(int a, int b, int c);

int main(void)
{
    char string1[] = "adobecodebanc";
    char *pstring = string1;
    int first_char = -1;
    int second_char = -1;
    int third_char = -1;
    int final_distance = 1000;
    int final_min = 0;
    int final_max = 0;
    int counter = 0;

    /* iteratre over the string and compare each characeter*/
    while (*pstring != '\0')
    {
        /* if any character matches substring condition
           assign index value of the string to that variable  */
        if (*pstring == 'a' || *pstring == 'b' || *pstring == 'c')
        {
            if (*pstring == 'a')
            {
                first_char = counter;
            }
            else if (*pstring == 'b')
            {
                second_char = counter;
            }
            else
            {
                third_char = counter;
            }

            if (first_char != -1 && second_char != -1 && third_char != -1)
            {
                /* get the min and max index value difference*/
                int min_1 = min(first_char, second_char, third_char);
                int max_1 = max(first_char, second_char, third_char);
                int distance = max_1 - min_1;

                /*track the minimum substring so far*/
                if (distance < final_distance)
                {
                    final_distance = distance;
                    final_min = min_1;
                    final_max = max_1;
                }
            }
        }
        pstring++;
        counter++;
    }

    int temp = final_min;

    while (final_max >= temp)
    {
        std::cout << string1[temp];
        temp++;
    }
    std::cout << std::endl;
    return 0;
}

/* Find the min index value*/
int min(int first_char, int second_char, int third_char)
{
    if (first_char < second_char)
    {
        if (first_char < third_char)
        {
            return first_char;
        }
        else
        {
            return third_char;
        }
    }
    else
    {
        if (second_char < third_char)
        {
            return second_char;
        }
        else
        {
            return third_char;
        }
    }
}

/* Find the max index value*/
int max(int first_char, int second_char, int third_char)
{
    if (first_char > second_char)
    {
        if (first_char > third_char)
        {
            return first_char;
        }
        else
        {
            return third_char;
        }
    }
    else
    {
        if (second_char > third_char)
        {
            return second_char;
        }
        else
        {
            return third_char;
        }
    }
}

There are already the functions std::min and std::max that you should use. It either takes 2 elements or an std::initializer_list, so you could use

int min_1 = std::min({first_char, second_char, third_char});

There is also std::minmax to get both the minimum and the maximum, possibly more efficiently than calling std::min and then std::max. Not that it matters for your application, but it is a good habit to get into.

There is also std::none_of to check if none of first_char, second_char and third_char are -1, but the use is awkward because it needs a predicate.
Use range based for when possible. In this case you iterate over all the chars in the string, so it is easy:

//while (*pstring != '\0')
//for (const char c : string1)
for (const char c : "adobecodebanc")

If you initialize first_char, second_char and third_char with std::numeric_limits<int>::min() you could leave out the check if all of them are not -1. You need to be careful for when printing though.
I assume this is supposed to be a general function for any string. If that string is "erf" the function prints "e". That doesn't seem right.
main(void) is considered bad style in C++. In C a function void f(); takes any number of arguments, so f(234, 2) compiles fine. To make that fail to compile you have to write void f(void). In C++ f(234, 2) doesn't compile and void f(void) is redundant, making you look like a C programmer trying out C++.
final_distance is initiated to "some big number". It makes sense to use std::numeric_limits<int>::max() instead of 1000.
Use const when it makes sense, for example for min_1, max_1 and distance.
Putting it all together it looks like this:

#include <iostream>
#include <stdio.h>
#include <algorithm>

int main()
{
    char string1[] = "adobecodebanc";
    int first_char = -1;
    int second_char = -1;
    int third_char = -1;
    int final_distance = std::numeric_limits<int>::max();
    int final_min = 0;
    int final_max = 0;
    int counter = -1;

    /* iteratre over the string and compare each characeter*/
    for (const char c : string1)
    {
        counter++;
        /* if any character matches substring condition
           assign index value of the string to that variable  */
        if (c == 'a')
        {
            first_char = counter;
        }
        else if (c == 'b')
        {
            second_char = counter;
        }
        else if (c == 'c')
        {
            third_char = counter;
        }
        else{
            continue;
        }

        if (first_char != -1 && second_char != -1 && third_char != -1)
        {
            /* get the min and max index value difference*/
            const auto minmax = std::minmax({first_char, second_char, third_char});
            const int distance = minmax.second - minmax.first;

            /*track the minimum substring so far*/
            if (distance < final_distance)
            {
                final_distance = distance;
                final_min = minmax.first;
                final_max = minmax.second;
            }
        }
    }

    int temp = final_min;

    while (final_max >= temp)
    {
        std::cout << string1[temp];
        temp++;
    }
    std::cout << std::endl; //should not be needed since the program exits
    return 0; //gets implicitly added if you leave it out
}

I feel like I'm missing an obvious improvement, so I'll take a look again later.


C# AES Encryption/Decryption or byte[] array with a custom Key/IV I wanted to create an AES encryption service which will use a custom key/iv.
I want 2 methods: dycrypt/encrypt byte[]
The code works as expected, any remarks/issues?
public class AesGenericEncryptionService
{
    private readonly byte[] m_Key;
    private readonly byte[] m_IV;

    public AesGenericEncryptionService(byte[] key, byte[] iv)
    {
        if (key == null || key.Length <= 0)
            throw new ArgumentNullException(nameof(key));
        if (iv == null || iv.Length <= 0)
            throw new ArgumentNullException(nameof(iv));

        m_Key = key;
        m_IV = iv;
    }

    public byte[] Encrypt(byte[] data)
    {
        byte[] encrypted;

        using (RijndaelManaged rijAlg = new RijndaelManaged())
        {
            rijAlg.Mode = CipherMode.CBC;
            rijAlg.KeySize = m_Key.Length * 8;
            rijAlg.Key = m_Key;
            rijAlg.BlockSize = m_IV.Length * 8;
            rijAlg.IV = m_IV;

            using (ICryptoTransform encryptor = rijAlg.CreateEncryptor(rijAlg.Key, rijAlg.IV))
            {
                using (MemoryStream msEncrypt = new MemoryStream())
                {
                    using (CryptoStream csEncrypt = new CryptoStream(msEncrypt, encryptor, CryptoStreamMode.Write))
                    {
                        csEncrypt.Write(data, 0, data.Length);
                        csEncrypt.FlushFinalBlock();

                        encrypted = msEncrypt.ToArray();
                    }
                }
            }
        }

        return encrypted;
    }
    public byte[] Decrypt(byte[] cipher)
    {
        // Check arguments.
        if (cipher == null || cipher.Length <= 0)
            throw new ArgumentNullException(nameof(cipher));

        byte[] dycrypted = null;
        using (RijndaelManaged rijAlg = new RijndaelManaged())
        {
            rijAlg.Mode = CipherMode.CBC;
            rijAlg.KeySize = m_Key.Length * 8;
            rijAlg.Key = m_Key;
            rijAlg.BlockSize = m_IV.Length * 8;
            rijAlg.IV = m_IV;

            using (ICryptoTransform decryptor = rijAlg.CreateDecryptor(rijAlg.Key, rijAlg.IV))
            {
                using (MemoryStream ms = new MemoryStream(cipher))
                {
                    using (var cryptoStream = new CryptoStream(ms, decryptor, CryptoStreamMode.Read))
                    {
                        dycrypted = new byte[cipher.Length];
                        var bytesRead = cryptoStream.Read(dycrypted, 0, cipher.Length);

                        dycrypted = dycrypted.Take(bytesRead).ToArray();
                    }
                }
            }
        }

        return dycrypted;
    }
}

ctor
Neither key.Length nor iv.Length can be smaller than 0 you only need to check if it is equal to 0. But for me as a user of that class it would look strange to receive an ArgumentNullException if I pass e.g a key with Length == 0. I would expect an ArgumentOutOfRangeException.
Omitting braces althougth they might be optional can lead to hidden and therfor hard to find bugs.
Encrypt()

The default Mode of RijndaelManaged is already CipherMode.CBC so there is no need to set it again.

By returning out of the most inner using you can remove byte[] encrypted.

By stacking the using's you save some levels of indentation.

You can use the parameterless CreateEncryptor() method because you already set the Key and IV.

As the method is public you should validate its parameter.

Decrypt()

The default Mode of RijndaelManaged is already CipherMode.CBC so there is no need to set it again.

By returning out of the most inner using you can remove byte[] decrypted.

By stacking the using's you save some levels of indentation.

You can use the parameterless CreateDecryptor() method because you already set the Key and IV.

You shouldn't mix styles. In the using's you sometimes use the concrete type and sometimes you use var.

General
Naming things in a good way is hard. Mostly it is sufficient and just more readable to just use a classname in camelCase casing. If you stumble in a few months over the variable rijAlg you will need to ask yourself what it stands for. By naming it rijndaelManaged you will see at first glance what it is about.
Another example of "bad"  naming is in Encrypt() you name the MemoryStream msEncrypt but in Decrypt() you name it ms the same is true for csEncrypt vs cryptoStream.
Implementing the mentioned points leads to
public class AesGenericEncryptionService
{
    private readonly byte[] m_Key;
    private readonly byte[] m_IV;

    public AesGenericEncryptionService(byte[] key, byte[] iv)
    {
        if (key == null)
        {
            throw new ArgumentNullException(nameof(key));
        }
        else if (key.Length == 0)
        {
            throw new ArgumentOutOfRangeException(nameof(key));
        }

        if (iv == null || iv.Length <= 0)
        {
            throw new ArgumentNullException(nameof(iv));
        }
        else if (iv.Length == 0)
        {
            throw new ArgumentOutOfRangeException(nameof(iv));
        }

        m_Key = key;
        m_IV = iv;
    }

    public byte[] Encrypt(byte[] data)
    {
        if (data == null)
        {
            throw new ArgumentNullException(nameof(data));
        }

        using (var rijndaelManaged = new RijndaelManaged())
        {
            rijndaelManaged.KeySize = m_Key.Length * 8;
            rijndaelManaged.Key = m_Key;
            rijndaelManaged.BlockSize = m_IV.Length * 8;
            rijndaelManaged.IV = m_IV;

            using (var encryptor = rijndaelManaged.CreateEncryptor())
            using (var ms = new MemoryStream())
            using (var cryptoStream = new CryptoStream(ms, encryptor, CryptoStreamMode.Write))
            {
                cryptoStream.Write(data, 0, data.Length);
                cryptoStream.FlushFinalBlock();

                return ms.ToArray();
            }
        }
    }
    public byte[] Decrypt(byte[] cipher)
    {
        // Check arguments.
        if (cipher == null)
        {
            throw new ArgumentNullException(nameof(cipher));
        }
        else if (cipher.Length == 0)
        {
            throw new ArgumentOutOfRangeException(nameof(cipher));
        }

        using (var rijndaelManaged = new RijndaelManaged())
        {
            rijndaelManaged.KeySize = m_Key.Length * 8;
            rijndaelManaged.Key = m_Key;
            rijndaelManaged.BlockSize = m_IV.Length * 8;
            rijndaelManaged.IV = m_IV;

            using (var decryptor = rijndaelManaged.CreateDecryptor())
            using (var ms = new MemoryStream(cipher))
            using (var cryptoStream = new CryptoStream(ms, decryptor, CryptoStreamMode.Read))
            {

                var dycrypted = new byte[cipher.Length];
                var bytesRead = cryptoStream.Read(dycrypted, 0, cipher.Length);

                return dycrypted.Take(bytesRead).ToArray();
            }
        }
    }
}


Boolean flags encoded as integer implemented with EnumSet I'm a beginner in Java so I would appreciate a review of following simple class - in fact it's my first real usage of enums.

The background: I'm parsing MySQL internal client-server protocol. One of the fields in server greeting and client authorization packet is "Client capabilities" which is 32 bit integer encoding 18 boolean flags ( if anyone is interested: http://forge.mysql.com/wiki/MySQL_Internals_ClientServer_Protocol#Handshake_Initialization_Packet)

I've read that the most 'java way' to implement such thing will be using EnumSet class.

I'll welcome any suggestions and other solutions - my needs are:

I must be able to parse flags from 4 bytes big endian buffer
To encode flags back to 4 bytes big endian buffer
To check if some arbitrary flag is set

here's the code:

public class ClientCapabilities
{
    public enum Flag {
        CLIENT_LONG_PASSWORD (1<<0),
        CLIENT_FOUND_ROWS (1<<1),
        CLIENT_LONG_FLAG (1<<2),
        CLIENT_CONNECT_WITH_DB (1<<3),
        CLIENT_NO_SCHEMA (1<<4),
        CLIENT_COMPRESS (1<<5),
        CLIENT_ODBC (1<<6),
        CLIENT_LOCAL_FILES (1<<7),
        CLIENT_IGNORE_SPACE (1<<8),
        CLIENT_PROTOCOL_41 (1<<9),
        CLIENT_INTERACTIVE (1<<10),
        CLIENT_SSL (1<<11),
        CLIENT_IGNORE_SIGPIPE (1<<12),
        CLIENT_TRANSACTIONS (1<<13),
        CLIENT_RESERVED (1<<14),
        CLIENT_SECURE_CONNECTION (1<<15),
        CLIENT_MULTI_STATEMENTS (1<<16),
        CLIENT_MULTI_RESULTS (1<<17);

        public final int weight;

        Flag(Integer weight) {
            this.weight = weight;
        }
    }

    private final int originalInt;
    private final EnumSet<Flag> flags = EnumSet.noneOf(Flag.class);

    public ClientCapabilities(int capabilities)
    {
        originalInt = capabilities;
        for (Flag f : Flag.values()) {
            if ((capabilities & f.weight) > 0) {
                flags.add(f);
            }
        }
    }

    public int getAsInt()
    {
        return originalInt;
    }

    public EnumSet<Flag> getFlags()
    {
        return flags;
    }

    @Override
    public String toString() {
        return flags.toString();
    }

    //only tests ahead - TestNG, not JUnit!
    @Test
    public static void testDecodingClientCapabilities()
    {
        byte[] exampleBuffer = new byte[] { (byte) 0x85, (byte) 0xa6, (byte) 0x03, (byte) 0x00 };
        EnumSet<Flag> expectedEnumSet = EnumSet.of(
                Flag.CLIENT_LONG_PASSWORD,
                Flag.CLIENT_LONG_FLAG,
                Flag.CLIENT_LOCAL_FILES,
                Flag.CLIENT_PROTOCOL_41,
                Flag.CLIENT_INTERACTIVE,
                Flag.CLIENT_TRANSACTIONS,
                Flag.CLIENT_SECURE_CONNECTION,
                Flag.CLIENT_MULTI_STATEMENTS,
                Flag.CLIENT_MULTI_RESULTS
        );

        ClientCapabilities capabilities = new ClientCapabilities(
                PacketByteBuffer.readIntFromFourBytes(exampleBuffer,0));

        Assert.assertEquals(capabilities.getFlags(), expectedEnumSet);
    }

    @Test
    public static void testEncodingClientCapabilities()
    {
        byte[] exampleBuffer = new byte[] { (byte) 0x85, (byte) 0xa6, (byte) 0x03, (byte) 0x00 };
        byte[] targetBuffer = new byte[] { 0, 0, 0, 0, 0};

        ClientCapabilities capabilities = new ClientCapabilities(
                PacketByteBuffer.readIntFromFourBytes(exampleBuffer,0));

        PacketByteBuffer.writeIntToFourBytes(targetBuffer, 1, capabilities.originalInt);

        Assert.assertEquals(targetBuffer[1], (byte) 0x85);
        Assert.assertEquals(targetBuffer[2], (byte) 0xa6);
        Assert.assertEquals(targetBuffer[3], (byte) 0x03);
        Assert.assertEquals(targetBuffer[4], (byte) 0x00);
    }
}

It looks fine. Some notes about the tests:

Test methods should be in another class (ClientCapabilitiesTest) and in a separate source tree. For example, Maven projects usually store sources under src/main/java and test sources under src/test/java. (Introduction to the Standard Directory Layout).

The first parameter of assertEquals is the expected value and the second is the actual one, so you should change

assertEquals(targetBuffer[1], (byte) 0x85);

to

assertEquals((byte) 0x85, targetBuffer[1]);

JUnit error messages assumes this order.

Create more tests which helps defect localization. A parameterized test class would be fine:

import static org.junit.Assert.assertEquals;

import java.util.Arrays;
import java.util.Collection;
import java.util.EnumSet;

import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;

@RunWith(Parameterized.class)
public class ClientCapabilitiesTest {

    private final byte[] input;
    private final EnumSet<Flag> expected;

    @Parameters
    public static Collection<Object[]> data() {
        // TODO: add more cases
        final Object[][] data = new Object[][] {
                { new byte[] { (byte) 0x01, (byte) 0x00, (byte) 0x00, (byte) 0x00 },
                        EnumSet.of(Flag.CLIENT_LONG_PASSWORD) },
                { new byte[] { (byte) 0x02, (byte) 0x00, (byte) 0x00, (byte) 0x00 },
                        EnumSet.of(Flag.CLIENT_FOUND_ROWS) },
                { new byte[] { (byte) 0x03, (byte) 0x00, (byte) 0x00, (byte) 0x00 },
                        EnumSet.of(Flag.CLIENT_LONG_PASSWORD, Flag.CLIENT_FOUND_ROWS) },
                { new byte[] { (byte) 0x85, (byte) 0xa6, (byte) 0x03, (byte) 0x00 },
                        EnumSet.of(Flag.CLIENT_LONG_PASSWORD,
                                Flag.CLIENT_LONG_FLAG,
                                Flag.CLIENT_LOCAL_FILES,
                                Flag.CLIENT_PROTOCOL_41,
                                Flag.CLIENT_INTERACTIVE,
                                Flag.CLIENT_TRANSACTIONS,
                                Flag.CLIENT_SECURE_CONNECTION,
                                Flag.CLIENT_MULTI_STATEMENTS,
                                Flag.CLIENT_MULTI_RESULTS) } };
        return Arrays.asList(data);
    }

    public ClientCapabilitiesTest(final byte[] input, final EnumSet<Flag> expected) {
        this.input = input;
        this.expected = expected;
    }

    @Test
    public void testDecodingClientCapabilities() {
        final ClientCapabilities capabilities = new ClientCapabilities(
            PacketByteBuffer.readIntFromFourBytes(input, 0));

        Assert.assertEquals(expected, capabilities.getFlags());
    }
}

I'd add one case for every flag and some cases with multiple enabled flags.


Making a script file to perform maintenance I wish to make a file to click on to do the 'usual' tasks I would do to clean/perform maintenance on my mother's computer when I'm not present.  I made a script file and took a few command lines from Google.  Am I on the right path?

<job>
<script language="VBScript">
Option Explicit
On Error Resume Next
Dim WshShell
Dim retVal
set WshShell=CreateObject("WScript.Shell")
WshShell.run "CCleaner.exe /AUTO"

WshShell.run "WiseRegCleaner.exe /AUTO"

WshShell.run "Cleanmgr /sagerun:1"

WshShell.run "Defrag volume c:"

wshshell.run "WiseDiskCleaner.exe /AUTO"

wshshell.run "mbam.exe /fullscanterminate"

WScript.Quit
</script>
</job>

This is awesome:

Option Explicit

By doing that you've forced yourself to declare every variable you use; making a typo in a variable's name will stop the code from being executable, which is a very good thing. Well done!

This is less awesome:

Dim retVal

Declaring variables you're not using is, well, useless. Use them or kill 'em.

Then there's this:

On Error Resume Next

It basically says "look, I don't care what happens, just run every single executable statement you see and whatever happens happens" - so if any command raises an error, you completely ignore it and move on: if CCleaner.exe doesn't exist or the /AUTO option isn't valid for that command, your script will never even know.

You could log it in a text file that you can consult and clean up when you check out mom's computer, or simply output a message that contains the description of the error, for example:

On Error GoTo ErrHandler

'...script...

WScript.Quit

ErrHandler:
WScript.Echo "Unexpected error: " & Err.Description
Resume Next

This is minimal error handling - it does pretty much exactly the same thing (it just keeps going), except if there's an error it will output an error message before resuming with the next instruction.

That "jumping around" isn't ideal though - when there's an error with a command, execution jumps to ErrHandler, prints the error, and the jumps back to the instruction immediately following the line that raised the error. There's a better way.

Make yourself a Function. The advantage of a function is that it makes the shell object short-lived (it's scoped to the function), and it can handle an error that not only could happen when passing the command string to the shell object's Run method, but also if Run returns a non-zero error code - if all goes well the function itself returns a success message, and if anything goes wrong it returns an error message. That level of abstraction also makes the actual script much less cluttered.

With this function, your entire script could look like this:

<job>
<script language="VBScript">
    Option Explicit

    Function Execute(command)
        On Error GoTo CleanFail

        Dim shell
        Set shell = CreateObject("WScript.Shell")

        Dim code
        code = shell.Run(command)

        Dim result
        If code <> 0 Then
            result = "Non-zero exit code: " & CStr(code)
        Else
            result = "Command '" & command & "' completed successfully."
        End If

    CleanExit:
        Execute = result
        Exit Function
    CleanFail:
        result = "Unexpected error #" & CStr(Err.Number) & ": " & Err.Description
        Resume CleanExit
    End Function

    WScript.Echo Execute("CCleaner.exe /AUTO")
    WScript.Echo Execute("WiseRegCleaner.exe /AUTO")
    WScript.Echo Execute("Cleanmgr /sagerun:1")
    WScript.Echo Execute("Defrag volume c:")
    WScript.Echo Execute("WiseDiskCleaner.exe /AUTO")
    WScript.Echo Execute("mbam.exe /fullscanterminate")

    WScript.Echo "Completed."
</script>
</job>

Notice there's no need to declare any script-level ("global") variables, handle errors at script-level, or to explicitly Quit anything - it simply runs to completion and lets the script host deal with termination.

If all goes well then your output should look like this:

Command 'CCleaner.exe /AUTO' completed successfully.
Command 'WiseRegCleaner.exe /AUTO' completed successfully.
Command 'Cleanmgr /sagerun:1' completed successfully.
Command 'Defrag volume c:' completed successfully.
Command 'WiseDiskCleaner.exe /AUTO' completed successfully.
Command 'mbam.exe /fullscanterminate' completed successfully.
Completed.