Async task with timeout I am calling a service outside of my control. My application must include a time out, so that if the call to the service takes too long, an appropriate time-out message is returned.

// Client connected to remote service
RemoteClient Client = new RemoteClient();

private async Task<MyResult> CheckItAsync(CheckRequest rqst, int timeout)
{
    // MyResult object gets some values populated on construction
    MyResult Result = new MyResult()

    // RemoteResponse auto generated from 'Add Service Reference'
    RemoteResponse response;

    try
    {
        // Perform the check and return the appropriate response.
        var task = Client.PerformCheckAsync(rqst);
        var res = await Task.WhenAny(task, Task.Delay(timeout));
        if (res == task)
        {
            // Task completed within time.
            response = task.GetAwaiter().GetResult();
            // Populate my result object with the necessary values
            Result.populate(response);
        }
        else
        {
            // Task timed out, populate result with timeout message
            Result.timedOut(rqst.RequestNumber);
        }

    }
    catch (Exception e)
    {
        // Something else happened, log exception and return failed result
        Elmah.ErrorSignal.FromCurrentContext().Raise(e);
        Result.failed(rqst.RequestNumber);
    }

    return Result;
}

I read that Task.WhenAny.. will still continue calling the service in the background even after the time out, creating a leak, but I am not sure how to stop the call as the remote service does not take a CancellationToken.

The code works fine and returns the correct responses when it times out and when it doesn't.

*Additional follow-up question, does this code call the remote service multiple times? (at both var task = ... and Task.WhenAny...)

private async Task<MyResult> CheckItAsync(CheckRequest rqst, int timeout)

Never use abbreviations neither for method arguments not for variable/method/class names. A few more characters to make the name meaningful doesn't slow down the code nor does it increase the code size in a bad way. In fact you are saving yourself a lot of time if you come back in a few weeks/months to fix some bugs because you see at first glance what it is about.

MyResult Result = new MyResult()

This is a classname which doesn't tell what the class is about. Sure it will be some kind of result of some kind of operation but it neither tells what kind of result this is nor from which operation that result is coming. You could name it Result and it would have the same meaning (both times no meaning).

Variables names should be named using camelCase casing based on the .NET naming guidelines.

RemoteResponse response;

Variables should be placed as near to their usage as possible. Here you have placed this outside of the try..catch but you use it only in the try block.

Result.populate(response);

Methods should be named using PascalCase casing, hence populate should become Populate. The same is true for the other MyResult methods.

// Task timed out, populate result with timeout message
Result.timedOut(rqst.RequestNumber);

Comments should tell the reader why something is done in the way it is done. What is done should be told by the code itself using meaningful names for classes, methods, properties and variables. Furthermore comments shouldn't lie like this comment is doing. The result isn't populated with a timeout message but with the RequestNumber of the CheckRequest.

Because methodnames should be made out of verbs or verb phrases timedOut isn't a good name for a method.


Adapter for RecyclerView with supported onClick handling and select color text on last clicked item I have written an adapter for RecyclerView. It can handle onClicks for RecyclerView items and select the color name and edit TextView for the clicked item. By default the first item TextView name is the selected color.
When a user clicks another item the last item TextView is named black. When the the user clicks a color, such as granadier, then the TextView is changed to that color's name. And so on.

Can my code quality and code style be improved? Additionally I'm not a fan of, when a user clicks on new item, calling notifyDataSetChanged on all items.

AudioRecordsListAdapter.kt

class AudioRecordsListAdapter :
    RecyclerView.Adapter<AudioRecordsListAdapter.AudioRecordViewHolder>() {

    var items: MutableList<AudioRecordUI> = ArrayList()
        set(value) {
            if (value.size > 0) {
                value.first().isActive = true
            }
            items.clear()
            items.addAll(value)
            notifyDataSetChanged()
        }

    var isSelectableMode = false
    var onAudioRecordClickListener: OnAudioRecordClickListener? = null

    companion object {
        var dateFormatter: DateTimeFormatter = DateTimeFormatter.ofPattern("dd.MM.yyyy HH:mm")
        var timeFormatter: DateTimeFormatter = DateTimeFormatter.ofPattern("mm::ss")
    }

    override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): AudioRecordViewHolder {
        val view = LayoutInflater.from(parent.context)
            .inflate(R.layout.sound_record_item, parent, false)
        return AudioRecordViewHolder(view)
    }

    override fun getItemCount(): Int {
        return items.size
    }

    override fun onBindViewHolder(holder: AudioRecordViewHolder, position: Int) {
        holder.bind(items[position])
    }

    inner class AudioRecordViewHolder(itemView: View) : RecyclerView.ViewHolder(itemView) {

        private var audioRecordUI: AudioRecordUI? = null

        fun bind(audioRecordUI: AudioRecordUI) {
            this.audioRecordUI = audioRecordUI

            val size = formatFileSize(itemView.context, audioRecordUI.getSize())

            itemView.name.text = audioRecordUI.getName()
            itemView.date.text = audioRecordUI.getDate().format(dateFormatter)
            itemView.time.text = audioRecordUI.getTime().format(timeFormatter)
            itemView.size.text = size
            itemView.select.isChecked = audioRecordUI.isSelect

            if (audioRecordUI.isActive)
                itemView.name.setTextColor(itemView.context.getColor(R.color.grenadier))
            else
                itemView.name.setTextColor(itemView.context.getColor(R.color.black))

            setClickListener(audioRecordUI)
            setLongClickListener()
        }

        private fun setClickListener(audioRecorderUI: AudioRecordUI) {
            itemView.setOnClickListener {
                if (isSelectableMode) {
                    itemView.select.isChecked = !itemView.select.isChecked
                } else {
                    onAudioRecordClickListener?.onClick(audioRecorderUI.audioRecorderEmpty)
                    clearActive()
                    audioRecorderUI.isActive = true
                    notifyDataSetChanged()
                }
            }
        }

        private fun clearActive() {
            items.forEach { it.isActive = false }
        }

        private fun setLongClickListener() {
            itemView.setOnLongClickListener {
                isSelectableMode = !isSelectableMode

                if (!isSelectableMode) {
                    clearSelect()
                }

                itemView.select.isChecked = !itemView.select.isChecked
                true
            }
        }

        private fun clearSelect() {
            items.forEach { it.isSelect = false }
            notifyDataSetChanged()
        }
    }

    interface OnAudioRecordClickListener {
        fun onClick(audioRecordEmpty: AudioRecordEmpty)
    }

}

I like nitwitting about kotlin, therefor only feedback about the language.
single expression functions
If your function exists out of one expression, you can write it easier.
Instead of having to write:
override fun getItemCount(): Int {
    return items.size
}

you can write:
override fun getItemCount() = items.size

code duplication
Try to remove as much duplication as possible.
if (audioRecordUI.isActive)
    itemView.name.setTextColor(itemView.context.getColor(R.color.grenadier))
else
    itemView.name.setTextColor(itemView.context.getColor(R.color.black))

In the code above, you do three things:

Choose a colorId
Get the matching color
set the itemview to the color

Actions 2 and 3 are duplicated.
With a simple variable, this code becomes much clearer.
val textColorId = if(audioRecordUi.isActive) R.color.grenadier else R.color.black
itemView.name.setTextColor(itemView.context.getColor(textColorId))

scoped functions
When you need to reuse a variable multiple times after eachother, you can use a scoped function, eg also and apply.
instead of writing:
itemView.select.isChecked = !itemView.select.isChecked

you can write:
itemView.select.also { it.isChecked = !it.isChecked }

or even:
itemView.select.apply { this.isChecked = !this.isChecked }

And as you don't have to write this:
itemView.select.apply { isChecked = !isChecked }

Personal choice
I personally don't like the three lines for a simple if:
if (!isSelectableMode) {
    clearSelect()
}

If it's this short, I would place it on one line.
If it would be a bit longer, two lines without brackets.
if (!isSelectableMode) clearSelect()


errorlocation() routine, implemented in C++ based on pseudocode Recently I was reading a book called Code Complete by Steve McConnell and I came across a chapter called The PseudoCode Programming Process(PPP) where the chapter taught me on how to write Pseudo code and gave guidelines on how to convert that pseudo code to real code. That chapter inspired me a lot and I decided to follow that technique for writing all of my code. Inspired from the example given in the book I tried to write a simple routine that nearly follows those guidelines. Can you guys help me out on whether I am going in the right direction after looking the code below.

Here is the Pseudo code and real code that I wrote for a function called getErrorLocation that basically returns the coordinates of where the error occurred in a piece of code while parsing it.

Pseudo Code:

This routine outputs coordinate of error location found in gml code in
x/80 format based on the startlocation and currentlocation as supplied
by the calling routine. Also it outputs a error state indicating whether
the operation was successful
or not.

Set default value of error to FAIL

*Precondition
IF currentlocation<startlocation
    return FAIL
Calculate distance by subtracting currentlocation from startlocation
Divide distance by 80 and set the rownumber and colnumber to quotient and remainder of the division result respectively.
Set errorstate to SUCCESS
return coordinate with errorstate

Here is the C++ code written from the Pseudo Code given above

#include<iostream>
#include<cstdlib>

struct Point
{
    int x;
    int y;

};

/*This routine outputs coordinate of error location found in gml code in
x/80 format based on the startlocation and currentlocation as supplied by
the calling routine. Also it outputs a error state indicating whether the operation
was successful or not.*/

template<typename Iter>
std::pair<Point, bool>getErrorLocation(Iter locationcurrent, Iter locationstart)
{

    //Set default value of errorstate to FAIL
    bool error = true;

    //*Precondition

    //IF currentlocation<startlocation
    if (locationcurrent < locationstart)
        // return FAIL
        return { {0,0},error };

    //Calculate distance by subtracting currentlocation from startlocation
    int distance = locationcurrent - locationstart;

    //Divide distance by 80 and set the rownumber and colnumber to quotient and remainder of the division result respectively.
    auto [rownum, colnum] = div(distance, 80);
    //Set errorstate to SUCCESS
    error = false;
    //return coordinate with errorstate
    return { {rownum,colnum},error };
}

The main thing I want to highlight here are the comments that were actually the Pseudo code itself. I read in Clean Code by Robert C Martin that comments are harmful for the code and must be used to the minimum. I clearly understand why they are harmful, but I like this method also. Advantages weigh more I think for comments rather than their disadvantages. I feel like writing comments helps novices like me to understand the idea in the first glance itself rather than digging into the technical details and figuring out the code's real purpose. So now I am in a complete dilemma. There are contradictory statements from two of my favorite books.

What should I do?

Writing psuedo-code first, then turning it into code is great. However, we should then delete the pseudo-code.

The problem with comments like this is that they mainly just repeat the code. If we need a comment to explain what the code does, we should make the code clearer instead. Comments that explain why the code does something the way it does are more helpful.

Note also that if we change the code in the future, we would have to update all these comments too, which is twice the work.

Comments for documentation purposes tend to be structured according to the documentation tool in use (e.g. short description, purpose of arguments, return values, etc.). However, they are probably unnecessary for a small function that will be used internally in a project.

Some issues with the code:

Do we ever really expect locationStart to be after locationCurrent? Perhaps an assertion, or an exception would be more appropriate.
Otherwise std::optional would be a better choice for the return value.
We should use appropriate types for indexing. If the range of indices for Iter cannot be negative, and needs to cover the range of a standard container (e.g. std::string), we should use std::size_t, not int.
It's probably clearer to use the divide and remainder operators directly, instead of std::div (if that's what div is). Note that std::div, std::ldiv, and std::lldiv all use signed integer types, which probably don't cover the appropriate range.
The local error variable is unnecessary.
What is 80? Perhaps this should be a function argument (or at least a named constant).
We can be more consistent with naming. (The function is getErrorPosition, but we're returning a Point. Perhaps CharacterPosition would be a better name than Point).
Although it's called getErrorPosition, this function is not specific to errors - it just gets the character position from the iterators. If we give it a more general name, then we can use it for other things too.

So I'd suggest something like this (though the doc comments are probably not needed):

#include <cassert>
#include <cstdlib>
#include <iterator>

struct CharacterPosition
{
    std::size_t row;
    std::size_t column;
};

/// \brief Returns the character position (row / column) from its index.
/// \param index The distance of the character from start of the document.
/// \param lineLength The line length of the document (all lines must be this length for the returned character position to be correct).
/// \return Row and column of the character.
CharacterPosition getCharacterPosition(std::size_t index, std::size_t lineLength)
{
    return { index / lineLength, index % lineLength };
}

/// \brief Returns the character position (row / column) from its iterator position.
/// \param start An iterator pointing to the first character of the document.
/// \param current An iterator pointing to the current character (for which we will return the position).
/// \param lineLength The line length of the document (all lines must be this length for the returned character position to be correct).
/// \return Row and column of the character.
/// \pre The current iterator must not be positioned before the start iterator
template<typename Iter>
CharacterPosition getCharacterPosition(Iter start, Iter current, std::size_t lineLength)
{
    assert(start <= current);

    auto distance = std::distance(start, current);

    return getCharacterPosition(distance, lineLength);
}


Add method to Ruby's Enumerable module that produces every nth element The task is to enhance the Enumerable module with a method nth().

The method shall create a new list, which contains every nth-element of the object, when it is invoked.

For example: [2, 4, 6, 8, 10, 12].nth(2) shall return [4, 8, 12]

Here's my implementation:

module Enumerable
  def nth(n)
    i = 0
    ordinal = 1
    relevant_iteration = n * ordinal

    self.select do | num |
        i += 1

        if i == relevant_iteration
          ordinal += 1
          relevant_iteration = ordinal * n

          num
        end
    end
  end
end

print [2, 4, 6, 8, 10, 12].nth(2) # => [4, 8, 12]
print "\n"
print (4..12).nth(3)  # => [6, 9, 12]

It works as expected and it has passed the unit-tests.

But: My solution isn't elegant. At least that's my own impression.

Could my code be improved in regard to more compactness?
Is my code readable and understandable?
What could be improved?

nth slicing

Perhaps you could use a slice with map instead.

module Enumerable
  def nth(n)
    self.each_slice(n).select{|c| c.length == n}.map(&:last)
  end
end

unit tests

print [2, 4, 6, 8, 10, 12].nth(3) # => [6, 12]
print "\n"
print [2, 4, 6, 8, 10].nth(3) # => [6]
print "\n"
print [2, 4, 6, 8].nth(3) # => [6]
print "\n"
print [2, 4, 6].nth(3) # => [6]
print "\n"
print [2, 4].nth(3) # => []
print "\n"

Rubocop Report

Skipping an iteration should be done like this..

 next unless i == relevant_iteration

    ordinal += 1
    relevant_iteration = ordinal * n

rather than..

if i == relevant_iteration
  ordinal += 1
  relevant_iteration = ordinal * n

General Guidelines:

use 2 spaces for indentation
frozen string
n as parameter name: min length should be 3 -> takeIndex instead?

Refactored

# frozen_string_literal: true

module Enumerable
  def nth(takeIndex)
    i = 0
    ordinal = 1
    relevant_iteration = takeIndex * ordinal

    self.select do |num|
      i += 1

      next unless i == relevant_iteration

      ordinal += 1
      relevant_iteration = ordinal * takeIndex

      num
    end
  end
end


Find element yielding the largest value w.r.t. custom function I made a function to find the element that yields the largest value when put into a custom functor.

I think this is missing from the STL. I can think of three ways to do this in STL:

Transform the container by the functor. Find the maximum in the transformed version. Take the corresponding element from the original container.
Transform the container to an iterable of std::pairs containing the iterators to the original values and the corresponding functor result value. Write a custom comparison function which compares the .second of the pairs. Find max among the pairs with std::max_element. Take .first of the resulting iterator.
Use a custom comparator functor in std::max_element (return f(lhs) < f(rhs)).

The first two are really cumbersome and the third calls the functor twice as many times as necessary. Hence this:

template<typename Iterable, typename UnaryFun>
auto maxElementBy(Iterable const& iterable, UnaryFun criterion)
    -> decltype(std::begin(iterable))
{
    typedef decltype(criterion(*std::begin(iterable))) CriterionResult;

    auto itCurrent = std::begin(iterable);
    auto itEnd = std::end(iterable);

    if (itCurrent == itEnd)
    {
        return itEnd;
    }

    CriterionResult bestValue = criterion(*itCurrent);
    auto itBest = itCurrent;
    ++itCurrent;

    for (; itCurrent != itEnd; ++itCurrent)
    {
        CriterionResult currentValue = criterion(*itCurrent);
        if (bestValue < currentValue)
        {
            itBest = itCurrent;
            bestValue = std::move(currentValue);
        }
    }
    return itBest;
}

Any suggestions?

Setting aside the std::accumulate discussion, I have one recommendation. The signature

template<typename Iterator, typename UnaryFun>
auto maxElementBy(Iterator first, Iterator last, UnaryFun criterion)
    -> Iterator

seems to be more flexible. It allows partial ranges, and seems more in line with the std design approach.


Creating a special Toeplitz matrix in R I'd like to know if there is a standard way of creating a matrix like the one shown below using the R programming language.

$$
        \begin{bmatrix}
        1 & \rho & \rho^2 & \cdots & \rho^{T-1}\\
        \rho & 1 & \rho & \cdots & \rho^{T-2}\\
        \rho^2 & \rho & 1  & \cdots & \rho^{T-3}\\
        \vdots &\vdots &\vdots & \ddots & \vdots\\
        \rho^{T-1} & \rho^{T-2} & \rho^{T-3} & \cdots & 1
        \end{bmatrix}
$$

The code I've been using to create matrices like this one is shown below.

For illustrative purposes, let rho = 0.8 and T = 20.

rho <- 0.8
t   <- 20
toeplitz(c(1, poly(rho, t-1, raw=TRUE)))

Although the output of this code does, indeed, produce the matrix of interest, I get the feeling that there's probably a better way of doing it.

Can this code snippet be improved - in the sense that there is a more elegant, more standard, more correct, or more efficient way of creating the matrix?

In this case, poly is probably too much trouble. You can just calculate the polynomial yourself with rho^seq.int(0, t-1). Altogether that gives

toeplitz(rho^seq.int(0, t-1))


Erlang code to list all IP addresses This is some Erlang code I wrote to output a string of all the IP addresses of the machine. The string is simply printed to the user for him to read.

-module(ip).
-export([get_ip_address_string/0]).

get_ip_address_string() ->
    {ok, IPAddress} = inet:getif(),
    filter_ip_address(IPAddress).

filter_ip_address(IPAddress) ->
    filter_ip_address(IPAddress, []).

filter_ip_address([], Acc) ->
    Acc;
filter_ip_address([{{127,0,0,1}, _Broadcast, _Mask} | Tail], Acc) ->
    %Do not include the loopback address.
    filter_ip_address(Tail, Acc);
filter_ip_address([Head | Tail], Acc) ->
    {{Oct1, Oct2, Oct3, Oct4}, _Broadcast, _Mask} = Head,
    IPString = io_lib:format("~b.~b.~b.~b ", [Oct1, Oct2, Oct3, Oct4]),
    filter_ip_address(Tail, IPString ++ Acc).

Comments on how to write this in a more Erlang way, make it more readable, or anything you would do different are welcome.

First of all your variable IPAddress is misnamed. It sounds as if it contained a single IP address, but in fact it contains a list of multiple triples, where each one does not only contain an IP address, but also the corresponding broadcast address and netmask. You should probably call it something like IPTriples.

If you look at your code it's basically doing three things:

Filtering out localhost
Replacing each ip address with a string representation
Concatenating the thus created list of strings, separating them with spaces.

For each of these 3 steps there's already an erlang function, which can be used to do it:

lists:filter which takes a predicate and a list and returns a list only containing elements that match this predicate.
lists:map which takes a function and a list and returns a list containing the result of the function for each element.
string:join which takes a list of strings and a separator and returns a string with each item in the list separated by the separator.

Usually it it is good style to reuse existing functionality as much as possible instead of reimplementing it. So using these functions, your get_ip_address_string can be rewritten to something like this:

get_ip_address_string() ->
    {ok, IPTriples} = inet:getif(),
    FilteredIPTriples = lists:filter(fun is_not_localhost/1), IPTriples),
    IPAddresses = lists:map(fun ip_triple_to_string/1, FilteredIPTriples),
    string:join(IPAddresses, " ").

is_not_localhost({{127,0,0,1}, _Broadcast, _Mask}) -> false;
is_not_localhost({_IP, _Broadcast, _Mask})         -> true.

ip_triple_to_string({{Oct1, Oct2, Oct3, Oct4}, _Broadcast, _Mask}) ->
    io_lib:format("~b.~b.~b.~b", [Oct1, Oct2, Oct3, Oct4]).

Note that this has also the benefit that the logic for filtering out localhost, the logic for turning the IP addresses into strings and the logic for building the result string from that are now no longer intertwined, which should make the code more manageable.


Implementation of Dijkstra algorithm I am trying to implement Dijkstra algorithm. I try to calculate cost of reaching every node from first node. Though I have tried with couple of graphs and results are correct but I have one doubt which leads me to think that something is wrong with my implementation. Here is my implementation

private int[] findShortestPathBetween(int[][] graph, int src) {

    int[] result = new int[graph.length];
    for (int i = 0 ; i < result.length ; i++) {
        result[i] = Integer.MAX_VALUE;
    }

    result[src] = 0;

    boolean[] visited = new boolean[graph.length];

    int i = 0;
   while (true) {
        for (int kk = 0 ; kk < graph.length ; kk++) {
            if (graph[i][kk] != 0 ) { // edge between nodes exists
                int previousCost = result[i];
                if (previousCost == Integer.MAX_VALUE) {
                    previousCost = 0;
                }
                int newCost = previousCost + graph[i][kk];
                newCost = Math.min(newCost, result[kk]);
                if (newCost != result[kk]) {
                    visited[kk] = false; //weight of visited node changed, remove it from visited nodes.
                }
                result[kk] = newCost;
            }
        }
        visited[i] = true;

        int nextNode = -1;
        int nextNodeCost = Integer.MAX_VALUE;

        for (int k = 0 ; k < graph.length ; k++) {
            if (!visited[k]) {
                nextNode = k;
                break;
            }
        }

        if (nextNode == -1) {
            break;
        }

        i = nextNode;
   }
    return result;
}

I think line containing following code should not be required.

visited[kk] = false;

Here I am marking an already visited node as not visited if its cost has changed. Not doing this gives wrong results. As per my understanding this should not be required because if a node has been marked visited it should never be visited again.

I am trying to implement Dijkstra without using any queue (using queue is 2nd part of my implementation).

Please check and confirm whether my implementation has any gaps or this is expected when we are not using queue to implement Dijkstra algorithm.

Assumptions: I have used 2D array to represent my graph. If edge exist between 2 nodes corresponding entry in array is non-zero and if edge does not exist corresponding entry contains 0.

The graph is actually an adjacency graph. This is a much better name than graph to avoid people confusing it with a grid of nodes.

The next node should be the unvisited node with the lowest cost.

for (int k = 0 ; k < graph.length ; k++) {
    if (!visited[k] && result[k] < nextNodeCost) {
        nextNode = k;
        nextNodeCost = result[k];
    }
}

This way you avoid needing to visit nodes multiple times because you will then never change a visited node's cost.

The result array should be initialized with result[0] = 0; then you can remove the check in the for loop.


Space complexity of the "add two binary strings" challenge I was solving this problem of "adding two string binary numbers" and came up with the following algorithm. I am trying to analyze its time and space complexity. While I was satisfied with its time complexity which is \$O(n)\$, I am not really sure about the space. I think the space complexity is \$O(n)\$ as well but I might be wrong because of the StringBuilder usage here.

public String sumOfBinary(String x, String y) {
    if((x.length == 0 && y.length == 0) || (x == null && y == null))
       return;

    int i = x.length - 1, j = y.length - 1, carry = 0;
    StringBuilder sb = new StringBuilder();

    while(i > = 0 || j >= 0) {
     int sum = carry;
     if(i >= 0) {
       sum+= x.charAt(i--) - '0';
     }
     if(j>=0) {
       sum+=y.charAt(j--) - '0';
     }

     sb.append(sum%2);
     carry = sum/2;

    }
    if(carry!=0) {
      sb.append(carry);
    }
   return sb.reverse().toString();
}

It would be awesome if someone could review the code and let me know if there is any other better way to do this, along with what the space complexity is in the algorithm.

I agree with you, space complexity should be \$O(n)\$, using StringBuilder should not matter because, internally it would store char in an array which would expand / shrink as needed.

Now some code-reviews.

Error check:

Null check? x or y could be null. This (x == null || y == null) looks better.
Your code says "sumofbinary", but what if your string was "123" + "543"?

Name check:

sb is not a professional name, call it sum.

Syntax check:

while(i > = 0 || j >= 0) .. there is a space > = for i > = 0, but not for j.

Misc:

StringBuilder can be marked final.
You end for loop when both i & j are 0. This has a drawback, if i is longer than j, you would waste error checking if (j > 0).


Chain of CompletableFutures in Java I am new to this CompletableFuture function in Java. I have the following code below that works fine but looks quite messy IMHO.

CompletableFuture<String> cf = CompletableFuture.completedFuture(isRQMRunning_3);

if(cf.isDone()) {
    System.out.println("Value- " + cf.get());
    cf = CompletableFuture.completedFuture(startRQM_4);

    if(cf.isDone()) {
        System.out.println("Value- " + cf.get());
        cf = CompletableFuture.completedFuture(pendingChangesStatus);

        if(cf.isDone()) {
            System.out.println("Value- " + cf.get());
            cf = CompletableFuture.completedFuture(acceptPendingChanges_8);

            if(cf.isDone()) {
                System.out.println("Value- " + cf.get());
                cf = CompletableFuture.completedFuture(loadWorkspace_5);

                if(cf.isDone()) {
                    System.out.println("Value- " + cf.get());
                    cf = CompletableFuture.completedFuture(acceptPendingChanges_8);

                    if(cf.isDone()) {
                        System.out.println("Value- " + cf.get());
                        cf = CompletableFuture.completedFuture(fixConflicts_9);

                        if(cf.isDone()) {
                            System.out.println("Value- " + cf.get());
                        }
                    }
                }
            }
        }
    }
}

Is it possible to make that look a lot nicer/easier to read?

You have a lot of repetition in your code.
The only thing different with the 2 lines of code repeated over and over is the argument.

You could put all of these arguments into a list and iterate over them, running those 2 lines of code for each:

List<String> states = Arrays.asList(isRQMRunning_3, startRQM_4,
  pendingChangesStatus, acceptPendingChanges_8,
 loadWorkspace_5, acceptPendingChanges_8, fixConflicts_9);

for (String state : states)
{
    // Loop until array is complete
    CompletableFuture<String> cf = CompletableFuture.completedFuture(state);

    if (!cf.isDone())
    {
        break;
    }

    System.out.println("Value- " + cf.get());
}


Max path sum algorithm This is for Project Euler question 18 and I'm looking for feedback on whether my algorithm is incorrect or whether I've just implemented it wrong (which I don't think is the case after some testing).

The question is to traverse the maximum path in a triangle from top to bottom.

                            75
                          95  64
                        17  47  82
                      18  35  87  10
                    20  04  82  47  65
                  19  01  23  75  03  34
                88  02  77  73  07  63  67
              99  65  04  28  06  16  70  92
            41  41  26  56  83  40  80  70  33
          41  48  72  33  47  32  37  16  94  29
        53  71  44  65  25  43  91  52  97  51  14
      70  11  33  28  77  73  17  78  39  68  17  57
    91  71  52  38  17  14  91  43  58  50  27  29  48
  63  66  04  68  89  53  67  30  73  16  69  87  40  31
04  62  98  27  23  09  70  98  73  93  38  53  60  04  23

Starting from the second bottom row, each value is its current value + max(left child, right child), which transforms each position in the tree to be its max path sum from that point.

#include <iostream>
#include <fstream>
#include <iterator>
#include <sstream>
#include <vector>

using namespace std;
using Tree = vector<vector<int>>;

void transform_mps(Tree& tree) {
    // start from bottom up, each node's value is node.val + max(left child, right child)
    // transforms tree such that each node is that node's max path sum
    for (int layer = tree.size() - 2; layer >= 0; --layer) {
        for (size_t pos = 0; pos < tree[layer].size(); ++pos) {
            tree[layer][pos] = tree[layer][pos] + max(tree[layer+1][pos], tree[layer+1][pos+1]);
        }
    }
}

int main(int argc, char* argv[]) {
    if (argc < 2) { cout << "Need to pass in file to read from\n"; return 1; }

    ifstream f {argv[1]};
    if (!f.is_open()) { cout << "Could not open file\n"; return 1; }

    Tree tree;
    string line;
    while (getline(f, line)) {
        istringstream is {line};
        tree.push_back(vector<int>(istream_iterator<int>(is), istream_iterator<int>()));
    }
    transform_mps(tree);
    int mps {tree[0][0]};
    cout << "Max path sum: " << mps << endl;
}

/********** leftovers from previous implementation, ignore
constexpr bool LEFT {false};
constexpr bool RIGHT {true};

int pathsum(Tree& tree, size_t layer, size_t pos, bool right) {
    int sum {};
    ++layer;    // next level
    for (; layer < tree.size(); ++layer)
        if (right) sum += tree[layer][++pos];
        else sum += tree[layer][pos];
    return sum;
}
***********/

Looks good to me, algorithmically and syntactically. What makes you think it's wrong?

Stylistically, there are a few things you could improve:

Don't using namespace std. It's more idiomatic and readable to explicitly qualify std::max, std::cout, etc., and it's not much more typing.
Don't comment out dead code; delete it instead. ;)
tree[layer][pos] = tree[layer][pos] + ...; should be simply
tree[layer][pos] += ...;

And algorithmically, notice that if you start at the top of the tree and work downward, you don't need to store O(Rows²) numbers; you just need to store the previous row's sums, for a total space cost of O(Rows). (And then at the end you have Rows possible answers, and you pick the std::max_element of all of them.)

This might be a Project Euler constraint; but in the Unix/Linux world, it's more traditional to get your input from stdin rather than reading a filename off the command line. (Most utility programs support both, of course. The only one I know of that doesn't is tr.) Combine that with the top-down approach, and you get a nice Unixy stream filter that makes just one single pass over the data.


Go function to test whether a file exists How could one write in a more elegant way that if error is null, the result is true? I really need a bool as returned value.

func existsFile(pPath string) bool {
    _, errStat := os.Stat(pPath)

    if errStat != nil {
        return false
    }
    return true
}

Instead of checking with an if statement if errStat is null and then returning false:

if errStat != nil {
    return false
}
return true

you can return a Boolean expression:

return errStat == nil


Radix Sort Efficiency I'm implementing a (hopefully) efficient MSB Radix Sort in Java. My current implementation isn't efficient enough for my use-case (see bottom for a brief performance comparison against Arrays.sort).

Please rip me to shreds over any general improvements you can suggest; I am currently most interested in suggestions for performance improvements, though.

I do use recursion, as I'm only copying references in each stack frame (so I don't think I'm going to be taking much of a hit there). I allocate two buffers and each layer in the recursion tree uses one arrays as an input and one as an output - each node on each layer sorts a different bucket within the input, storing its results in the output before recursing.

// Sort "amount" integers from "input" and write them to "output".
protected void radicsSort(DataInputStream input, DataOutputStream output, int amount) throws IOException
{
    // Read in our buffer to memory
    int[] inputBuffer = new int[amount];
    for (int x = 0; x < inputBuffer.length; ++x)
        inputBuffer[x] = input.readInt();

    // Allocate an output buffer as big as the input one
    int[] outputBuffer = new int[amount];

    //// Sort the input  buffer
    radicsSort(inputBuffer, outputBuffer, 0, amount, Integer.SIZE - 1);

    // Input and output are swapped each layer of recursion, and we recurse as many times
    // as there are bits in an integer, so ensure the final output is in outputBuffer.
    // Commented out at the moment as we get a warning as technically we already know the result...
    //if (Integer.SIZE % 2 == 0)
        outputBuffer = inputBuffer;

    // Write the output to the file
    for (int x = 0; x < outputBuffer.length; ++x)
        output.writeInt(outputBuffer[x]);
}
// Sort the integers in "inBuffer" at indices between "start" inclusive and "end" exclusive,
// by the value of the bit at "bit", storing the results in "outBuffer" in the same
// interval as they were taken from
protected void radicsSort(int[] inBuffer, int[] outBuffer, int start, int end, int bit)
{
    // TODO: Check negative numbers are sorted correctly with this
    int zeroIndex = start; // Counts up from the start
    int oneIndex = end; // Counts down from the end

    for (int x = start; x < end; ++x)
    {
        if (((inBuffer[x] >> bit) & 1) == 0) // Bit is 0
            outBuffer[zeroIndex++] = inBuffer[x];
        else // Bit is 1
            outBuffer[--oneIndex] = inBuffer[x];
    }

    if (bit > 0)
    {
        // Recurse to sort the two sub-regions, moving one bit towards the LSB.
        // Swap the input/output buffers
        radicsSort(outBuffer, inBuffer, start, oneIndex, bit - 1);
        radicsSort(outBuffer, inBuffer, oneIndex, end, bit - 1);
    }
}

Performance comparison with Arrays.sort, times given in milliseconds:

Number of integers        Arrays.sort        Above implementation
1024                      1                  8328
1048576                   1221               19367

I understand that radix sort has a high constant overhead, but I thought it would have overcome that by the time we're sorting 4MiB of data.

The things I can think of to tweak are:

Use iteration instead of recursion (don't think this will change much).
Sort by eg. decimal digit instead of binary digit. Again, don't know how much of a performance impact this'll have.
Switch to eg. insertion sort after a threshold. I believe this is normally done to overcome the overhead of allocating new structures when you're working with small buckets. I think my implementation avoids this overhead anyway, by using a common output buffer and swapping input/output buffers, so I don't think this'll have much of an impact.

Post line-oriented timings from a profiling run, please. Where does it actually spend its time?
Curly braces {} are good for you, even when your for or if statement is a one-liner. Someone will be maintaining this code later, and it might not be you.
Please rename to radixSort(), from the Latin word for root.
Recursion overhead is simply not an issue here.
I imagine you're testing with random integers, so stability or reversing would not affect that. But think about timsort https://en.wikipedia.org/wiki/Timsort which is stable. If there are runs of non-descending input values, you would really like to preserve them. That would let you verify and terminate early.
Arrays.sort() runs in \$O(n \log{n})\$ time. Your radix sort is "efficient" in the sense that it runs in \$O(n)\$ time, but the notation can hide impressively large constants, like bits per Integer in your case. A linear read scan tries to blow out memory bandwidth, and L2 (last level) cache won't help you. You're not even working on cache-size subproblems with a subsequent merge. Yes, I agree that switching to an \$n\log{n}\$ sort for limited size subproblems would be helpful.
You mentioned decimal digits, and I see no reason to go there. But consider sorting into 4 buckets during a pass, or 8, or 16....


Program to shutdown computer after a certain time I'm trying to make a program that will shutdown my computer after a certain time that I will input via GUI. I'm wondering if I did it the correct way.

Also, will this slow down my computer while it waits to shut off? Do you see anything wrong with the code?

import java.io.IOException;
import java.util.Timer;
import java.util.TimerTask;

public class PCShutdown {
    private int minutes;
    private int hours;

    private void setHours(int hoursArg) {
        this.hours = hoursArg;
    }

    private void setMinutes(int minutesArg) {
        this.minutes = minutesArg;
    }

    private void shutDown() {
        Thread shutDownThread = new Thread(new ShutDownThread());
        shutDownThread.start();
    }

    private class ShutDownThread implements Runnable {

        @Override
        public void run() {
            Timer timer = new Timer();
            timer.schedule(new TimerTask() {

                @Override
                public void run() {
                    Runtime runtime = Runtime.getRuntime();
                    Process process = null;
                    try {
                        process = runtime.exec("shutdown -s -t 30");
                        System.exit(0);
                    } catch (IOException e) {
                        e.printStackTrace();
                    } finally { process.destroy();}
                }
            }, (hours * 60 * 60 * 1000) + (minutes * 60 * 1000));
        }
    }
}

Also, I still have to figure out how to kill all open programs except for this one before it shuts down. I'm going to work on that now.

Also, this might be a very dumb question, but will any of these operations harm my computer in any way? Will it be better if I just learn to do this via powershell?

Your questions at the end are the most important points here, so I answer those first:

  but will any of these operations harm my computer in any way?

No, and how could it anyway?

  Will it be better if I just learn to do this via powershell?

Yes! Writing a Java program for this is overkill!

In terms of a Java implementation, I think you're really overcomplicating it:

Why use a thread to do this? As you're not going to do anything else in parallel, it's pointless.
Why use a timer? Essentially the same argument applies here too as with threads: you're not doing anything else in parallel, so what's the point? And putting a timer inside a thread seems doubly pointless.

Maybe you assumed that by running this in a thread, the application will just schedule the task and exit. That is not the case. The JVM will continue running until the task is completed, and the program will stay in the foreground. You would have to background the process in the calling shell.

As such, you could replace this with a much simpler implementation:

public static void main(String[] args) throws InterruptedException, IOException {
    if (args.length < 2) {
        System.out.println("usage: java PCShutdown HOURS MINUTES");
    }
    int hours = Integer.parseInt(args[0]);
    int minutes = Integer.parseInt(args[1]);
    Thread.sleep(hours * 60 * 60 * 1000 + minutes * 60 * 1000);
    Runtime.getRuntime().exec("shutdown -s -t 30");
}

But then, I'd return to my earlier statement: Java is overkill for this, use the shell and native scheduling features instead (for example at in UNIX).


Optimize BFS weighted search in python I have a weighted BFS search in python for networkx that I want to optimize:

def bfs(graph, node, attribute, max = 10000):
    # cProfile shows this taking up the most time. Maybe I could use heapq directly? Or maybe I would get more of a benefit making it multithreaded?
    queue = Queue.PriorityQueue()
    num = 0
    done = set()
    for neighbor, attrs in graph[node].iteritems():
        # The third element is needed because we want to return the ajacent node towards the target, not the target itself.
        queue.put((attrs['weight'], neighbor, neighbor))
    while num <= max and not queue.empty():
        num += 1
        item = queue.get()
        if graph.node[item[1]].get(attribute, False):
            return item[2]
        for neighbor, attrs in graph[item[1]].iteritems():
            if neighbor not in done:
                queue.put((item[0] + attrs['weight'], neighbor, item[2]))
                done.add(neighbor)

Some things that might improve your code:

Use heapq rather than Queue.PriorityQueue. The latter does synchronization stuff that you don't need, so heapq will be faster.
Don't add neighbor to done in your inner loop. This will find incorrect routes on any graphs where some subpath A->B->C is shorter than A->C. Instead, you want to be be putting item[1] into done when you pop it off the queue. Since this means you will get some duplicated nodes in your queue, you'll also need to check if the newly popped node was already done before processing it. You might be able to save a little processing time by storing the cost of the path to the node in a dictionary, and only adding a new path if it's less than the previous one (but I don't think the improvement is very large).
This is a readability improvement (not a performance or correctness fix), but I suggest unpacking item into several values, rather than indexing it. cost, node, first_step = queue.get(), or something similar.
A terminology suggestion: Rather than being a basic breadth-first search, this is Dijkstra's Algorithm. If all the edge weights are equal, you'll examine nodes (roughly) the same order as in a BFS, but if you have different weights you'll go in a different order. It might be a good idea to use a function name that reflect what you're actually doing in the code.


Dictionary load function using hash table This loads a dictionary text file into memory to be used as part of a spell checker.  It's part of a larger program, but I wanted general comments so I can clean it up further.

#define TABLESIZE 500
#define LENGTH 45

bool load(const char* dictionary)
{
    //initiate hash table
    node* hashtable[TABLESIZE];

    //open dictionary and check
    FILE* dict = fopen(dictionary, "r");
    if (dict == NULL)
    {
        printf("Could not open file.");
        return false;
    }

    //initiate variable to store current word
    char* dword = calloc(LENGTH+ 1,sizeof(char));

    //read the file
    while(fscanf(dict, "%s", dword) != EOF)
    {
        //if there is a word, create node and put word in it
        node* new_node = malloc(sizeof(node));
        strcpy(new_node->word,dword);

        //find spot in hash table and put it in that bucket
        unsigned int hashkey= 0;
        for (int counter = 0; dword[counter]!= '\0'; counter++)
        {
            hashkey = (hashkey*dword[counter] + dword[counter] + counter)%TABLESIZE;
        }

        //check if spot in table exists; if not, start the linked list
        if (hashtable[hashkey] == NULL)
        {
            hashtable[hashkey] = new_node;
            new_node->next = NULL;
        }
        //otherwise made current node the first, shift rest over
        else
        {
            new_node->next = hashtable[hashkey];
            hashtable[hashkey] = new_node;
        }
        //count words for later use
        nwords++;

    }

    fclose(dict);
    return true;
}

Your constants could be named slightly differently; consider TABLE_SIZE and MAX_WORD_LENGTH.
You could also rename dictionary to better indicate that it's a file path and not a dictionary object.
Consider printing error output to stderr: fputs(stderr, ...) or fprintf(stderr, ...).
You're using a global nwords, but a local hashtable; this looks to me like a scoping error. You could consider just making a new struct.


Summing up distinct elements in steps My current task is to find a score from an array where the highest/lowest scores have been taken away, and if the highest/lowest occur more than once (ONLY if they occur more than once), one of them can be added:

E.g. int[] scores = [4, 8, 6, 4, 8, 5] therefore the final addition will be \$\sum{4,8,6, 5} = 23 \$.

Another condition of the task is that LINQ cannot be used, as well as any of the System.Array methods (you can see by my previously ask questions that has been a bit of a pain for me, since I solved this with LINQ in less than 5 minutes).

I have working code the solves the problem but the task requires multiple methods/functions. I have been trying to restructure the program but with all sorts of issues.

using System;
using System.Collections.Generic;
//using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Scoring {
class Program {

    static int highOccurrence = 0;
    static int lowOccurrence = 0;
    //static int high; <------
    //static int low; <------

    static void Main(string[] args) {
        int[] scores = { 4, 8, 6, 4, 8, 5 };

        findScore(scores);
        ExitProgram();
    }

    static int findOccurrence(int[] scores, int low, int high) { //find the number of times a high/low occurs

        for (int i = 0; i < scores.Length; i++) {
            if (low == scores[i]) {
                lowOccurrence++;
                //record number of time slow occurs
            }

            if (high == scores[i]) {
                highOccurrence++;
                //record number of times high occurs                }
        }
        return highOccurrence;
    }

    static int findScore(int[] scores) { //calculates score, needs to be restructured

        int[] arrofNormal = new int[scores.Length];

        //int low = scores[0]; <----This is where the issue is
        //int high = scores[0]; <----- ^^^^^
        int total = 0;

        for (int i = 0; i < scores.Length; i++) {
            if (low > scores[i]) {
                low = scores[i];
            } //record lowest value

            if (high < scores[i]) {
                high = scores[i];
                //record highest value
            }
        }
        for (int x = 0; x < scores.Length; x++) {
            if (scores[x] != low && scores[x] != high) {
                arrofNormal[x] = scores[x];
                //provides the total of the scores (not including the high and the low)
            }
            total += arrofNormal[x];
        }

        findOccurrence(scores, low, high);
        if (highOccurrence > 1) { //if there is more than 1 high (or 1 low) it is added once into the total
            total += high;
            if (lowOccurrence > 1) {
                total += low;
            }
        }
        Console.WriteLine("Sum = " + total);
        return total; //remove not all code paths return.. error
    }
    static void ExitProgram() {
        Console.Write("\n\nPress any key to exit program: ");
        Console.ReadKey();
    }//end ExitProgram
}
}

I have placed arrows in the code above to show where my issue is. If I try to declare "high" and "low" as global variables, my final answer is always a few numbers off, buy if I leave the variables declared as high = scores[0] etc, I will get the right answer.

What I want ideally is to have separate methods for each step of the calculation, so right now I have method for finding the number of times a specific value shows up in the array.

The next I would like to do is finding the highest/lowest value in the array, one method would do the final calculation, and the final one would write the results into the console window. The last two parts (finding the high/low and final calculation) are currently in the find score method.

My current task is to find a score from an array where the
  highest/lowest scores have been taken away, and if the highest/lowest
  occur more than once (ONLY if they occur more than once), one of them
  can be added

So the programflow can be written as:

Find the highest item of the array
Find the lowest item of the array
Find the number of occurance of the highest item of the array
Find the number of occurance of the lowest item of the array
Remove the first highest item found
Remove the first lowest item found
Sum together the remaining items
Export/Print the results

So you will have 8 methods without the ExitProgram() method.

Review

Naming

Based on the naming conventions for C#, all methodnames should be written using the PascalCasing style. So findOccurrence should be FindOccurrence and findScore should be FindScore

So based on the list of tasks above the methods should be named

FindHighestItem() or GetHighestItem()
FindLowestItem() or GetLowestItem();
FindNumberOfHighestOccurance() or GetCountOfOccurance()
FindNumberOfLowestOccurance() or GetCountOfOccurance()
RemoveHighestItems() or SetArrayitemsByValueToZero()
RemoveLowestItems() or SetArrayitemsByValueToZero()
GetHighestItemIndex()
GetLowestItemIndex()
GetNormalizedArray() which returns a new array or
SetArrayitemsByValueToZero() which manipulates the items in the array
SumItems()
Print()

Style

Opening brackets should be placed on the line below the statement so

for (int i = 0; i < scores.Length; i++) {

should be

for (int i = 0; i < scores.Length; i++)
{

Programflow

By combining some of these tasks you are violating the single responsible principle.
The findScore() method

is searching for the highest and lowest item in the array
removes the highest and lowest item from the array (basically add only the items which aren't highest or lowest to the new array)
is summing the items of this new array stated above
is printing/exporting the sum

Refactoring

For getting the highest/lowest item in the array we can use the Math.Max() and Math.Min() methods. For finding the highest item we will initialize the var highestValue with Int32.MinValue and for finding the lowest item we will initialize the var lowestValue with Int32.MaxValue.
Based of the comment as it is not allowed for this assignment to use Math.Min() or Math.Max() I have changed the methods

    private int GetLowestItem(int[] items)
    {
        int lowestItem = Int32.MaxValue;
        foreach (int item in items)
        {
            // lowestItem = Math.Min(item, lowestItem);
            if (item < lowestItem)
            {
                lowestItem = item;
            }
        }
        return lowestItem;
    }

    private int GetHighestItem(int[] items)
    {
        int highestItem = Int32.MinValue;
        foreach (int item in items)
        {
            // highestItem = Math.Max(item, highestItem);
            if (item > highestItem)
            {
                highestItem = item;
            }
        }
        return highestItem;
    }

    private int GetCountOfOccurance(int[] items, int comparingValue)
    {
        int count = 0;
        foreach (int item in items)
        {
            if (item == comparingValue)
            {
                count++;
            }
        }
        return count;
    }

private int GetHighestItemIndex(int[] items, int highestItem)
{
    for(int i = 0 ; i<items.Length ; i++)
    {
        if(items[i] == highestItem)
        {
            return i;
        }
    }
    return -1;
}

private int GetLowestItemIndex(int[] items, int lowestItem)
{
    // to be filled by you
}

Next we should create a new calssed named BoundaryItem which holds the min and max values of the array.

class BoundaryItem
{
    internal int Max { get; set; }
    internal int Min { get; set; }
}

now we add a method to set each item of the array to zero if the value is either the max or the min of the array.

private void SetArrayitemsByValueToZero(int[] items, BoundaryItem boundaryItem)
{
    for (int i = 0; i < items.Length; i++)
    {
        if (items[i] == boundaryItem.Min || items[i] == boundaryItem.Max)
        {
            items[i] = 0;
        }
    }
}

Next we will add a 2 overloaded SumItems() methods

public int SumItems(int[] items)
{
    BoundaryItem boundaryItem = PreProcessArray(items);

    return SumItems(items, boundaryItem);
}

private int SumItems(int[] items, BoundaryItem boundaryItem)
{

    int sum = boundaryItem.Min + boundaryItem.Max;
    foreach (int item in items)
    {
        sum += item;
    }
    return sum;
}

and last we add the missing PreProcessArray() method

private BoundaryItem PreProcessArray(int[] items)
{
    BoundaryItem boundaryItem = new BoundaryItem();

    boundaryItem.Min = GetLowestItem(items);
    boundaryItem.Max = GetHighestItem(items);

    int minOccurance = GetCountOfOccurance(items, boundaryItem.Min);
    int maxOccurance = GetCountOfOccurance(items, boundaryItem.Max);

    SetArrayitemsByValueToZero(items, boundaryItem);

    if (minOccurance < 2) { boundaryItem.Min = 0; }
    if (maxOccurance < 2) { boundaryItem.Max = 0; }

    return boundaryItem;
}

But wait, we can still do better, if the amount of methods needed would not matter. Instead of GetLowestItem() and GetHighestItem() we create a method GetBoundaryItem()

private BoundaryItem GetBoundaryItem(int[] items)
{
    BoundaryItem boundaryItem = new BoundaryItem();
    boundaryItem.Min = Int32.MaxValue;
    boundaryItem.Max = Int32.MinValue;

    foreach (int item in items)
    {
        if (item < boundaryItem.Min)
        {
            boundaryItem.Min = item;
        }
        else if (item > boundaryItem.Max)
        {
            boundaryItem.Max = item;
        }
    }
    return boundaryItem;
}

and change the PreProcessArray() method

private BoundaryItem PreProcessArray(int[] items)
{
    BoundaryItem boundaryItem = GetBoundaryItem(items);

    int minOccurance = GetCountOfOccurance(items, boundaryItem.Min);
    int maxOccurance = GetCountOfOccurance(items, boundaryItem.Max);

    SetArrayitemsByValueToZero(items, boundaryItem);

    if (minOccurance < 2) { boundaryItem.Min = 0; }
    if (maxOccurance < 2) { boundaryItem.Max = 0; }

    return boundaryItem;
}

The printing I will leave to you. As you see I have made instance methods out of the static methods. So it would be a good idea to create a class named Scoring where you place these methods.Then you would call this like

static void Main(string[] args) {

    Scoring scoring = new Scoring();

    int[] scores = { 4, 8, 6, 4, 8, 5 };
    int sum = scoring.SumItems(scores);
    // now print the result
    ExitProgram();
}

See: When to Use Static Classes in C#

As I have written the refactorings I came to the conclusion that we just need to get the index of the highest/lowest items so we can skip those items at composing the new array.
After the coment from Malachi I realized that using the index of the highest/lowest item will be an invalid aproach if these values are contained more than 2 times in the array.
This implementation assumes that for the given array [1 ,1 ,1 ,2 ,3 ,3 ,3]  it's sum should be 1 + 2 + 3 .


Long switch statement to lookup fifty-something commands I have a very long switch statement which is expected to have nearly 50-55 cases.

switch (commandType) {
            case COMMAND_LOGIN:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_MAKE_READY:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_MAKE_NOT_READY:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_MAKE_NOT_READY_WITH_REASON:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_LOGOUT_WITH_REASON:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_ANSWER_CALL:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_HOLD_CALL:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_RETRIEVE_CALL:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_END_CALL:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_TRASFER_SST_CALL:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_CONSULT_CALL:
                command = getCommand(commandType, agentId);
                break;
                 .
                 .
                 .
                 .
                 .
           default :
                command = "";
                break;

        }//end switch

It doesn't seem to be something which should exist in this current form, I have gone through some questions here 'but they doesn't seem to help in this condition' I have seen the "strategy pattern", but that also seems to be suitable for the case where i have to execute different method for every case. Kindly guide me how the size of this code can be reduced.

Giant switch statements are a code smell.  In Java, the usual remedy is to refactor using the Replace Conditional with Polymorphism recipe.

However, I think your problem runs even deeper.  I raise the following objections:

Why should this code exist at all?  Shouldn't getCommand(commandType, agentId); just return the appropriate result for all values of commandType?  Or are there specific command types that you want to disable?  The latter possibility seems disturbing.  If this code is, say, part of a permission-checking system to allow only certain types of commands, I would say that the permission-checking system is woefully underdeveloped.
Why does getCommand(…) return a String?  What use could a constant-to-string translation possibly have?

I wish I could give you more constructive suggestions, but unfortunately, you haven't presented much context in the question to allow a deeper analysis.


Best way to preload images with callback on completion with pure JavaScript (ES6) There are many related questions and answers but mostly with JQuery and/or from many years ago.

I prepared a version myself, and my question is whether this is currently (2020, hence ECMAScript 6 or similar) an optimal way to preload images in pure JavaScript. It appears to work (note the tests), although not all edge-cases are tested.

let preload_imgs = function(src_list, callback) {
    let loaded = 0;
    src_list.forEach((src) => {
        img = new Image();
        img.onload = () => {
            if (++loaded == src_list.length && callback) {
                callback();
            }
        };
        img.onerror = (err) => {
            console.log(err);
        };
        img.src = src;
    });
};

Here is an example usage:

let preload_imgs = function(src_list, callback) {
  let loaded = 0;
  src_list.forEach((src) => {
    img = new Image();
    img.onload = () => {
      if (++loaded == src_list.length && callback) {
        callback();
      }
    };
    img.onerror = (err) => {
      console.log(err);
    };
    img.src = src;
  });
};

let urls = ['https://i.picsum.photos/id/1000/5626/3635.jpg',
  'https://i.picsum.photos/id/10/2500/1667.jpg',
  'https://homepages.cae.wisc.edu/~ece533/images/cat.png',
  'https://homepages.cae.wisc.edu/~ece533/images/airplane.png'
];

let starttime = Date.now();
preload_imgs(urls, () => {
  console.log("All images preloaded in", Date.now() - starttime, 'ms.');
});

Firstly, kudo's to you for trying to use vanilla javascript. Great choice.

 The Question
Let us start with your question:

[Is this code] an optimal way to preload images in pure JavaScript?

Sorry, but that is very subjective so I cannot answer that. I am assuming good will, and thus will rather answer the following question I inferred from your original question:

Am I making good use of common best practices?

Now that is a question I can answer, and the simple answer is no. Sorry :)
Ofcourse, I will not leave you an answer without explanation.

 Consistency and Deliberateness
Firstly, the biggest improvement one can make in their life as a programmer would be consistency. Do not listen to the people who argue about single vs double quotes, or tabs vs spaces. As a software engineer myself, I can wholeheartedly say that all that matters is when you make a choice, you stick to it (at least within the project -- you are allowed to change your mind). Most people do not mind style (it often is enforced to a company standard anyway), but whenever you use any function or syntax, a reader would assume you did so deliberately. Why did you choose both var function and x => y?
I have to admit that javascript tricked us by allowing three ways of creating functions, but that does not mean you should use them all together. Let us look at the ways and their reasons:
function name (args) { /*block*/ }
This is the function declaration statement. This just means that you declare a variable called name and assign the function to it. This was made primarily to support an imperative sequence of steps. It is my personal opinion that it should have been named procedure, because that is what it is.
var name = function (args) { /* block */ }
This is actually what happens when you declare a function, so there is no difference between this and the first type of function -- except for the more letters so I see no reason to use this notation. The only difference that could be made is through the storage modifier (i.e. var in this case), which I will tell you about a bit later.
var name = args => { /*block*/ }
This is javascript's lambda syntax. It allows for a very interesting way to write programs, but only if you stop appending blocks to it:
var add = (x, y) => {
  return x + y;
};
// is actually the same as
var add = (x, y) => x + y;

The arrow function (x => y) is a function that exists of only an expression, with an implied return. But if you use a block, you just turned it into a procedure again which negates the usefulness of the lambda/arrow notation. It can save you a lot of keystrokes, and, if I can add my opinion, looks much more clean.
Taking this into consideration, your code could look like this:
function preload_imgs (src_list, callback) {
    let loaded = 0;
    src_list.forEach(function(src) {
        img = new Image();
        img.onload = function() {
            if (++loaded == src_list.length && callback) {
                callback();
            }
        };
        img.onerror = function(err) {
            console.log(err);
        };
        img.src = src;
    });
};

 Storage and Mutability
Remember the storage modifiers I just talked about? Javascript also has three ways of declaring variables. What a nightmare, right? Well, there is some use to the differences -- have a look:
var x = ...
In the beginning there was only var. It is short for variable, and thus should just declare a variable, right? Nope, javascript is weird. I could go on about why it is the weirdest feature ever, but here is the link about var if you are interested. The main takeaway is that this notation does a lot of stuff behind the scenes, making it a common source of bugs. My opinion: don't use it.
let x = ...
Everyone who has ever used var probably meant to use let. It does not do any fancy stuff, it creates a variable and assigns a value, except that it is block scoped. This variable is only ever accessible within the confines of the closest block, i.e. anything within { curly brackets }. You use only let, so I assume you know of this advantage but I wanted to clarify it nonetheless.
const x = ...
Again, a very autological name: const creates a constant. A constant can only be declared once, and never overwritten. This might sound like a useless trait, but it is actually really important. One of the most famous bugs in the world was caused by mutability (no. 4 on the list, probably more) and cost a whopping $60 million. I am not saying that using const will always save you millions of dollars, but it disallows any overwriting of the value, meaning:

you cannot accidentally overwrite your or anyone else's functions and variables.
you can guarantee that its value will never change, so you can refer to it anywhere safely -- even in a for within a for within a for.
AT&T would have lost $60m less.

Coming back to deliberateness: you declare functions using let, implying you are going to change them, but you are not. I suggest using const to more clearly signal your intentions:
const preload_imgs = function (src_list, callback) {
    let loaded = 0;
    src_list.forEach(function(src) {
        const img = new Image();
        img.onload = function() {
            if (++loaded == src_list.length && callback) {
                callback();
            }
        };
        img.onerror = function(err) {
            console.log(err);
        };
        img.src = src;
    });
};

 I promise to help
Talking about common best practises, there is one thing many javascript developers will run into. That thing is the internet. A great many developers have already walked this path before you, so you bet the industry has some handy stuff up its sleeves to help you become a better programmer.
Javascript has a fool-proof way of handling asynchronous tasks, such as preloading an image. For exactly this reason, promises were implemented. A Promise is a wrapper around an asynchronous task, so we can work with it synchronously. Without going into a lot of detail, let me summarise the most important parts of a Promise:

A promise allows for attaching "callbacks" for when the wrapped task completes

const response = new Promise(request);
response.then(doStuff);

A promise allows for attaching "callbacks" for when the wrapped task fails

const response = new Promise(request);
response.catch(handleError);

Can you see a similarity? Creating an Image and attaching onload/onerror callbacks sounds a lot like creating a Promise! Applying this knowledge to your example gives us the following code:
// Your "preload" function goes as follows
(src) => {
    img = new Image();
    img.onload = () => {
        if (++loaded == src_list.length && callback) {
            callback();
        }
    };
    img.onerror = (err) => {
        console.log(err);
    };
    img.src = src;
}
// Let's update it
const preload = src => new Promise(function(resolve, reject) {
  const img = new Image();
  img.onload = function() {
    // onload is called with an event, not the image so we
    // want to specifically resolve with img.
    resolve(img);
  }
  img.onerror = reject;
  // onerror is called with the error, so we can directly assign "reject" to it.
  img.src = src;
});

You might notice the lack of "free variables" (foreign variables that do not contribute to the function itself). We do not use loaded or src_list because those alter state / are mutable and are a perfect souce for the $60m bug (actually, this is probably the exact type of function that caused the race condition in their software).
We have solved the preload function in a very clean way:
const preload = src => new Promise(function(resolve, reject) {
  const img = new Image();
  img.onload = function() {
    resolve(img);
  }
  img.onerror = reject;
  img.src = src;
});

preload("https://s3.amazonaws.com/pix.iemoji.com/images/emoji/apple/ios-12/256/thumbs-up.png");
  .then(img => console.log("Preloaded", img))
  .catch(err => console.error("Failed", err));

BAM! You now have a function that preloads an image, and allows for individual succes and error handling. All we have to do now, is to apply this function to a list of srcs.

➰ Going in loops
You might have thought there were too many ways to declare functions or variables, but wait till you hear in how many ways you can loop over things in javascript!
There is for (let i = 0...), for ... in, for ... of, while, do ... while, and we have not even touched on higher order array traversal functions. I am just going to say it: you never really need a for loop. You can solve any array problem with array functions.

Side note: the Array.forEach function you used is a weird function. It is just a slower version of a for loop with different notation, and I just said you never need a for loop. Either use for (const src of src_list) or use map or filter, but never forEach (this is a guideline, if it prohibits you from being productive then go ahead and do it).

For example, there is the Array.map function. This function takes a transformer, which goes over every element of the array, and transforms that element yielding a new array. That might sound weird so here is an example:
const numbers = [1, 2, 3];
const double = x => x * 2;
const doubledNumbers = numbers.map(double);
// doubledNumbers: [2, 4, 6]

Hmm, taking an array of values and applying a function to every value sounds a lot like what you are trying to do:
// Your old code
src_list.forEach((src) => {
  img = new Image();
  img.onload = () => {
    if (++loaded == src_list.length && callback) {
      callback();
    }
  };
  img.onerror = (err) => {
    console.log(err);
  };
  img.src = src;
});

// Using our "preload" function from before
const requests = src_list.map(preload);
// requests: [Promise<Image>, Promise<Image>, Promise<Image>, ...]

Whoaaaa, one line! But we have not fully refactored yet, because we now have a list of promises that can just resolve at any time and in no particular order. You seem to have understood this problem, but you solved it in a very unsafe way.
let loaded = 0;
//...
img.onload = () => {
    if (++loaded == src_list.length && callback) { /* ... */ }
    //...
}

You keep track of the amount of loaded images in the callback function that triggers when any image is loaded. Pretty clever, but I would bet my house that keeping track of such things in this way will cause bugs. If you google "wait for all promises javascript", you learn of a built in function that takes care of all of this for you! Promise.all takes a list of promises, and returns a promise, that rejects when any promise in that list rejects, or resolves when all promises in the list have resolved. That is exactly what you are trying to achieve:
//using the "requests" list we created a couple examples back
const done = Promise.all(requests);
done
  .then(images => console.log("Preloaded all", images))
  .catch(err => console.error("Failed", err));
// > Preloaded all [Image, Image, Image, ...]

⌛ Conclusion
Hopefully you now have a better understanding of javascript, and the lovely built-in functions that make these things almost as easy as jquery, if not more. Maybe you even learned a lot of things along the way, maybe you were sighing and moaning because you knew most of this stuff -- as long as you learned anything I consider it a success.
To summarise:

Use any kind of function notation but be consistent and deliberate.
Use const for constants and let for variables (I never even use let at all).
Use Promises to wrap asynchronous tasks to allow for really useful manipulation functions and callbacks.
Familiarise yourself with higher-order functions so you never have to use loops again.

And voila, the refactored code:
const preload = src => new Promise(function(resolve, reject) {
  const img = new Image();
  img.onload = function() {
    resolve(img);
  }
  img.onerror = reject;
  img.src = src;
});

const preloadAll = sources =>
  Promise.all(
    sources.map(
      preload));

const sources = [
  'https://i.picsum.photos/id/1000/5626/3635.jpg',
  'https://i.picsum.photos/id/10/2500/1667.jpg',
  'https://homepages.cae.wisc.edu/~ece533/images/cat.png',
  'https://homepages.cae.wisc.edu/~ece533/images/airplane.png'];

preloadAll(sources)
  .then(images => console.log('Preloaded all', images))
  .catch(err => console.error('Failed', err));

Preload to your hearts desire.


For finding whether a puzzle state is solvable I have code which returns whether or not a sliding tile puzzle state is solvable.
I was wondering whether there is any way I could either make the code a bit more readable or increase the performance in any way.

  public boolean isSolvable(int[] puzzle) {
        int parity = 0;
        int gridWidth = (int) Math.sqrt(puzzle.length);
        int row = 0; // the current row we are on
        int blankRow = 0; // the row with the blank tile

        for (int i = 0; i < puzzle.length; i++) {
            if (i % gridWidth == 0) { // advance to next row
                row++;
            }
            if (puzzle[i] == 0) { // the blank tile
                blankRow = row; // save the row on which encountered
                continue;
            }
            for (int j = i + 1; j < puzzle.length; j++) {
                if (puzzle[i] > puzzle[j] && puzzle[j] != 0) {
                    parity++;
                }
            }
        }

        if (gridWidth % 2 == 0) { // even grid
            if (blankRow % 2 == 0) { // blank on odd row; counting from bottom
                return parity % 2 == 0;
            } else { // blank on even row; counting from bottom
                return parity % 2 != 0;
            }
        } else { // odd grid
            return parity % 2 == 0;
        }
    }

You can use a fun fact from abstract algebra.  It's a group theory fact, to be more specific.  Read all about it here or here or, better yet, here.  I actually used this in my own sliding tile puzzle I created for the App Store in 2013.  But I used it to generate puzzles, not to verify solvability.

I see now where my original intended answer went wrong.  The math behind it requires the empty space be in the bottom right corner, and I missed that at first.  From the Wiki link:

  In particular if the empty square is in the lower right corner then the puzzle is solvable if and only if the permutation of the remaining pieces is even.

A permutation of numbers 1 through n is even if and only if it's obtained by performing an even number of swaps.  For example, the permutation 2 1 4 3 is an even permutation of 1 2 3 4 because it's obtained from two swaps: swapping 1 and 2, and swapping 3 and 4.  And the permutation 1 3 2 4 is not an even permutation (a so-called odd permutation) because it's obtained from one swap: swapping 2 and 3.  Note that the number of swaps is not unique for a given permutation, but the parity of the number of swaps is unique.

If you want to keep things as simple as possible then my recommendation to you is to use the following general process:

Start with a puzzle in the solved state.
Let m be a random positive even integer.
Swap m pairs of non-empty tiles (That is, leave the empty space where it is, in the bottom right corner).

Of course, you'll need some checks to make sure you don't swap the same pair several or even m times.  But this is by far the most efficient way to do this unless you consistently choose massively huge values of m.  This is what I did in my own app.

But anyway, if you want to stick with your original process of randomly generating a puzzle and seeing if it's solvable, then you can still use permutations to check solvability in O(n) time, where n = puzzle.length.

We already know the following:

  A puzzle is solvable if and only if the empty space is in the bottom right corner and the permutation is even.

Therefore for a randomly created puzzle we also need to figure out the Manhattan distance between the empty space and the bottom right corner.  This is equal to the number of rows the empty space is away from the last row plus the number of columns the empty space is away from the last column.  For simplicity I'll assume your board will always be square, and I think that's a reasonable assumption based on your original code.  Then with this Manhattan distance between where the empty square is and where it's supposed to be, a distance which I'll denote x, we have the following fact:

  A puzzle is solvable if and only the parity of the permutation equals the parity of x.

I won't rigorously prove this, but the basic idea is that every move you make will move the empty square and therefore can be thought of as a swap with the empty square.  And the empty square is an even Manhattan distance away (i.e., x is even) if and only if it takes an even number of swaps to get the empty square where it belongs.  Also, a composition of two permutations is even if and only if those two permutations have the same parity.

And, finally, here's the code:

public boolean isSolvable(int[] puzzle) {
    int [] myPuzzle;  // Copy puzzle because we'll be swapping elements.
    int [] indexArr;  // indexArr[0] is index of blank space in puzzle[].
                      // indexArr[1] is index of space #1 in puzzle[].
                      // indexArr[2] is index of space #2 in puzzle[], etc.
    int n = puzzle.length;
    int swapCount = 0;  // Number of swaps to get from shuffled state to solved state.
    int manDist = 0;  // Manhattan distance between shuffled empty space and bottom right corner.
    int gridWidth = (int) Math.sqrt(puzzle.length);
    int row, col;  // Row and column of the empty space in the shuffled puzzle.

    myPuzzle = new int[n];
    indexArr = new int[n];

    // Make a deep copy so we don't modify the original.
    // Also store indexes of the shuffled tiles.
    // This loop runs in O(n) time.
    for (int i = 0; i < n; i++) {
        myPuzzle[i] = puzzle[i];
        indexArr[puzzle[i]] = i;
    }

    // First get the Manhattan distance between where the empty space is and where it should be.
    row = indexArr[0] / gridWidth + 1;  // Row of empty space's shuffled position.  Add 1 to account for 0-based index.
    col = indexArr[0] % gridWidth + 1;  // Column of empty space's shuffled position.  Add 1 to account for 0-based index.
    manDist = 2*gridWidth - row - col;  // (gridWidth-row) + (gridWidth-col)

    // Count how many swaps we need to get to the solved state.
    // This loop runs in O(n) time.
    for (int i = 0; i < n; i++) {
        // Swap tile #i with whatever tile is in tile #i's correct position.
        // The current position for tile #i is indexArr[i].
        // The correct position for tile #i is (i+n-1) % n
        //   because the blank space #n is actually tile #0.
        // Thus we want to swap puzzle[indexArr[i]] with puzzle[(i+(n-1)) % n],
        //   but only if the swap is needed.
        // Note that we must also swap the corresponding stored indexes in
        //   indexArr so future loop iterations will work correctly.

        if (myPuzzle[indexArr[i]] != myPuzzle[(i+n-1) % n]) {  // If we need to swap
            // Swap the tiles, i.e., put tile #i in its proper place.
            swap(myPuzzle, indexArr[i], (i+n-1) % n);

            // Now swap their stored positions in indexArr.
            swap(indexArr, i, myPuzzle[indexArr[i]]);

            swapCount++;
        }
    }

    // Puzzle is solvable if and only if the permutation and manDist have the same parity.
    return (swapCount % 2 == manDist % 2);
}

private void swap(int[] arr, int a, int b) {
    arr[a] += arr[b];
    arr[b] = arr[a] - arr[b];
    arr[a] -= arr[b];
}

This runs in O(n) time overall.  I tested with a few samples and this agrees with your original implementation in all cases.


My first controller rspec test I'm just started learning TDD with Rails and RSpec. This is my first test for controller. Just plain RESTful UserController that responds with JSON, so it has no new and edit methods. Please review it

require 'spec_helper'

describe UsersController do
  let(:users) { 4.times.map { create(:user) } }

  describe '#index' do
    before(:each) { get :index }

    it 'assigns all users to @users' do
      expect(assigns(:users)).to match_array users
    end

    it 'success' do
      expect(response).to be_success
    end
  end

  describe '#show' do
    context 'when requested user exists' do
      let(:user) { users[rand 4] }
      before(:each) { get :show, id: user.id }

      it 'success' do
        expect(response).to be_success
      end

      it 'assigns it to @user' do
        expect(assigns(:user)).to eq user
      end
    end

    context 'when requested user does not exists' do
      it 'throws ActiveRecord::RecordNotFound' do
        expect { get :show, id: -1 }.to raise_exception ActiveRecord::RecordNotFound
      end
    end
  end

  describe '#create' do
    before(:each) { post :create, ** user_attrs }

    context 'when valid' do
      let(:user_attrs) { attributes_for(:user) }

      it 'success' do
        expect(response).to be_success
      end

      it 'saves and assigns new user to @user' do
        user = assigns(:user)

        expect(user).to be_kind_of ActiveRecord::Base
        expect(user).to be_persisted
        expect(users).not_to include user
      end
    end

    context 'when invalid' do
      let(:user_attrs) { attributes_for(:invalid_user) }

      it 'fails' do
        expect(response).not_to be_success
      end

      it 'assigns user to @user' do
        expect(assigns(:user)).to be_kind_of ActiveRecord::Base
      end
    end
  end

  describe '#update' do
    let(:user) { create(:user) }
    before(:each) { patch :update, ** new_values, id: user.id }

    context 'when valid' do
      let(:new_values) { attributes_for(:user) }

      it 'success' do
        expect(response).to be_success
      end

      it 'saves and assigns user to @user' do
        expect(assigns(:user)).to eq user
      end

      it 'saves updates' do
        expect { user.reload }.to change { user.nick }.to(new_values[:nick])
      end
    end

    context 'when invalid' do
      let(:new_values) { attributes_for(:invalid_user) }

      it 'fails' do
        expect(response).not_to be_success
      end

      it 'assigns user to @user' do
        expect(assigns(:user)).to eq user
      end
    end
  end

  describe '#destroy' do
    context 'when requested user exists' do
      let(:user) { users[rand 4] }
      before(:each) { delete :destroy, id: user.id }

      it 'success' do
        expect(response).to be_success
      end

      it 'removes user form DB' do
        expect(User.all).not_to include user
        expect { user.reload }.to raise_exception ActiveRecord::RecordNotFound
      end
    end

    context 'when requested user does not exists' do
      it 'throws ActiveRecord::RecordNotFound' do
        expect { delete :destroy, id: -1 }.to raise_exception ActiveRecord::RecordNotFound
      end
    end
  end
end

Here is controller, model and factory if you need them

It looks really good to me. All I have a very minor quibbles/suggestions.

Since you're using FactoryGirl, you should be able to use create_list :user, 4 instead of the 4.times.map { ... } block

You might as well check that a new record be_kind_of User. Just checking if it's an ActiveRecord is a little loose.

You don't need to create many users for some of these tests (like #show and #destroy). It's fine to do so of course, but the more tests you accumulate the faster you'll want your tests to be. And fully creating several records may be a bottleneck, especially for something like user models that likely have a uniqueness validation. That'll cause Rails to perform extra queries (one to check if the record is unique, and another to insert it in the DB).

Lastly, try running rspec with the --format documentation option, if you're not already. It'll format the output like so

UsersController
    #index
        assigns all users to @users
        success
    #show
        ...

which I personally prefer because it reads well (and, as the name implies, documents stuff). But for that same reason, you may want to change success to succeeds and when valid to something like with valid params just so you get nicer sentences.

Again, this is all really minor stuff.


Drawing simple shapes by mouse dragging I want to make figures of a rectangle and a circle.  How can I make the code more elegant?

import java.awt.*;
import java.awt.event.*;
import javax.swing.*;

class r extends JPanel
{
      public int x1,x2,y1,y2;
      public static double SWITCH;
      public r()
      {
          setBackground(Color.WHITE);
          addMouseListener(new MouseAdapter()
          {
               public void mousePressed(MouseEvent m)
               {
               x1=m.getX();
               y1=m.getY();
               repaint();
               }
               public void mouseReleased(MouseEvent m)
               {
               x2=m.getX();
               y2=m.getY();
               repaint();
               }
          });
          addMouseMotionListener(new MouseMotionAdapter()
          {
              public void mouseDragged(MouseEvent m)
              {
                 x2=m.getX();
                 y2=m.getY();
                 repaint();
              }
          });
      }
      public void paintComponent(Graphics g)
      {
          super.paintComponent(g);
          if(SWITCH == 2)
          {
              g.drawRect(x1, y1, x2, y2);
          }
          else if (SWITCH == 3)
          {
              g.drawOval(x1,y1,x2,y2);
          }
          else
          {
              g.drawString("qwe", x1, y1);
          }
      }

  }
  public class q extends JFrame
  {
      public static void main(String[] args)
      {
          q window = new q();
          window.setVisible(true);
          window.setSize(1024, 800);
          window.setDefaultCloseOperation(EXIT_ON_CLOSE);
          Container cont = window.getContentPane();
          cont.setLayout(new GridLayout(2,2));
          r panel = new r();
          JPanel BPanel = new JPanel();
          cont.add(panel);
          cont.add(BPanel);
          BPanel.setBackground(Color.blue);
          JButton button1,button2;
          button1 = new JButton("Rect");
          button2 = new JButton("Oval");
          BPanel.add(button1);
          BPanel.add(button2);
          button1.addActionListener(new ActionListener() {

              public void actionPerformed(ActionEvent e)
              {
                  r.SWITCH = 2;
              }
          });
          button2.addActionListener(new ActionListener() {

              public void actionPerformed(ActionEvent e)
              {
                  r.SWITCH = 3;
              }
          });
      }
  }

Just a few thoughts...

You are using x and y coordinates. Why not take advantage of the Point class? The MouseEvent class has methods that return Point objects, and it would probably make your code more readable. x1 and y1 don't mean anything to me, but you could probably give meaningful names to the points represented by (x1, y1) and (x2, y2).

The use of magic numbers (you storing 2 and 3 into SWITCH, for example) is a poor practice. Use constant ints or (as Landei suggested in the comments) an enum. It will allow you to improve the readable and understandability of the code - at a first read-through, I didn't get what you were doing.

As per Java standards, a class name should begin with a capital letter. Your class is currently called r. You should give it a meaningful name. You might also want to look into other Java standards for code formatting in terms of brace placement, indentation, and so forth. To me, your code is just plain hard to read because of formatting.

Rather than if and switch/case statements, consider leveraging polymorphism in order to draw different shapes. Move the logic to draw different shapes into concrete subclasses, while perhaps maintaining common logic in a superclass.


Correcting duplicate names in generic array - follow up Follow Up question of : Correcting duplicate names in array

I have an array of file names. For example:

FileContent[] files =
{
    new FileContent() {Content = threeItems, Name = "one.zip" },
    new FileContent() {Content = fiveItems, Name = "one.zip" },
    new FileContent() {Content = sevenItems, Name = "one.zip" },
    new FileContent() {Content = threeItems, Name = "two.zip" },
    new FileContent() {Content = fiveItems, Name = "two.zip" },
    new FileContent() {Content = sevenItems, Name = "two.zip" },
};

Model:

public sealed class FileContent
{
    public byte[] Content { get; set; }

    public string Name { get; set; }
}

And I've developed the following method to correct duplicate names. What the method does is just change duplicate names. I am just adding incremented value for the next duplicated value. For example, my developed method ChangingDuplicateNames(string[] files)correct the previous array to:

FileContent[] files =
{
    new FileContent() {Content = threeItems, Name = "one.zip" },
    new FileContent() {Content = fiveItems, Name = "one(1).zip" },
    new FileContent() {Content = sevenItems, Name = "one(2).zip" },
    new FileContent() {Content = threeItems, Name = "two.zip" },
    new FileContent() {Content = fiveItems, Name = "two(1).zip" },
    new FileContent() {Content = sevenItems, Name = "two(2).zip" },
};

And implementation of ChangingDuplicateNames(FileContent[] files) is:

private FileContent[] ChangingDuplicateNames(FileContent[] files)
{
    //Creating a dicitonary to store duplicated values. "Key" of dictionary
    //is duplicated name, "Value" of dictionary is number to add for name
    Dictionary<string, int> duplicateNames = files.GroupBy(x => x.Name)
            .Where(group => group.Count() > 1)
            .ToDictionary(grouped => grouped.Key, grouped => 0);

    if (duplicateNames.Count == 0)
       return files;

    int namesLength = files.Length;
    string actualName = string.Empty;
    for (int indexArray = 0; indexArray < namesLength; indexArray++)
    {
        int value;
        bool isDuplicate = duplicateNames
           .TryGetValue(files[indexArray].Name, out value);
        if (isDuplicate)
        {
           actualName = files[indexArray].Name;
           if (value == 0)
              files[indexArray].Name = files[indexArray].Name;
           else
           {
              //Adding increment to the mext duplicate name
              string fileNameWithoutExtension = Path
                  .GetFileNameWithoutExtension(files[indexArray].Name);
              string fileExtension = Path
                  .GetExtension(files[indexArray].Name);
              files[indexArray].Name = fileNameWithoutExtension + "(" + value + ")"
+ fileExtension;
           }
           duplicateNames[actualName] = ++value;
        }
    }
    return files;
}

And my question is: Is it possible to improve this algorithm? I mean could be this code smaller?

Maybe I should not iterate through the all array of names, but I cannot figure out how I can change names in files without iterating through all array. Thanks in advance.

Because we are dealing now with objects, we need to create a copy of each FileContent to achieve the same result like in your previous question.

Instead of having var currentFile = file where file had been a string, we now use var currentFile = new FileContent() { Content = file.Content, Name = file.Name };.

In addition, we are using the HashSet<T> only for look up and return an IEnumerable<FileContent> instead of an array.

private IEnumerable<FileContent> ChangingDuplicateNames(FileContent[] files)
{
    var hashSet = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
    foreach (var file in files)
    {
        var currentFile = new FileContent() { Content = file.Content, Name = file.Name };
        int counter = 0;
        while (!hashSet.Add(currentFile.Name))
        {
            currentFile.Name = CreateFileName(file.Name, ref counter);
        }
        yield return currentFile;
    }
}

and call it like so

FileContent[] result = ChangingDuplicateNames(files).ToArray();


WebService --> XML Cache Repository I'm working on a small application that uses a WebService to get meta-data about television episodes.  What I'd like to do is start building out a local cache of the series so that I can reduce the number of calls to the service.  Data being stale is not a concern at all for this case.  I've chosen to use an XML file to implement the local repository.

Here's the code:

public interface IVideo { int SeriesId { get; set; } string Name { get; set; } }

public class TVShow : IVideo { public int SeriesId { get; set; } public string Name { get; set; } }

public class IVideoCollection : List<IVideo>, IXmlSerializable
{
    public IVideoCollection() : base() { }

    #region IXmlSerializable
    public System.Xml.Schema.XmlSchema GetSchema() { return null; }

    public void ReadXml(XmlReader reader)
    {
        reader.ReadStartElement("IVideoCollection");
        while (reader.IsStartElement("IVideo"))
        {
            Type type = Type.GetType(reader.GetAttribute("AssemblyQualifiedName"));
            XmlSerializer serial = new XmlSerializer(type);

            reader.ReadStartElement("IVideo");
            this.Add((IVideo)serial.Deserialize(reader));
            reader.ReadEndElement();
        }
        reader.ReadEndElement();
    }

    public void WriteXml(XmlWriter writer)
    {
        foreach (IVideo vid in this)
        {
            writer.WriteStartElement("IVideo");
            writer.WriteAttributeString("AssemblyQualifiedName", vid.GetType().AssemblyQualifiedName);
            XmlSerializer xmlSerializer = new XmlSerializer(vid.GetType());
            xmlSerializer.Serialize(writer, vid);
            writer.WriteEndElement();
        }
    }
    #endregion
}

public class SeriesRepository
{
    public IVideoCollection Collection;
    public string RepoPath {get;set;}

    public SeriesRepository()
    {
        Collection = new IVideoCollection();
        RepoPath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.MyDocuments), "IVideoCollection.xml");
        if (File.Exists(RepoPath))
        {
            var xmlSerializer = new XmlSerializer(Collection.GetType());
            var xmlReader = XmlReader.Create(new StreamReader(RepoPath));
            Collection = (IVideoCollection)xmlSerializer.Deserialize(xmlReader);
        }
    }

    public void SaveChanges()
    {
        var xmlSerializer = new XmlSerializer(Collection.GetType());
        StreamWriter writer = new StreamWriter(RepoPath);
        xmlSerializer.Serialize(writer, Collection);
        writer.Flush();
        writer.Close();
    }
}

I'm trying looking to see if there are any obvious points of failure that I haven't seen.  Any feedback is appreciated.

The resulting XML looks like:

<?xml version="1.0" encoding="utf-8"?>
<IVideoCollection>
  <IVideo AssemblyQualifiedName="MyNamespace.TVShow, MyNameSpace, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null">
    <TVShow xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
      <SeriesId>55512345</SeriesId>
      <Name>This is the show name</Name>
    </TVShow>
  </IVideo>
</IVideoCollection>

I tend to prefer composition over inheritance, but I see why you inherited from List<T>, and I feel like it's okay, but you'll run into some trouble if you decide that some other type of collection would be better suited to your task. I would spend a little time thinking about what you'd need to do in your client code if you changed the implementation. It may be worth implementing IList by delegating to a private instance of List<IVideo>. Then again, it may not be.

XmlSerializer and StreamWriter both implement IDisposable, so it'd be better to put your usages of these into a using block. For example:

public void SaveChanges()
{
    using (var xmlSerializer = new XmlSerializer(Collection.GetType()))
    using (StreamWriter writer = new StreamWriter(RepoPath))
    {
        xmlSerializer.Serialize(writer, Collection);
    }
}